-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[jobframework] Use the jobs uid in workload's name hash. #1732
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
/cc @alculquicondor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think it would be good to get to the bottom of 1726 before adding UID to the hash. At least I still don't fully understand it, so maybe just explaining would be enough if you already know. I'm worried that with this change with risk covering some issues, and complications on upgrade.
Concrete questions:
- in case there is a conflict in a scenario when a job is recreated with different pod spec, so that the workload is not longer equivalent, should we not trigger recreation the workload?
- what is the impact during upgrade, what will happen to user workloads which are pending, and admitted ones?
I'm ok with this change, but before we merge it, I think we should make sure we are not depending on the predictability of the workload name. |
What happens if admins do cluster migration? I guess that workloads are just recreated with a new name since Job UID is changed by migration, right? |
I still have concerns about this approach, summarized here: #1726 (comment). EDIT: Having said that, I'm ok with this if we find it "least" evil, but I think it warrants a more descriptive release-note, describing the user-facing impact. EDIT2: I checked that if I do in-place upgrade then the jobs are not actually stopped. In that case the concerns aren't that important, I'm ok with this change too. |
@alculquicondor From a human user perspective the wl names will continue to look similar @tenzen-y |
If we directly backup and restore etcd, UID wouldn't be changed, but if we use cluster backup tools like the |
My worry is this line:
|
Yes , this could be problematic in case of kueue upgrades, it could keep the child job Suspended if the workload for the parent was created before the upgrade. A similar problem we have with prebuilt workloads, but only for jobsets at this point, and we do this kueue/pkg/controller/jobs/jobset/jobset_webhook.go Lines 60 to 69 in 01bd532
to overcome it. Maybe we could droop the usage of the parentWlAnotation and cover the prebuilt scenario as well. |
Can you expand on this? |
We can use an ownership based approach, the child job and the worker have the same parent . 4dc9ba0 |
I like that. Let's start with a PR just for that. |
4dc9ba0
to
7cc2922
Compare
Moved in #1747 |
With this we can avoid name collisions for workloads create for jobs having the same name and type created at different pints in time.
7cc2922
to
a3acdd5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, just some nits. Also, a suggestion for potential follow up.
I'm on a fence regarding the release note saying "to avoid potential name collisions". They are rare at rate 1/16mln they are still sort of possible to observe. Maybe we can rephrase a little not to be definitive.
/lgtm |
LGTM label has been added. Git tree hash: fb96d8b5f91182d9714c4384522dfdba0f546a99
|
For the release note, maybe something more user-oriented: WDYT @tenzen-y |
@mimowo Generally, sgtm, but it is better to emphasize that the creating workloads names lose consistency or obtain completely random names. WDYT? |
Yeah, it is tricky, they are "completely random" from user perspective, but technically they are predictible, so I wanted to have something which captures the situation from user perspective (different names on recreation), yet don't say "random". However, I'm ok adding a sentence to clarify "The workload names are virtually random now." (or something like that). |
Cannot be added since they are not random. |
Well, I think saying "virtually" random, or "random from user's perspective" is fine (after saying that it is a hash-like in the previous sentence), but I don't have a strong opinion. Still, I think we should make set the release note to something understandable to users. WDYT @tenzen-y ? |
Release note updated |
@@ -2164,7 +2164,7 @@ func TestReconciler(t *testing.T) { | |||
Effect: corev1.TaintEffectNoSchedule, | |||
}).Obj(), | |||
workloads: []kueue.Workload{ | |||
*utiltesting.MakeWorkload(GetWorkloadNameForJob(baseJobWrapper.Name), "ns"). | |||
*utiltesting.MakeWorkload(GetWorkloadNameForJob(baseJobWrapper.Name, baseJobWrapper.GetUID()), "ns"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have another unit test where we put a completely different name, but the reconciler still recognizes it as the workload for the job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be a bit of a stretch and has nothing to do with the current PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does have to do. It's one minimal safety guard to ensure that existing jobs (before upgrading to the latest version) continue to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add this when owner based workload section was added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of bad foresight, but it wasn't that important then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the vast majority of tests the workload has name a
not the name that is usually generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks for checking. That should be enough.
/approve |
[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:
Approvers can indicate their approval by writing |
Sorry for the late. It sounds good to me. |
…sigs#1732) * [jobframework] Use the jobs uuid in workload's name hash. With this we can avoid name collisions for workloads create for jobs having the same name and type created at different pints in time. * Review Remarks * Review Remarks
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
[jobframework] Use the jobs uuid in workload's name hash. With this we can avoid name collisions for workloads create for jobs having the same name and type created at different pints in time.
Which issue(s) this PR fixes:
Fixes #1726
Special notes for your reviewer:
Does this PR introduce a user-facing change?