Record original node selectors #660

trasc · 2023-03-24T13:47:25Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Records the original node selectors in a annotation kueue.x-k8s.io/original-selectors, the content of this annotation is used to restore the "job's" node selectors content if a workload is not present when a the job is suspended (stopJob), this being the case when a workload is deleted while it's admitted.

Which issue(s) this PR fixes:

Fixes #518

Special notes for your reviewer:

netlify · 2023-03-24T13:47:33Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`776aefa`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/642d0c9f721b160008668412
😎 Deploy Preview	https://deploy-preview-660--kubernetes-sigs-kueue.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-03-24T13:47:35Z

Hi @trasc. 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.

trasc · 2023-03-24T13:49:08Z

/cc @alculquicondor

/cc @mwielgus

/cc @mimowo

tenzen-y · 2023-03-24T18:30:27Z

/ok-to-test

mimowo

Some comments around error handling though, and a question if there is a possibility of a malicious actor injecting unmarshalable node selectors. Either directly in the job annotation or via a recreated workload object.

pkg/controller/jobframework/constants.go

pkg/controller/jobframework/reconciler.go

test/integration/controller/mpijob/mpijob_controller_test.go

pkg/controller/jobframework/constants.go

pkg/controller/jobframework/reconciler.go

kerthcet · 2023-03-28T10:04:06Z

pkg/controller/jobframework/constants.go

+	// node selectors are recorded upon a workload admission. This information,
+	// if present, will be used to restore them if a workload is deleted while
+	// it is admitted. The content is a json marshaled slice of selectors.
+	OriginalNodeSelectorsAnnotation = "kueue.x-k8s.io/original-node-selectors"


Have we weighted the suggestion? #518 (comment)

Yes, but it will unnecessary complicate the workload lifecycle. Also setting the annotation is done without any extra api calls.

The annotation is also useful as a record for users to look at

test/integration/controller/job/job_controller_test.go

pkg/controller/jobframework/reconciler.go

trasc · 2023-03-31T08:53:27Z

/test pull-kueue-test-integration-main

mimowo

A couple more non-blocking nits. @alculquicondor over to you.

pkg/controller/jobs/job/job_webhook_test.go

pkg/controller/jobframework/validation.go

test/integration/controller/job/job_controller_test.go

test/integration/controller/mpijob/mpijob_controller_test.go

pkg/controller/jobframework/reconciler.go

alculquicondor · 2023-03-31T16:04:16Z

pkg/controller/jobframework/reconciler.go

+	if err != nil {
+		log.V(3).Error(err, "Unable to get original node selectors")
+	} else {
+		job.RestoreNodeAffinity(selectors)


we could fallback into getting the selectors from the workload object

It was my original approach but was change during the review.

IIRC in the original approach you first looked up the workload object and then fallback to annotation.

Anyway, I think that once we made the annotation immutable, we can fully rely on it, unless I'm missing some scenario.

If such a scenario exists my point was that we should add a comment why this is done (what is the scenario). Otherwise we will end up in a suspicious code which no-one remembers / knows why needed.

The order was inverted due to performance reasons, getting the selectors from the workload wold not have needed additional un-marshaling.

The scenario wold be when a job is missing the annotation, could happen if the job (other than core.Job or MPIJob) in question is not blocking the change of the annotation while running.

Yes, but we have a webhook to prevent the change. So, the only scenario is that the webhook is malfunctioning for some reason and we have a bad actor.

However, if the webhook is malfunctioning and we have a bad actor, the actor could both modify the annotation and delete the workload so the fallback would not work either. So, iiuc, it would not be bullet proof either, just misleadingly making that impression.

pkg/controller/jobframework/reconciler.go

pkg/controller/jobframework/validation.go

pkg/controller/jobs/mpijob/mpijob_controller.go

Save and try to restore the original node selectors in/from a job annotation "kueue.x-k8s.io/original-selectors".

pkg/controller/jobframework/validation.go

pkg/controller/jobframework/reconciler.go

pkg/controller/jobs/mpijob/mpijob_controller.go

alculquicondor · 2023-04-03T19:15:46Z

/approve
/lgtm
/hold
Feel free to lift the hold if you want to address the suggestions in a follow up.

k8s-ci-robot · 2023-04-03T19:15:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, trasc

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~~ [alculquicondor]

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

alculquicondor · 2023-04-03T19:16:22Z

/label tide/merge-method-squash

trasc · 2023-04-05T05:54:52Z

/unhold

alculquicondor · 2023-04-05T12:12:28Z

/lgtm

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 24, 2023

k8s-ci-robot requested review from ahg-g and kerthcet March 24, 2023 13:47

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 24, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 24, 2023

k8s-ci-robot requested review from alculquicondor, mimowo and mwielgus March 24, 2023 13:49

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 Mar 24, 2023

mimowo reviewed Mar 27, 2023

View reviewed changes

trasc force-pushed the record_restore_original_node_selectors branch from 6a6c502 to f1ab6b8 Compare March 27, 2023 15:31

trasc requested review from mimowo and removed request for alculquicondor, mwielgus, kerthcet and ahg-g March 27, 2023 15:34

kerthcet reviewed Mar 28, 2023

View reviewed changes

mimowo reviewed Mar 28, 2023

View reviewed changes

test/integration/controller/job/job_controller_test.go Outdated Show resolved Hide resolved

pkg/controller/jobframework/reconciler.go Outdated Show resolved Hide resolved

trasc force-pushed the record_restore_original_node_selectors branch from f1ab6b8 to 56be3aa Compare March 28, 2023 12:04

trasc requested review from mimowo, kerthcet and alculquicondor and removed request for mimowo and kerthcet March 29, 2023 08:06

trasc force-pushed the record_restore_original_node_selectors branch from 4d500be to 68bc891 Compare March 31, 2023 08:41

mimowo reviewed Mar 31, 2023

View reviewed changes

alculquicondor added this to the v0.4 milestone Mar 31, 2023

alculquicondor reviewed Mar 31, 2023

View reviewed changes

trasc added 3 commits April 3, 2023 10:27

[jobframework] Improve node selectors tracking

3568614

Save and try to restore the original node selectors in/from a job annotation "kueue.x-k8s.io/original-selectors".

[test/integration/jobs] Check selectors restoration on workload deletion

79a2871

[jobframework] Validate original node selectors

1022e23

trasc force-pushed the record_restore_original_node_selectors branch from 68bc891 to 1022e23 Compare April 3, 2023 07:28

alculquicondor reviewed Apr 3, 2023

View reviewed changes

pkg/controller/jobframework/validation.go Show resolved Hide resolved

pkg/controller/jobframework/reconciler.go Outdated Show resolved Hide resolved

pkg/controller/jobs/mpijob/mpijob_controller.go Show resolved Hide resolved

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

k8s-ci-robot assigned alculquicondor Apr 3, 2023

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

alculquicondor modified the milestones: v0.4, v0.3 Apr 3, 2023

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

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 3, 2023

[jobframework] Record podSet name in originalNodeSelectors annotation

776aefa

trasc force-pushed the record_restore_original_node_selectors branch from aac2e55 to 776aefa Compare April 5, 2023 05:52

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

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 5, 2023

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

k8s-ci-robot merged commit c25bbc0 into kubernetes-sigs:main Apr 5, 2023

trasc mentioned this pull request Apr 19, 2023

REQUEST: New membership for trasc kubernetes/org#4168

Closed

9 tasks

trasc deleted the record_restore_original_node_selectors branch April 20, 2023 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record original node selectors #660

Record original node selectors #660

trasc commented Mar 24, 2023

netlify bot commented Mar 24, 2023 •

edited

Loading

k8s-ci-robot commented Mar 24, 2023

trasc commented Mar 24, 2023

tenzen-y commented Mar 24, 2023

mimowo left a comment

kerthcet Mar 28, 2023

trasc Mar 28, 2023

alculquicondor Mar 31, 2023

trasc commented Mar 31, 2023

mimowo left a comment

alculquicondor Mar 31, 2023

trasc Mar 31, 2023

mimowo Apr 3, 2023 •

edited

Loading

trasc Apr 3, 2023

mimowo Apr 3, 2023 •

edited

Loading

alculquicondor commented Apr 3, 2023

k8s-ci-robot commented Apr 3, 2023

alculquicondor commented Apr 3, 2023

trasc commented Apr 5, 2023

alculquicondor commented Apr 5, 2023

Record original node selectors #660

Record original node selectors #660

Conversation

trasc commented Mar 24, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

netlify bot commented Mar 24, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

k8s-ci-robot commented Mar 24, 2023

trasc commented Mar 24, 2023

tenzen-y commented Mar 24, 2023

mimowo left a comment

Choose a reason for hiding this comment

kerthcet Mar 28, 2023

Choose a reason for hiding this comment

trasc Mar 28, 2023

Choose a reason for hiding this comment

alculquicondor Mar 31, 2023

Choose a reason for hiding this comment

trasc commented Mar 31, 2023

mimowo left a comment

Choose a reason for hiding this comment

alculquicondor Mar 31, 2023

Choose a reason for hiding this comment

trasc Mar 31, 2023

Choose a reason for hiding this comment

mimowo Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

trasc Apr 3, 2023

Choose a reason for hiding this comment

mimowo Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

alculquicondor commented Apr 3, 2023

k8s-ci-robot commented Apr 3, 2023

alculquicondor commented Apr 3, 2023

trasc commented Apr 5, 2023

alculquicondor commented Apr 5, 2023

netlify bot commented Mar 24, 2023 •

edited

Loading

mimowo Apr 3, 2023 •

edited

Loading

mimowo Apr 3, 2023 •

edited

Loading