kube-proxy: treat failure to bind to a port as fatal #89350

SataQiu · 2020-03-23T05:35:39Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
kube-proxy: treat failure to bind to a port as fatal

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kube-proxy: add `--bind-address-hard-fail` flag to treat failure to bind to a port as fatal

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

fejta-bot · 2020-03-23T07:23:02Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

neolit123 · 2020-03-23T12:45:55Z

/remove-area kubeadm

aojea · 2020-03-23T14:55:47Z

/retest
it looks good to me, but I'm not aware of the possible ramifications so I defer to the people more familiar with it.
/assign @danwinship @thockin

thockin · 2020-03-23T16:32:48Z

cmd/kube-proxy/app/server.go

@@ -168,6 +168,7 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) {
 	fs.Var(utilflag.IPVar{Val: &o.config.BindAddress}, "bind-address", "The IP address for the proxy server to serve on (set to '0.0.0.0' for all IPv4 interfaces and '::' for all IPv6 interfaces)")
 	fs.Var(utilflag.IPPortVar{Val: &o.config.HealthzBindAddress}, "healthz-bind-address", "The IP address with port for the health check server to serve on (set to '0.0.0.0:10256' for all IPv4 interfaces and '[::]:10256' for all IPv6 interfaces). Set empty to disable.")
 	fs.Var(utilflag.IPPortVar{Val: &o.config.MetricsBindAddress}, "metrics-bind-address", "The IP address with port for the metrics server to serve on (set to '0.0.0.0:10249' for all IPv4 interfaces and '[::]:10249' for all IPv6 interfaces). Set empty to disable.")
+	fs.BoolVar(&o.config.MetricsBindHardFail, "metrics-bind-hard-fail", o.config.MetricsBindHardFail, "If true kube-proxy will treat metrics bind failure as fatal and exit")


Should we cover both metrics and healthz? E.g. --bind-address-hard-fail?

Thank you very much @thockin ! I'll try to refactor my code.

cmd/kube-proxy/app/server.go

thockin · 2020-03-23T16:46:48Z

cmd/kube-proxy/app/server.go

@@ -595,6 +597,7 @@ func (s *ProxyServer) Run() error {
 		s.Broadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: s.EventClient.Events("")})
 	}

+	errCh := make(chan error)


comments? This seems out of place here, but if we handle both healthz and metrics, it makes more sense.

SataQiu · 2020-03-25T15:55:11Z

/priority important-soon

thockin · 2020-03-27T05:17:41Z

cmd/kube-proxy/app/server.go

 	// Start up a healthz server if requested
 	if s.HealthzServer != nil {
-		s.HealthzServer.Run()
+		s.HealthzServer.Run(errCh)


As a "pre-factoring", what would yo think about (in a commit of its own) making Run() be synchronous (take out the go inside it) and making callers call it like:

go func() { err := s.HealthzServer.Run() if err != nil { errCh <- err } }

Then when you add the hard fail, that just becomes a flag to Run(), instead of a constructor flag.

I can do this prefactoring if you prefer - this code is itching for some cleanups, but I wonder if I am clear in my goals?

As a starting example: https://github.com/kubernetes/kubernetes/compare/master...thockin:proxy-cleanup?expand=1

I'd like to see the same for metrics (moved to a function as much as possible).

Then error handling becomes passing errCh to those (or nil for not reporting errors)

Thanks a lot @thockin !
Please feel free to do the prefactoring.
If you don't have the time, I can also pick it up (thank you for your starting example).

Rebased on #89654

#89654 merged.

andrewsykim

Given E2Es passed (with known test failures) and we've validated some of the changes in moby/ipvs (thanks @rikatz) I think this is good to merge. Prefer merging early in the cycle to leave more time for extra validation/testing.

Sorry commented in the wrong PR (was for #89116)

SataQiu · 2020-04-01T02:22:58Z

/test pull-kubernetes-node-e2e

thockin

Almost there!

thockin · 2020-04-01T23:32:16Z

cmd/kube-proxy/app/server.go

-			// For historical reasons we do not abort on errors here.  We may
-			// change that in the future.
-			klog.Errorf("healthz server failed: %v", err)
+			klog.Errorf("starting healthz server failed: %v", err)


It's not an error starting, it's an error. If the HTTP server starts and later fails, this path is hit. The original error string is correct

thockin · 2020-04-01T23:32:40Z

cmd/kube-proxy/app/server.go

-			// change that in the future.
-			klog.Errorf("healthz server failed: %v", err)
+			klog.Errorf("starting healthz server failed: %v", err)
+			if hardFail {


Why not just test if errCh is nil or not - one less flag

thockin · 2020-04-01T23:33:12Z

cmd/kube-proxy/app/server.go

-			// For historical reasons we do not abort on errors here.  We may
-			// change that in the future.
-			utilruntime.HandleError(fmt.Errorf("starting metrics server failed: %v", err))
+			err = fmt.Errorf("starting metrics server failed: %v", err)


same comments as above

…ind to a port as fatal Signed-off-by: SataQiu <1527062125@qq.com>

SataQiu · 2020-04-02T09:50:25Z

/test pull-kubernetes-integration
/test pull-kubernetes-e2e-kind

SataQiu · 2020-04-05T01:54:25Z

kindly ping @thockin
need any other modifications？

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2020-04-06T21:51:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SataQiu, thockin

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:

~~cmd/kube-proxy/OWNERS~~ [thockin]
~~cmd/kubeadm/OWNERS~~ [thockin]
~~pkg/proxy/apis/config/OWNERS~~ [thockin]
~~staging/src/k8s.io/kube-proxy/config/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…d to a port as fatal kubernetes/kubernetes#89350 Signed-off-by: Maxim Filatov <pipopolam@gmail.com>

k8s-ci-robot requested review from andrewsykim and aojea March 23, 2020 05:36

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 23, 2020

SataQiu force-pushed the fix-kube-proxy-20200323 branch from 339a9c6 to 9e3b531 Compare March 23, 2020 06:57

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Mar 23, 2020

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Mar 23, 2020

SataQiu force-pushed the fix-kube-proxy-20200323 branch from 28e6fb5 to 626e9e3 Compare March 23, 2020 07:42

k8s-ci-robot removed the area/kubeadm label Mar 23, 2020

k8s-ci-robot assigned danwinship and thockin Mar 23, 2020

thockin reviewed Mar 23, 2020

View reviewed changes

SataQiu changed the title ~~kube-proxy: treat metrics bind failure as fatal~~ kube-proxy: treat failure to bind to a port as fatal Mar 24, 2020

SataQiu force-pushed the fix-kube-proxy-20200323 branch from 626e9e3 to 459dd73 Compare March 24, 2020 04:10

k8s-ci-robot added the area/kubeadm label Mar 24, 2020

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 25, 2020

SataQiu requested a review from thockin March 26, 2020 02:33

thockin reviewed Mar 27, 2020

View reviewed changes

andrewsykim reviewed Mar 30, 2020

View reviewed changes

k8s-ci-robot assigned andrewsykim Mar 30, 2020

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 30, 2020

SataQiu force-pushed the fix-kube-proxy-20200323 branch from 459dd73 to a1751c3 Compare March 31, 2020 14:29

thockin reviewed Apr 1, 2020

View reviewed changes

kube-proxy: add '--bind-address-hard-fail' flag to treat failure to b…

871b90b

…ind to a port as fatal Signed-off-by: SataQiu <1527062125@qq.com>

SataQiu force-pushed the fix-kube-proxy-20200323 branch from a1751c3 to 871b90b Compare April 2, 2020 05:13

SataQiu requested a review from thockin April 5, 2020 01:54

thockin reviewed Apr 6, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2020

k8s-ci-robot merged commit cabf5d1 into kubernetes:master Apr 7, 2020

k8s-ci-robot added this to the v1.19 milestone Apr 7, 2020

SataQiu mentioned this pull request Apr 7, 2020

Self nominate SataQiu as a sig-network reviewer #89924

Closed

Bregor added a commit to evilmartians/chef-kubernetes that referenced this pull request Sep 10, 2020

Kube-proxy: add --bind-address-hard-fail flag to treat failure to bin…

0cffe74

…d to a port as fatal kubernetes/kubernetes#89350 Signed-off-by: Maxim Filatov <pipopolam@gmail.com>

Bregor added a commit to evilmartians/chef-kubernetes that referenced this pull request Sep 14, 2020

Kube-proxy: add --bind-address-hard-fail flag to treat failure to bin…

68e62e8

…d to a port as fatal kubernetes/kubernetes#89350 Signed-off-by: Maxim Filatov <pipopolam@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kube-proxy: treat failure to bind to a port as fatal #89350

kube-proxy: treat failure to bind to a port as fatal #89350

SataQiu commented Mar 23, 2020 •

edited

Loading

fejta-bot commented Mar 23, 2020

neolit123 commented Mar 23, 2020

aojea commented Mar 23, 2020

thockin Mar 23, 2020

SataQiu Mar 24, 2020

thockin Mar 23, 2020

SataQiu commented Mar 25, 2020

thockin Mar 27, 2020

thockin Mar 27, 2020

SataQiu Mar 27, 2020

thockin Mar 30, 2020

SataQiu Mar 31, 2020 •

edited

Loading

thockin Apr 1, 2020

andrewsykim left a comment •

edited

Loading

SataQiu commented Apr 1, 2020

thockin left a comment

thockin Apr 1, 2020

thockin Apr 1, 2020

SataQiu Apr 2, 2020

thockin Apr 1, 2020

SataQiu commented Apr 2, 2020

SataQiu commented Apr 5, 2020

thockin left a comment

k8s-ci-robot commented Apr 6, 2020

kube-proxy: treat failure to bind to a port as fatal #89350

kube-proxy: treat failure to bind to a port as fatal #89350

Conversation

SataQiu commented Mar 23, 2020 • edited Loading

fejta-bot commented Mar 23, 2020

neolit123 commented Mar 23, 2020

aojea commented Mar 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SataQiu commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SataQiu Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim left a comment • edited Loading

Choose a reason for hiding this comment

SataQiu commented Apr 1, 2020

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SataQiu commented Apr 2, 2020

SataQiu commented Apr 5, 2020

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 6, 2020

SataQiu commented Mar 23, 2020 •

edited

Loading

SataQiu Mar 31, 2020 •

edited

Loading

andrewsykim left a comment •

edited

Loading