Skip to content

Add an option to cilium-agent for disabling 'HealthCheckNodePort'#11236

Merged
aanm merged 1 commit intocilium:masterfrom
soumynathan:add-option-disable-healthchecknodeport
Jul 3, 2020
Merged

Add an option to cilium-agent for disabling 'HealthCheckNodePort'#11236
aanm merged 1 commit intocilium:masterfrom
soumynathan:add-option-disable-healthchecknodeport

Conversation

@soumynathan
Copy link
Copy Markdown

@soumynathan soumynathan commented Apr 29, 2020

This PR adds in an option to the cilium-agent for disabling
'HealthCheckNodePort' based on the KubeProxyReplacement configuration.

Related: #11168
Signed-off-by: Swaminathan Vasudevan svasudevan@suse.com

@soumynathan soumynathan requested a review from a team April 29, 2020 19:47
@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Apr 29, 2020

Please set the appropriate release note label.

@soumynathan
Copy link
Copy Markdown
Author

soumynathan commented Apr 29, 2020

This is work in progress.

@gandro gandro added the wip label Apr 29, 2020
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 29, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.04%) to 36.938% when pulling a596710 on soumynathan:add-option-disable-healthchecknodeport into d8e0d26 on cilium:master.

gandro
gandro reviewed Apr 30, 2020
View reviewed changes
Comment thread daemon/cmd/daemon_main.go Outdated
gandro
gandro reviewed Apr 30, 2020
View reviewed changes
Comment thread pkg/option/config.go Outdated
@aanm aanm marked this pull request as draft April 30, 2020 18:12
@aanm
Copy link
Copy Markdown
Member

aanm commented Apr 30, 2020

I've marked the PR as draft since it has a WIP label

@soumynathan soumynathan force-pushed the add-option-disable-healthchecknodeport branch from 7d3f074 to a801727 Compare April 30, 2020 18:42
@soumynathan
Copy link
Copy Markdown
Author

soumynathan commented Apr 30, 2020

I've marked the PR as draft since it has a WIP label

You can remove the 'WIP' from this PR. It is good to review.

@tklauser tklauser added release-note/misc This PR makes changes that have no direct user impact. and removed wip labels Apr 30, 2020
@tklauser tklauser marked this pull request as ready for review April 30, 2020 22:14
@tklauser tklauser requested a review from a team April 30, 2020 22:14
@tklauser
Copy link
Copy Markdown
Member

tklauser commented Apr 30, 2020

Marked as "ready for review" as per #11236 (comment)

@tklauser
Copy link
Copy Markdown
Member

tklauser commented Apr 30, 2020

test-me-please

@tklauser tklauser requested a review from brb April 30, 2020 22:22
@aanm
Copy link
Copy Markdown
Member

aanm commented May 1, 2020

@soumynathan the CI failed with a real failure.

00:35:10      runtime: Successfully built afd473816320
00:35:10      runtime: Successfully tagged cilium/docs-builder:latest
00:35:10      runtime: rm -rf cmdref/cilium*.md
00:35:10      runtime: docker container run --rm --workdir /src/Documentation --volume /home/vagrant/go/src/github.com/cilium/cilium/Documentation/..:/src --user "1000:1000" cilium/docs-builder ./update-cmdref.sh
00:35:11      runtime: docker container run --rm --workdir /src/Documentation --volume /home/vagrant/go/src/github.com/cilium/cilium/Documentation/..:/src --user "1000:1000" cilium/docs-builder ./check-cmdref.sh
00:35:12      runtime: diff --git a/Documentation/cmdref/cilium-agent.md b/Documentation/cmdref/cilium-agent.md
00:35:12      runtime: index 778bb0ed5..86169a3e9 100644
00:35:12      runtime: --- a/Documentation/cmdref/cilium-agent.md
00:35:12      runtime: +++ b/Documentation/cmdref/cilium-agent.md
00:35:12      runtime: @@ -58,6 +58,7 @@ cilium-agent [flags]
00:35:12      runtime:        --enable-endpoint-health-checking               Enable connectivity health checking between virtual endpoints (default true)
00:35:12      runtime:        --enable-endpoint-routes                        Use per endpoint routes instead of routing via cilium_host
00:35:12      runtime:        --enable-external-ips                           Enable k8s service externalIPs feature (requires enabling enable-node-port) (default true)
00:35:12      runtime: +      --enable-health-check-nodeport                  flag to allow users to opt-out of the service health server running inside cilium-agent when running NodePort BPF (default true)
00:35:12      runtime:        --enable-health-checking                        Enable connectivity health checking (default true)
00:35:12      runtime:        --enable-host-port                              Enable k8s hostPort mapping feature (requires enabling enable-node-port) (default true)
00:35:12      runtime:        --enable-host-reachable-services                Enable reachability of services for host applications (beta)
00:35:12      runtime: HINT: to fix this, run 'make -C Documentation update-cmdref ; git commit Documentation/cmdref --message "docs: Update cmdref"'
00:35:12      runtime: Makefile:30: recipe for target 'check' failed
00:35:12      runtime: make[1]: Leaving directory '/home/vagrant/go/src/github.com/cilium/cilium/Documentation'
00:35:12      runtime: make[1]: *** [check] Error 1
00:35:12      runtime: Makefile:502: recipe for target 'postcheck' failed
00:35:12      runtime: make: *** [postcheck] Error 2
00:35:12  The SSH command responded with a non-zero exit status. Vagrant
00:35:12  assumes that this means the command failed. The output for this command
00:35:12  should be in the log above. Please read the output to determine what
00:35:12  went wrong.

aanm
aanm requested changes May 1, 2020
View reviewed changes
Comment thread daemon/cmd/daemon_main.go Outdated
Comment thread pkg/service/service.go Outdated
@soumynathan soumynathan force-pushed the add-option-disable-healthchecknodeport branch from a801727 to 769a766 Compare May 1, 2020 22:45
@soumynathan soumynathan requested a review from a team as a code owner May 1, 2020 22:45
@soumynathan soumynathan force-pushed the add-option-disable-healthchecknodeport branch from 769a766 to bfbf15b Compare May 1, 2020 22:46
@qmonnet
Copy link
Copy Markdown
Member

qmonnet commented May 2, 2020

test-me-please

gandro
gandro reviewed May 2, 2020
View reviewed changes
Comment thread pkg/service/healthserver/healthserver.go Outdated
@soumynathan soumynathan force-pushed the add-option-disable-healthchecknodeport branch from bfbf15b to a72e0b0 Compare May 4, 2020 06:21
@aanm aanm removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 12, 2020
@gandro
Copy link
Copy Markdown
Member

gandro commented Jun 15, 2020 •
edited
Loading

retest-4.19

Previous failure was a known flake, K8sServicesTest_Checks_service_across_nodes_Supports_IPv4_fragments

https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Kernel/2069/

@aanm aanm added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 17, 2020
@aanm
Copy link
Copy Markdown
Member

aanm commented Jun 17, 2020

@soumynathan needs rebase

@soumynathan soumynathan force-pushed the add-option-disable-healthchecknodeport branch from 5d0bfec to f493d1c Compare June 23, 2020 21:30
@aanm
Copy link
Copy Markdown
Member

aanm commented Jun 24, 2020

test-me-please

@aanm aanm removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 24, 2020
@gandro
Copy link
Copy Markdown
Member

gandro commented Jun 25, 2020

I assume the pending checks are expected due to the fact that this PR was opened with different github actions setup?

I think this PR is only missing reviews from @cilium/cli & @cilium/helm

errordeveloper
errordeveloper reviewed Jun 30, 2020
View reviewed changes
Comment thread pkg/service/service.go Outdated
errordeveloper
errordeveloper reviewed Jun 30, 2020
View reviewed changes
Comment thread pkg/service/service.go Outdated
errordeveloper
errordeveloper reviewed Jun 30, 2020
View reviewed changes
Comment thread pkg/service/service.go Outdated
errordeveloper
errordeveloper reviewed Jun 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

Looks good overall, just a few nits!

agent: Add an option to cilium-agent for disabling 'HealthCheckNodePort'
a596710 
This PR adds in an option 'enable-health-check-nodeport' to the
cilium-agent for disabling 'HealthCheckNodePort' based on the
KubeProxyReplacement configuration.
It also adds in an opton 'enableHealthCheckNodePort' to the helm
charts and documents the new config options with impacts when
kubeProxyReplacement is set to 'partial'.

Fixes: cilium#11168
Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>
@soumynathan soumynathan force-pushed the add-option-disable-healthchecknodeport branch from f493d1c to a596710 Compare July 1, 2020 18:26
@soumynathan
Copy link
Copy Markdown
Author

soumynathan commented Jul 1, 2020

Looks good overall, just a few nits!

Addressed your review comments.

@aanm
Copy link
Copy Markdown
Member

aanm commented Jul 2, 2020

test-me-please

@gandro gandro requested a review from errordeveloper July 2, 2020 12:00
@maintainer-s-little-helper maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 3, 2020
@aanm aanm merged commit edff374 into cilium:master Jul 3, 2020
This was referenced Jul 7, 2020
Merged
Merged
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

@aanm aanm aanm approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

+1 more reviewer

@errordeveloper errordeveloper errordeveloper approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.