Move exclusive placement annotation to ReplicatedJob template #389
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[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
the
approved
k8s-ci-robot
added
the
size/M
danielvegamyhre
changed the title
|
For backwards compatibility, we shouldn't move, but rather we should support both. If set at the jobset level, then apply to all replicatedJobs regardless whether or not it is set at the replicatedJob. |
I see, yeah I was planning on messaging you on Monday to discuss the best way to support backward compatibilty for users still using the JobSet level annotation. I think the most intuitive way to do so would be to document that if the JobSet level annotation is set, the ReplicatedJob level annotations will not matter, regardless if they are set or not. I will update the PR to support both. |
k8s-ci-robot
added
size/L
| jobIdx int | ||
| restarts int | ||
| topology string | ||
| jobSetName string |
There was a problem hiding this comment.
We can do this as a followup, but we are using the wrapper pattern, we shouldn't need a makeJobArgs struct, we should be able to do something like
makeJob(name, namespace)
.ReplicatedJobName(replicatedJobName)
.Replicas(n)
| if topologyDomain, exists := rjob.Template.Annotations[jobset.ExclusiveKey]; exists { | ||
| annotations[jobset.ExclusiveKey] = topologyDomain | ||
| // Check if we are using nodeSelectorStrategy implementation of exclusive placement at the ReplicatedJob level. | ||
| if value, ok := rjob.Template.Annotations[jobset.NodeSelectorStrategyKey]; ok { |
There was a problem hiding this comment.
This labelAndAnnotate function is invoked for both the job template and the pod template. The job template inherits gets this already because the job is created out of rjob.Template anyways, and we don't need to set this annotation on the pod.
There was a problem hiding this comment.
anyways, we can clean this up when we add the canonical placement policy API.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Fixes #344