Update workload AdmissionChecks status on AdmissionRequest creation error.

[mbobrovskyi]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Update workload AdmissionChecks status on AdmissionRequest creation error.

Which issue(s) this PR fixes:

Fixes #3025

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix the bug that prevented Kueue from updating the AdmissionCheck state in the Workload status on a ProvisioningRequest creation error.

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	a2d05b0
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/679cc7bc5004340008eac744

[mbobrovskyi]

/cc @mimowo

[mimowo]

Please describe why we need this PR in the description and release note that is focused on what is fixed from the user perspective.

cc @PBundyra or @gabesaba could you help with the first pass?

[PBundyra]

Other than my comment above, LGTM

[PBundyra]

Please describe why we need this PR in the description and release note that is focused on what is fixed from the user perspective.

cc @PBundyra or @gabesaba could you help with the first pass?

The rationale behind this PR is that we haven't been actually patching the changes we make if ProvReq's creations fails:
https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/admissionchecks/provisioning/controller.go#L309-L311

[mimowo]

as I synced with @mbobrovskyi this is likely a bug which made #3056 not working. I believe we should think of better testing. Maybe an integration or e2e with a custom webhook?

EDIT: I'm ok with just manual testing for this PR if this unblocks #4086 (as I was told), but please open an issue for an automated testing at the integration level.

EDIT2: not sure how complex it is to deploy in Kueue a webhook during integration tests (might be simpler or harder depending on the testing frameworks). FWIIW we do it in core k8s at the integration and e2e level (example integration, example e2e), but I agree it would be overkill for this PR.

[mimowo]

proposal, is it accurate?
/release-note-edit

Fix the bug that prevented Kueue from updating the AdmissionCheck state in the Workload status on a ProvisioningRequest creation error.

[mbobrovskyi]

I'm ok with just manual testing for this PR if this unblocks #4086 (as I was told).

I manually tested it, and it works fine. I added CEL validation for spec.podSets[0].podTemplateRef.name to prevent creating a ProvisioningRequest with a name containing xyz. The AdmissionChecks status message has been updated:

Error creating ProvisioningRequest "xyz-ac-prov-1": ProvisioningRequest.autoscaling.x-k8s.io "xyz-ac-prov-1" is invalid: [spec.podSets[0].podTemplateRef.name: Invalid value: "string": The name must not contain 'xyz', spec.podSets[1].podTemplateRef.name: Invalid value: "string": The name must not contain 'xyz']

please open an issue for an automated testing at the integration level.

Created an issue #4115 for follow-ups.

[mimowo]

/lgtm
/approve
Looks great!, thanks for manual double checking!

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 2c328aeb3b039c3aff6bf5e4f078f14de8e67822

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbobrovskyi, mimowo

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 [mimowo]

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

[mimowo]

/cherry-pick release-0.10 release-0.9

[k8s-infra-cherrypick-robot]

@mimowo: once the present PR merges, I will cherry-pick it on top of release-0.10 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.10 release-0.9

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-sigs/prow repository.

[k8s-infra-cherrypick-robot]

@mimowo: #4114 failed to apply on top of branch "release-0.10":

Applying: Update workload AdmissionChecks on AdmissionRequest creation error.
Using index info to reconstruct a base tree...
M	pkg/controller/admissionchecks/provisioning/controller.go
M	pkg/controller/admissionchecks/provisioning/controller_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/admissionchecks/provisioning/controller_test.go
Auto-merging pkg/controller/admissionchecks/provisioning/controller.go
CONFLICT (content): Merge conflict in pkg/controller/admissionchecks/provisioning/controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Update workload AdmissionChecks on AdmissionRequest creation error.

In response to this:

/cherry-pick release-0.10 release-0.9

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-sigs/prow repository.

[mimowo]

@mbobrovskyi please prepare cherry-picks manually