Create PodTemplate before ProvisioningRequest #4086
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mbobrovskyi The full list of commands accepted by this bot can be found 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
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
mbobrovskyi
force-pushed
the
fix/create-pod-template-before-provision-request
branch
from
f66cc05 to
3aa0a49
Compare
k8s-ci-robot
added
size/M
mbobrovskyi
force-pushed
the
fix/create-pod-template-before-provision-request
branch
from
3aa0a49 to
0c0efc0
Compare
k8s-ci-robot
added
size/L
|
/cc @PBundyra |
mbobrovskyi
force-pushed
the
fix/create-pod-template-before-provision-request
branch
from
0c0efc0 to
08ad38a
Compare
| newPt := &corev1.PodTemplate{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: ptKey.Name, | ||
| Namespace: ptKey.Namespace, | ||
| Labels: map[string]string{ | ||
| constants.ManagedByKueueLabel: "true", | ||
| }, | ||
| }, | ||
| Template: ps.Template, | ||
| } | ||
|
|
||
| // apply the admission node selectors to the Template | ||
| psi, err := podset.FromAssignment(ctx, c.client, psaMap[psName], podSet.Count) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| err = podset.Merge(&newPt.Template.ObjectMeta, &newPt.Template.Spec, psi) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // copy limits to requests if needed | ||
| workload.UseLimitsAsMissingRequestsInPod(&newPt.Template.Spec) | ||
|
|
||
| if err = c.client.Create(ctx, newPt); err != nil { | ||
| msg := fmt.Sprintf("Error creating PodTemplate %q: %v", newPt.Name, err) | ||
| ac.Message = api.TruncateConditionMessage(msg) | ||
| workload.SetAdmissionCheckState(&wl.Status.AdmissionChecks, *ac, c.clock) | ||
|
|
||
| c.record.Eventf(wl, corev1.EventTypeWarning, "FailedCreate", api.TruncateEventMessage(msg)) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Could we make a separate function for PodTemplate creation?
| if err != nil { | ||
| // it's a not found, so create it | ||
| newPt := &corev1.PodTemplate{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: ptKey.Name, | ||
| Namespace: ptKey.Namespace, | ||
| Labels: map[string]string{ | ||
| constants.ManagedByKueueLabel: "true", | ||
| }, | ||
| }, | ||
| Template: ps.Template, | ||
| } | ||
|
|
||
| // apply the admission node selectors to the Template | ||
| psi, err := podset.FromAssignment(ctx, c.client, psaMap[psName], reqPS.Count) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = podset.Merge(&newPt.Template.ObjectMeta, &newPt.Template.Spec, psi) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // copy limits to requests if needed | ||
| workload.UseLimitsAsMissingRequestsInPod(&newPt.Template.Spec) |
There was a problem hiding this comment.
Since we moved this part to different place, can we name the function accordingly?
| @@ -371,7 +421,7 @@ func (c *Controller) syncProvisionRequestsPodTemplates(ctx context.Context, wl * | |||
| for i := range request.Spec.PodSets { | |||
| reqPS := &request.Spec.PodSets[i] | |||
| psName, refFound := podsetRefsMap[reqPS.PodTemplateRef.Name] | |||
There was a problem hiding this comment.
Do we need those check? Is there any risk of race condition? We are already checking that in the syncOwnedProvisionRequest
There was a problem hiding this comment.
I also wonder if we need to check if the PodSets are the same in Workload and ProvReq:
https://github.com/kubernetes-sigs/kueue/pull/4086/files#diff-d5b88e9a8af6b97ce61788f1307ec9ba1f4e3581a9c0634a151ce310c9ca3d91R412-R416
Since we set them a few lines above. Could you evaluate if there are any scenarios where such a need indeed occurs?
What type of PR is this?
/kind bug
What this PR does / why we need it:
Create
PodTemplatebeforeProvisioningRequest.Which issue(s) this PR fixes:
Fixes #3957
Special notes for your reviewer:
Does this PR introduce a user-facing change?