Code cleanup and refactoring #484
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielvegamyhre 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 |
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/L
✅ Deploy Preview for kubernetes-sigs-jobset canceled.
|
| if inOrderStartupPolicy(startupPolicy) { | ||
| return r.ensureCondition(ctx, ensureConditionOpts{ | ||
| if err := r.ensureCondition(ctx, ensureConditionOpts{ |
There was a problem hiding this comment.
This is changing the behavior.
We want to return here if startup policy is being applied.
There was a problem hiding this comment.
Yeah sorry I forgot to put a note here explaining this change. My thought was this was a bug that we are returning early here, because otherwise we never set the condition below that indicates the JobSet has been resumed; we only add the condition that startup policy is done. Wdyt?
There was a problem hiding this comment.
I did that by design.
I return the jobset to trigger a reconcile. I only want to resume the jobset once all the jobs have resumed.
There was a problem hiding this comment.
Why I return is that I don't want to go to the next job in the loop until the previous job is started.
I think this could potentially start the next job once the previously is resumed.
There was a problem hiding this comment.
Why I return is that I don't want to go to the next job in the loop until the previous job is started.
This is outside the loop though. Inside the loop we still do the early return on line 361, to wait for the replicatedJob we just resumed to be ready before moving on to the next.
There was a problem hiding this comment.
Ha. Yea I guess that is so..
There was a problem hiding this comment.
imo I would really try to keep bug fixes separate from refactor PRs. It makes backports and such much more difficult..
/hold
There was a problem hiding this comment.
Could you move this fix into a separate PR?
|
/lgtm |
k8s-ci-robot
added
lgtm
|
Thanks for the quick review @kannon92 ! We can go ahead and merge since this is just refactoring and a minor bug fix. I will keep bug fixes separate from refactors going forward as you mentioned /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Code cleanup and refactoring
suspendJobSettosuspendJobsto follow same pattern as createJobs and deleteJobs, as well as improve readability (since before the logic read "if jobset is suspended, suspend jobset"resumeJobsIfNecessarysimilarly to abovegenerateStartupPolicyCondition(metav1.True or metav1.False)" into two functions for readability, which better indicate the possible conditions and their semantic meaning:rjobStartedtoallReplicasStartedto better indicate what exactly is required for a replicatedJob to be considered started.