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

NodePort health checks should be disabled when kube-proxy is installed #16477

Merged
merged 2 commits into from Jun 10, 2021
Merged

NodePort health checks should be disabled when kube-proxy is installed #16477

merged 2 commits into from Jun 10, 2021

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Jun 8, 2021

See commit descriptions.

@pchaigno
healthserver: Clarify when NodePort health checks should be disabled
d107ffb 
The first part of the message says the error happens when running with
kube-proxy, but the second part only recommends to disable health checks
if running with KPR=partial. My own tests also show this error can
happen when KPR=strict and kube-proxy is installed.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno
test/helpers: Disable NodePort health checks if using kube-proxy
be6b21e 
In the 4.19 CI job, we run with KPR=strict for most tests even though
kube-proxy is installed. As a result, we get an error message because
the agent fails to listen on the health checks port. In that setup, we
should instead disable health checks.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno added area/CI Continuous Integration testing issue or flake area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. release-note/ci This PR makes changes to the CI. area/kube-proxy-free area/kube-proxy Issues related to kube-proxy (not the kube-proxy-free mode). labels Jun 8, 2021
@pchaigno pchaigno requested a review from brb June 8, 2021 17:35
@pchaigno pchaigno requested a review from a team as a code owner June 8, 2021 17:35
@pchaigno pchaigno requested review from a team and nbusseneau June 8, 2021 17:35
@pchaigno
Copy link
Member Author

pchaigno commented Jun 8, 2021

test-me-please

brb
brb approved these changes Jun 9, 2021
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

Asking @gandro to review it as well, as he might have more context for this feature.

@brb brb requested a review from gandro June 9, 2021 08:02
gandro
gandro approved these changes Jun 9, 2021
View reviewed changes
Copy link
Member

@gandro gandro left a comment •
edited

Choose a reason for hiding this comment

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

Looks good! At first I was a bit confused, since the flag shouldn't do anything if KPR is turned off, but looks like in that test we have both KPR and KP enabled.

Edit: I guess I could have also read the commit messages. Thanks!

nbusseneau
nbusseneau approved these changes Jun 9, 2021
View reviewed changes
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the context in commit message.

@pchaigno
Copy link
Member Author

pchaigno commented Jun 9, 2021

ConformanceAKS is failing with known flake #16292, k8s-1.19-kernel-5.4 with #15455. Reviews are in. Marking as ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 9, 2021
@aditighag aditighag merged commit 90a1b60 into cilium:master Jun 10, 2021
@pchaigno pchaigno deleted the disable-health-checks-kube-proxy branch June 10, 2021 20:13
pchaigno added a commit to pchaigno/cilium that referenced this pull request Jun 22, 2021
@pchaigno
test: Update list of allowed error logs
939691b 
cilium#16477 was merged and a new error,
cilium#16402 (comment) was
discovered since the PR disallowing level=error in CI was merged.

Signed-off-by: Paul Chaignon <paul@cilium.io>
christarazi pushed a commit that referenced this pull request Jun 23, 2021
@pchaigno @christarazi
test: Update list of allowed error logs
e68d43f 
#16477 was merged and a new error,
#16402 (comment) was
discovered since the PR disallowing level=error in CI was merged.

Signed-off-by: Paul Chaignon <paul@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@brb brb brb approved these changes

@nbusseneau nbusseneau nbusseneau approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake area/kube-proxy Issues related to kube-proxy (not the kube-proxy-free mode). area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pchaigno @gandro @brb @nbusseneau @aditighag