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.

Sign up for GitHub

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

Jump to bottom

WIP: ✨ Add the liveness and readiness probe in the manager deployment #1811

Closed
wants to merge 1 commit into from
Closed

WIP: ✨ Add the liveness and readiness probe in the manager deployment #1811

wants to merge 1 commit into from

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Nov 10, 2020 •
edited
Loading

description

Cherry-pick of #1795

motivation
Re-introduce the change to close the #1761
Note that it was reverted because the CI was not doing tthe e2e tests with the v3-alpha plugin which is done now.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 10, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2020
estroz
estroz reviewed Nov 10, 2020
View reviewed changes
pkg/plugin/v3/scaffolds/internal/templates/config/manager/config.go Outdated
livenessProbe:
httpGet:
path: /readyz
port: 9443
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to set HealthProbeBindAddress in main.go for this to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in #1856

@camilamacedo86 camilamacedo86 changed the title ✨ Add the liveness and readiness probe in the manager deployment WIP: ✨ Add the liveness and readiness probe in the manager deployment Nov 10, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 11, 2020
camilamacedo86
camilamacedo86 commented Nov 11, 2020
View reviewed changes
pkg/plugin/v3/scaffolds/internal/templates/main.go
@@ -260,6 +264,9 @@ func main() {

%s

_ = mgr.AddHealthzCheck("check", func(_ *http.Request) error { return nil })
_ = mgr.AddReadyzCheck("check", func(_ *http.Request) error { return nil })
Copy link
Member Author

@camilamacedo86 camilamacedo86 Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Required to check we really need to add.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have verified that we need this change to actually serve the readyz and healthz endpoints

camilamacedo86
camilamacedo86 commented Nov 11, 2020
View reviewed changes
testdata/project-v3-addon/config/default/manager_auth_proxy_patch.yaml
@@ -21,5 +21,6 @@ spec:
name: https
- name: manager
args:
Copy link
Member Author

Choose a reason for hiding this comment

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

we might need to setup the auth proxy to allow that ( - -skip-auth-regex=^/readyz )

@k8s-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-kubebuilder-e2e-k8s-1-17-0 c93d4a3 link /test pull-kubebuilder-e2e-k8s-1-17-0
pull-kubebuilder-e2e-k8s-1-18-0 c93d4a3 link /test pull-kubebuilder-e2e-k8s-1-18-0
pull-kubebuilder-e2e-k8s-1-15-3 c93d4a3 link /test pull-kubebuilder-e2e-k8s-1-15-3
pull-kubebuilder-e2e-k8s-1-16-2 c93d4a3 link /test pull-kubebuilder-e2e-k8s-1-16-2
pull-kubebuilder-e2e-k8s-1-14-1 c93d4a3 link /test pull-kubebuilder-e2e-k8s-1-14-1

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2020
@k8s-ci-robot
Copy link
Contributor

@camilamacedo86: PR needs rebase.

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.

@prafull01
Copy link
Contributor

@camilamacedo86 asked me to see this through while she is on PTO. I have raised another PR #1856 for this. Hence this PR can be closed.

@camilamacedo86 camilamacedo86 deleted the enable-1761 branch November 23, 2020 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estroz estroz estroz left review comments

@prafull01 prafull01 prafull01 left review comments

@droot droot Awaiting requested review from droot

@mengqiy mengqiy Awaiting requested review from mengqiy

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@camilamacedo86 @k8s-ci-robot @prafull01 @estroz