Skip to content
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

Use DeactivationTarget to improve message when deactivating due to Rejected AdmissionChecks #3331

Closed
mimowo opened this issue Oct 28, 2024 · 7 comments · Fixed by #3350
Closed
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@mimowo
Copy link
Contributor

mimowo commented Oct 28, 2024

What would you like to be added:

When deactivating due to rejected admission checks we just active=false:

log.V(3).Info("Workload is evicted due to admission checks")
if workload.HasRejectedChecks(wl) {
wl.Spec.Active = ptr.To(false)
if err := r.client.Update(ctx, wl); err != nil {
return false, err
}
rejectedCheck := workload.RejectedChecks(wl)[0]
r.recorder.Eventf(wl, corev1.EventTypeWarning, "AdmissionCheckRejected", "Deactivating workload because AdmissionCheck for %v was Rejected: %s", rejectedCheck.Name, rejectedCheck.Message)
return true, nil
}

Then, the generic message for the Evicted condition is set here.

The message can be improved when DeactivationTarget is used, see here.

The message candidate: The workload is deactivated due to rejected AdmissionChecks(s): check1, check2.

Why is this needed:

To inform the end user about the reason of deactivation.

@mimowo mimowo added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 28, 2024
@mimowo
Copy link
Contributor Author

mimowo commented Oct 28, 2024

cc @PBundyra @tenzen-y @mbobrovskyi

@tenzen-y
Copy link
Member

I was supposed to improve this mechanism in #1353.
So, I guess that @PBundyra is implementing this improvement now.

@mimowo
Copy link
Contributor Author

mimowo commented Nov 5, 2024

I see @KPostOffice is already working on this under #3350

@mimowo
Copy link
Contributor Author

mimowo commented Nov 5, 2024

/assign @KPostOffice

@k8s-ci-robot
Copy link
Contributor

@mimowo: GitHub didn't allow me to assign the following users: KPostOffice.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @KPostOffice

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
Copy link
Contributor Author

mimowo commented Nov 5, 2024

@KPostOffice you should be able to assign yourself

@KPostOffice
Copy link
Contributor

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants