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

Update webhook failurePolicy from Ignore to Fail #1784

Closed
wants to merge 2 commits into from

Conversation

knkski
Copy link
Contributor

@knkski knkski commented Jan 19, 2022

What this PR does / why we need it:

Updates katib-controller webhook for operator and manifests to set failurePolicy to Fail. This allows for greater visibility on errors. Also updates webhook creation handling in katib-controller operator.

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: knkski
To complete the pull request process, please assign gaocegege after the PR has been reviewed.
You can assign the PR to them by writing /assign @gaocegege in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knkski knkski changed the title Update webhook failurePolicy from Fail to Ignore Update webhook failurePolicy from Ignore to Fail Jan 19, 2022
Updates katib-controller webhook for operator and manifests to set
failurePolicy to Fail. This allows for greater visibility on errors.
@coveralls
Copy link

coveralls commented Jan 19, 2022

Coverage Status

Coverage decreased (-0.05%) to 74.088% when pulling 68e2761 on knkski:webhook-failure-policy into f5abfd0 on kubeflow:master.

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this update @knkski!
Do we want to include this change to Katib 0.13 release ?

@andreyvelich
Copy link
Member

/retest

@andreyvelich
Copy link
Member

It seems that this change doesn't work on K8s 1.18.
@knkski Have you tested this change on that version ?

@knkski
Copy link
Contributor Author

knkski commented Jan 20, 2022

I've only tested as far back as 1.21. It does look like mutating/validating webhooks in 1.18 have a failurePolicy field that allows for Fail or Ignore, so I'm not sure offhand why it wouldn't work. The docs say that the field defaults to Fail, which is what this change does anyways, so I can try just removing the field altogether.

As far as which version to release this in, no strong preference. Whichever works best for you.

Adds new charmcraft.yaml that is required by charmhub.io. Also adds name
field to bundle.yaml, as required by charmhub.io.
@aws-kf-ci-bot
Copy link
Contributor

@knkski: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
kubeflow-katib-presubmit 68e2761 link /test kubeflow-katib-presubmit

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

@@ -53,7 +51,6 @@ webhooks:
- experiments
- name: mutator.pod.katib.kubeflow.org
sideEffects: None
failurePolicy: Ignore
Copy link
Member

@tenzen-y tenzen-y Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @knkski !
I think that we need to specify Ignore in mutator.pod.katib.kubeflow.org since the katib-cert-generator job generates a Pod before the certificate is injected into MutatingWebhookConfiguration.

@knkski
Copy link
Contributor Author

knkski commented Feb 4, 2022

Apologies for the delay in responding, @DomFleischmann will be taking this over

@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@andreyvelich
Copy link
Member

We can close this in favour to PR: #2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants