Define new selector for building image job #311

erusso7 · 2023-02-23T16:18:56Z

netlify · 2023-02-23T16:19:01Z

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name	Link
🔨 Latest commit	`b6fb69c`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/643d4453477aaf000847cca3
😎 Deploy Preview	https://deploy-preview-311--kubernetes-sigs-kmm.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

k8s-ci-robot · 2023-02-23T16:19:05Z

Hi @erusso7. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov-commenter · 2023-02-23T16:23:47Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03 ⚠️

Comparison is base (d8ed348) 82.27% compared to head (b6fb69c) 82.25%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
- Coverage   82.27%   82.25%   -0.03%     
==========================================
  Files          31       31              
  Lines        3075     3082       +7     
==========================================
+ Hits         2530     2535       +5     
- Misses        448      450       +2     
  Partials       97       97

Impacted Files	Coverage Δ
api/v1beta1/module_types.go	`100.00% <ø> (ø)`
internal/build/job/maker.go	`90.49% <100.00%> (+0.17%)`	⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

yevgeny-shnaidman · 2023-02-24T14:18:21Z

internal/build/job/maker.go

@@ -129,6 +129,11 @@ func (m *maker) specTemplate(
 		kanikoImage += ":" + buildConfig.KanikoParams.Tag
 	}

+	nodeSelector := mld.Selector


why do we need to set the nodeSelector at all for Builds? Why not let kubernetes decide where to run the Build job?

This is a good point. Removing it completely will also solve #140 (comment)

Why not let kubernetes decide where to run the Build job?

This comes from that issue. You may want to reserve nodes equipped with expensive hardware for some workloads only, for example.

I don't see in that issue any mention of nodes with expensive hardware. They just say that it would be nice if build/sign can also run on other nodes, and not only those with specific hardware. Also, allowing kuberentes to pick the running node makes use of the schedule, which probably has much more data to decide which nodes are less "overloaded"

I have understood the issue the same way as @yevgeny-shnaidman did.

Also, allowing kuberentes to pick the running node makes use of the schedule, which probably has much more data to decide which nodes are less "overloaded"

This change makes a user able to do just that, with selector: {} in the build section.

I agree, but why give him an option at all?

ybettan · 2023-02-27T09:47:02Z

/ok-to-test

ybettan · 2023-02-27T09:51:16Z

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kernel-module-management/311/pull-kernel-module-management-check-api-changes/1630142470052384768#1:build-log.txt%3A201

Please run `make generate bundle` and repush

api/v1beta1/module_types.go

qbarrand · 2023-03-01T10:32:03Z

/hold
This change may brake flows for users building for a specific architecture in multi-arch clusters.
Let us consider a cluster with amd64 and arm64 nodes.
Before this change, if you created a Module targeting amd64 nodes, the builds would be automatically scheduled on amd64 nodes.
With this change, the build may run on an arm64 node, which by default will produce a module for arm64, not amd64.

cc @hershpa @yevgeny-shnaidman @ybettan

yevgeny-shnaidman · 2023-03-01T11:24:47Z

/hold This change may brake flows for users building for a specific architecture in multi-arch clusters. Let us consider a cluster with amd64 and arm64 nodes. Before this change, if you created a Module targeting amd64 nodes, the builds would be automatically scheduled on amd64 nodes. With this change, the build may run on an arm64 node, which by default will produce a module for arm64, not amd64.

cc @hershpa @yevgeny-shnaidman @ybettan

@qbarrand maybe all we need to add a label to the Build/Sign job node selector is architecture label. I think that all the nodes are label with it anyways, and it can be derived from the KernelMapping

ybettan · 2023-03-01T11:38:22Z

This change may brake flows for users building for a specific architecture in multi-arch clusters.
Let us consider a cluster with amd64 and arm64 nodes.
Before this change, if you created a Module targeting amd64 nodes, the builds would be automatically scheduled on amd64 nodes.
With this change, the build may run on an arm64 node, which by default will produce a module for arm64, not amd64.

@qbarrand @yevgeny-shnaidman I would instead just ask kaniko to build for a specific arch (regardless of the host) using https://github.com/GoogleContainerTools/kaniko#flag---customplatform.

This will also allow to build for ARM in a x86 cluster or any other combination. This is especially handy in the hub-spoke topology.

This will requires extending the build section in our CRD though.

yevgeny-shnaidman · 2023-03-01T11:52:55Z

This change may brake flows for users building for a specific architecture in multi-arch clusters.
Let us consider a cluster with amd64 and arm64 nodes.
Before this change, if you created a Module targeting amd64 nodes, the builds would be automatically scheduled on amd64 nodes.
With this change, the build may run on an arm64 node, which by default will produce a module for arm64, not amd64.

@qbarrand @yevgeny-shnaidman I would instead just ask kaniko to build for a specific arch (regardless of the host) using https://github.com/GoogleContainerTools/kaniko#flag---customplatform.

This will also allow to build for ARM in a x86 cluster or any other combination. This is especially handy in the hub-spoke topology.

This will requires extending the build section in our CRD though.

@ybettan does kaniko support something like that? build Dockerfile will probably include Makefile that needs to know how to cross-compile, not sure if customer will want to support it.

ybettan · 2023-03-02T09:00:14Z

This PR is blocked by #325 unless we modify the CRD to contain a nodeSelector for build/sign which is not ideal as we wish not to extend the API unless there is a requirement for it.

qbarrand · 2023-03-08T13:55:10Z

we wish not to extend the API unless there is a requirement for it

Sounds like it's the only way to properly address #140 though.

ybettan · 2023-03-13T09:46:44Z

@qbarrand
We have kubernetes.io/arch: amd64 on the nodes, how about we just set the inner selector of the build to match the same arch as of the module's selector node?

ybettan · 2023-03-22T16:27:57Z

As agreed in the community meeting we will proceed with the following:

We add a new selector for build. Regarding the default value we differentiate between v1 and v2.

v1:
If the build selector is not set, we use the selector from the module to maintain the same behavior that we had until now.

v2:
If the build selector is not set, we do not set any selector for the build job and let k8s make the scheduling.

k8s-ci-robot · 2023-03-31T14:01:32Z

@erusso7: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kernel-module-management-test-deploy	`9514d5a`	link	true	`/test pull-kernel-module-management-test-deploy`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

internal/build/job/maker.go

internal/sign/job/signer.go

Signed-off-by: Erusso7 <[email protected]>

ybettan · 2023-04-17T13:54:19Z

/unhold
/lgtm
/assign @qbarrand @yevgeny-shnaidman

yevgeny-shnaidman · 2023-04-18T06:06:18Z

/approve

k8s-ci-robot · 2023-04-18T06:06:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erusso7, yevgeny-shnaidman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yevgeny-shnaidman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested a review from ybettan February 23, 2023 16:19

k8s-ci-robot requested a review from yevgeny-shnaidman February 23, 2023 16:19

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 23, 2023

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 23, 2023

yevgeny-shnaidman reviewed Feb 24, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 27, 2023

ybettan reviewed Feb 27, 2023

View reviewed changes

api/v1beta1/module_types.go Show resolved Hide resolved

erusso7 force-pushed the separate-selectors branch from e9b3df8 to 9514d5a Compare February 28, 2023 10:49

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 28, 2023

qbarrand linked an issue Mar 1, 2023 that may be closed by this pull request

Separate node selector for build process #140

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2023

erusso7 force-pushed the separate-selectors branch from 9514d5a to f4c8942 Compare March 31, 2023 13:58

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 31, 2023

erusso7 marked this pull request as draft March 31, 2023 14:15

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 31, 2023

erusso7 force-pushed the separate-selectors branch 2 times, most recently from cc8dd8c to 13cb6dc Compare April 5, 2023 13:18

erusso7 marked this pull request as ready for review April 5, 2023 18:12

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2023

k8s-ci-robot requested review from qbarrand and ybettan April 5, 2023 18:12

ybettan reviewed Apr 17, 2023

View reviewed changes

internal/build/job/maker.go Outdated Show resolved Hide resolved

ybettan reviewed Apr 17, 2023

View reviewed changes

internal/sign/job/signer.go Outdated Show resolved Hide resolved

Enable build.Selector but keeping BC with module selector

b6fb69c

Signed-off-by: Erusso7 <[email protected]>

erusso7 force-pushed the separate-selectors branch from 13cb6dc to b6fb69c Compare April 17, 2023 13:06

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2023

k8s-ci-robot assigned qbarrand, yevgeny-shnaidman and ybettan Apr 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2023

k8s-ci-robot merged commit bb7b4e4 into kubernetes-sigs:main Apr 18, 2023

Define new selector for building image job #311

Define new selector for building image job #311

Uh oh!

Conversation

erusso7 commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Uh oh!

k8s-ci-robot commented Feb 23, 2023

Uh oh!

codecov-commenter commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yevgeny-shnaidman Feb 24, 2023

Choose a reason for hiding this comment

Uh oh!

ybettan Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

qbarrand Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

yevgeny-shnaidman Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

ybettan Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

qbarrand Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

yevgeny-shnaidman Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

ybettan commented Feb 27, 2023

Uh oh!

ybettan commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

qbarrand commented Mar 1, 2023

Uh oh!

yevgeny-shnaidman commented Mar 1, 2023

Uh oh!

ybettan commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yevgeny-shnaidman commented Mar 1, 2023

Uh oh!

ybettan commented Mar 2, 2023

Uh oh!

qbarrand commented Mar 8, 2023

Uh oh!

ybettan commented Mar 13, 2023

Uh oh!

ybettan commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ybettan commented Apr 17, 2023

Uh oh!

yevgeny-shnaidman commented Apr 18, 2023

Uh oh!

k8s-ci-robot commented Apr 18, 2023

Uh oh!

Uh oh!

erusso7 commented Feb 23, 2023 •

edited

Loading

netlify bot commented Feb 23, 2023 •

edited

Loading

codecov-commenter commented Feb 23, 2023 •

edited

Loading

ybettan commented Feb 27, 2023 •

edited

Loading

ybettan commented Mar 1, 2023 •

edited

Loading

ybettan commented Mar 22, 2023 •

edited

Loading

k8s-ci-robot commented Mar 31, 2023 •

edited

Loading