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

Fix /readyz to contain informer-sync #93670

Merged
merged 1 commit into from Aug 5, 2020
Merged

Fix /readyz to contain informer-sync #93670

merged 1 commit into from Aug 5, 2020

Conversation

wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Aug 4, 2020

Ref #93599

Fix kube-apiserver /readyz to contain "informer-sync" check ensuring that internal informers are synced.

/kind bug

@wojtek-t wojtek-t self-assigned this Aug 4, 2020
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 4, 2020
@k8s-ci-robot k8s-ci-robot added area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 4, 2020
@wojtek-t wojtek-t changed the title [WIP] Fix /readyz to contain informer-sync Fix /readyz to contain informer-sync Aug 4, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 4, 2020
@wojtek-t wojtek-t assigned liggitt and unassigned wojtek-t Aug 4, 2020
@mm4tt
Copy link
Contributor

mm4tt commented Aug 4, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2020
@wojtek-t
Copy link
Member Author

wojtek-t commented Aug 4, 2020

/hold

For @liggitt to take a look

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2020
@wojtek-t
Copy link
Member Author

wojtek-t commented Aug 4, 2020

/retest

@wojtek-t
Copy link
Member Author

wojtek-t commented Aug 4, 2020

/retest
[verify timeout]

liggitt
liggitt reviewed Aug 4, 2020
View reviewed changes
test/e2e/apimachinery/health_handlers.go Outdated
)

var (
expectedHealthzVerbose = `[+]ping ok
Copy link
Member

@liggitt liggitt Aug 4, 2020 •
edited

Choose a reason for hiding this comment

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

these are pretty fragile to apiserver configuration... for example, rbac might not be present in some clusters. also, other configurations (like KMS-based encryption add checks). exact matches based on presubmit API server config aren't likely to work well in general. maybe partition these to required and optional things, and tolerate absence of optional items?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

Please let me know what checks should be removed from required ones.

liggitt
liggitt reviewed Aug 4, 2020
View reviewed changes
test/e2e/apimachinery/health_handlers.go Outdated
var _ = SIGDescribe("health handlers", func() {
f := framework.NewDefaultFramework("health")

ginkgo.It("/healthz should contain all checks", func() {
Copy link
Member

Choose a reason for hiding this comment

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

three distinct tests seems like overkill... maybe one with three calls it verifies? remember there is per test overhead like namespace creation, etc

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

liggitt
liggitt reviewed Aug 4, 2020
View reviewed changes
test/e2e/apimachinery/health_handlers.go Outdated
"[+]poststarthook/start-apiextensions-controllers ok",
"[+]poststarthook/crd-informer-synced ok",
"[+]poststarthook/bootstrap-controller ok",
"[+]poststarthook/rbac/bootstrap-roles ok",
Copy link
Member

@liggitt liggitt Aug 4, 2020 •
edited

Choose a reason for hiding this comment

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

rbac is optional, depending on whether the RBAC authorizer is enabled

liggitt
liggitt reviewed Aug 4, 2020
View reviewed changes
test/e2e/apimachinery/health_handlers.go Outdated
"[+]poststarthook/rbac/bootstrap-roles ok",
"[+]poststarthook/scheduling/bootstrap-system-priority-classes ok",
"[+]poststarthook/start-cluster-authentication-info-controller ok",
"[+]poststarthook/aggregator-reload-proxy-client-cert ok",
Copy link
Member

Choose a reason for hiding this comment

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

this is optional based on API server flags

liggitt
liggitt reviewed Aug 4, 2020
View reviewed changes
test/e2e/apimachinery/health_handlers.go Outdated
Comment on lines 117 to 113
for _, check := range requiredChecks {
if !checks.Has(check) {
return fmt.Errorf("required %s check: %s not present: %s", path, check, string(body))
}
}
Copy link
Member

@liggitt liggitt Aug 4, 2020 •
edited

Choose a reason for hiding this comment

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

nit: testing if sets.NewString(requiredChecks...).Difference(checks) is non-empty and logging all missing required checks avoids iterating on CI adding a single string each time

@wojtek-t
Copy link
Member Author

wojtek-t commented Aug 4, 2020

@liggitt - thanks! PTAL

@wojtek-t
Copy link
Member Author

wojtek-t commented Aug 4, 2020

/retest
test-infra issues

@liggitt
Copy link
Member

liggitt commented Aug 4, 2020

/lgtm
/approve
/milestone v1.19

@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Aug 4, 2020
@liggitt
Copy link
Member

liggitt commented Aug 4, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 4, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wojtek-t

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

@wojtek-t
Copy link
Member Author

wojtek-t commented Aug 4, 2020

/retest

@apelisse
Copy link
Member

apelisse commented Aug 4, 2020

/assign @logicalhan

@liggitt
Copy link
Member

liggitt commented Aug 4, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit 9dec51e into kubernetes:master Aug 5, 2020
@liggitt liggitt mentioned this pull request May 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@logicalhan logicalhan Awaiting requested review from logicalhan

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@logicalhan logicalhan

@liggitt liggitt

@mm4tt mm4tt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wojtek-t @mm4tt @liggitt @k8s-ci-robot @apelisse @logicalhan