os exit when option is true #76732

LexCC · 2019-04-17T18:43:07Z

What type of PR is this?

/kind bug

What this PR does / why we need it:
When CleanupAndExit is true , kube-proxy doesn't exit
Related:

Lines 322 to 334 in 6c55e2d

    
           go func() { 
        
           	err := o.proxyServer.Run() 
        
           	o.errCh <- err 
        
           }() 
        
           for { 
        
           	select { 
        
           	case err := <-o.errCh: 
        
           		if err != nil { 
        
           			return err 
        
           		} 
        
           	} 
        
           }

kubernetes/cmd/kube-proxy/app/server.go

Lines 553 to 561 in 6c55e2d

    
           if s.CleanupAndExit { 
        
           	encounteredError := userspace.CleanupLeftovers(s.IptInterface) 
        
           	encounteredError = iptables.CleanupLeftovers(s.IptInterface) || encounteredError 
        
           	encounteredError = ipvs.CleanupLeftovers(s.IpvsInterface, s.IptInterface, s.IpsetInterface, s.CleanupIPVS) || encounteredError 
        
           	if encounteredError { 
        
           		return errors.New("encountered an error while tearing down rules") 
        
           	} 
        
           	return nil 
        
           }

kubernetes/cmd/kube-proxy/app/server.go

Line 466 in 6c55e2d

klog.Fatal(opts.Run())

Which issue(s) this PR fixes:

Fixes #76727

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kube-proxy: os exit when CleanupAndExit is set to true

k8s-ci-robot · 2019-04-17T18:43:15Z

Hi @jiejhih. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

neolit123

thanks for the PR.
this code is from 1.1 (2015).

/kind bug
/priority important-longterm
/ok-to-test

@kubernetes/sig-network-pr-reviews

vllry · 2019-04-19T06:53:11Z

/assign @andrewsykim

Handling exits like this (raising an error as a non-error exit signal) feels like a code smell to me, but this does appear to solve the issue.

dixudx

/lgtm

LexCC · 2019-04-19T08:48:08Z

@vllry

Handling exits like this (raising an error as a non-error exit signal) feels like a code smell to me, but this does appear to solve the issue.

Yeah it solved the issue but it can be better.
I think it can be something like this.
errCh chan error change into errCh chan errChannel

type errChannel struct {
      Message string
      Err           error
 }

But the change will affect some places , I will create another PR to refactor this.

andrewsykim · 2019-04-19T13:16:11Z

cmd/kube-proxy/app/server.go

@@ -557,7 +557,7 @@ func (s *ProxyServer) Run() error {
 		if encounteredError {
 			return errors.New("encountered an error while tearing down rules")
 		}
-		return nil
+		return errors.New("cleaup and exit")


Agree with @vllry, this seems like a bit of a code smell, we're effectively saying that nil errors are invalid. I personally think we should fix/refactor properly in this PR. I'm wondering if it would be cleaner if we pulled the clean up & exit logic out of Run() and ran the clean up check prior, that would reduce some of the complexity with passing errors through channels.

That is a good way!
It can run clean up & exit logic before run proxy server if success then os exit else return the error message.
I will refactor in this PR.

kubernetes/cmd/kube-proxy/app/server.go

Lines 321 to 325 in 3b12266

// run the proxy in goroutine

go func() {

err := o.proxyServer.Run()

o.errCh <- err

}()

Yeah that seems like a better approach to me, thoughts @vllry?

vllry · 2019-04-19T15:43:12Z

I was thinking about test coverage for something like this - e2e would be messy, and unsure how to properly deal with iptables/IPVS scope in a unit test.

LexCC · 2019-04-23T13:51:04Z

/retest

LexCC · 2019-04-23T15:38:56Z

@andrewsykim Updated. Thanks

thockin · 2019-04-23T22:21:28Z

cmd/kube-proxy/app/server.go

@@ -307,6 +308,11 @@ func (o *Options) Run() error {
 	if err != nil {
 		return err
 	}
+
+	if o.CleanupAndExit {
+		return proxyServer.CleanUpAndExit()


Can we name the method the same as the flag?

thockin · 2019-04-23T22:23:36Z

cmd/kube-proxy/app/server.go

 func (s *ProxyServer) Run() error {
 	// To help debugging, immediately log version
 	klog.Infof("Version: %+v", version.Get())
-	// remove iptables rules and exit
-	if s.CleanupAndExit {


can we remove the ProxyServer.CleanupAndExit field at the same time?

LexCC · 2019-04-24T11:27:29Z

@thockin Updated. PTAL.

andrewsykim · 2019-04-24T12:44:49Z

@jiejhih I think @thockin mentioned to also remove the field CleanupAndExit field from the ProxyServer struct. See here (ideally in a separate commit please)

LexCC · 2019-04-24T12:52:01Z

@andrewsykim Yeah, I have removed that field in the newest commit. Thanks
https://github.com/kubernetes/kubernetes/pull/76732/files#diff-cf93bbed37202b7d58b5841f07ddd89fL500

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2019-04-26T20:22:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JieJhih, 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/OWNERS~~ [thockin]

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

LexCC · 2019-04-27T05:44:51Z

/retest

LexCC · 2019-04-27T06:59:44Z

/test pull-kubernetes-integration

andrewsykim · 2019-04-27T13:56:11Z

/lgtm

Thanks for iterating @jiejhih!

LexCC · 2019-04-27T16:47:49Z

/test pull-kubernetes-node-e2e

tedyu · 2019-04-27T20:54:54Z

cmd/kube-proxy/app/server.go

+
+// CleanupAndExit remove iptables rules and exit if success return nil
+func (s *ProxyServer) CleanupAndExit() error {
+	encounteredError := userspace.CleanupLeftovers(s.IptInterface)


I wonder if CleanupLeftovers should return the (aggregate) error instead of returning just a boolean.
This would be useful for caller for analysis.

I agree that this is a bit ugly but I don't think it'll be easy to fix. A lot of errors from CleanupLeftovers don't warrant an early return. For what it's worth, most of the errors are still logged via klog.Error though.

…upstream-release-1.14 Automated cherry pick of #76732: os exit when option is true

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 17, 2019

LexCC force-pushed the proxy/server branch from 9766bd5 to af461da Compare April 17, 2019 18:44

k8s-ci-robot requested review from dchen1107 and thockin April 17, 2019 18:44

neolit123 reviewed Apr 17, 2019

View reviewed changes

vllry mentioned this pull request Apr 19, 2019

exit kube-proxy when configuration file changes #59176

Merged

k8s-ci-robot assigned andrewsykim and dixudx Apr 19, 2019

dixudx approved these changes Apr 19, 2019

View reviewed changes

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

andrewsykim reviewed Apr 19, 2019

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2019

LexCC force-pushed the proxy/server branch 2 times, most recently from 2debcb5 to 84e9d12 Compare April 23, 2019 12:58

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 23, 2019

thockin reviewed Apr 23, 2019

View reviewed changes

os exit when option is true

48f431a

LexCC force-pushed the proxy/server branch from 84e9d12 to 48f431a Compare April 24, 2019 08:32

thockin approved these changes Apr 26, 2019

View reviewed changes

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

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

Merge branch 'master' into proxy/server

94731c5

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2019

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

k8s-ci-robot merged commit a0b8d1c into kubernetes:master Apr 27, 2019

tedyu reviewed Apr 27, 2019

View reviewed changes

This was referenced Apr 30, 2019

Kube-proxy goes in non-operational mode after cleanup without reporting when cleanup job is done #77264

Closed

Automated cherry pick of #76732: os exit when option is true #77276

Merged

k8s-ci-robot added a commit that referenced this pull request May 2, 2019

Merge pull request #77276 from vllry/automated-cherry-pick-of-#76732-…

b083bd4

…upstream-release-1.14 Automated cherry pick of #76732: os exit when option is true

LexCC mentioned this pull request May 19, 2019

REQUEST: New membership for JieJhih kubernetes/org#833

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os exit when option is true #76732

os exit when option is true #76732

LexCC commented Apr 17, 2019 •

edited

Loading

k8s-ci-robot commented Apr 17, 2019

neolit123 left a comment

vllry commented Apr 19, 2019

dixudx left a comment

LexCC commented Apr 19, 2019

andrewsykim Apr 19, 2019

LexCC Apr 19, 2019

andrewsykim Apr 19, 2019

vllry Apr 19, 2019

vllry commented Apr 19, 2019

LexCC commented Apr 23, 2019

LexCC commented Apr 23, 2019

thockin Apr 23, 2019

thockin Apr 23, 2019

LexCC commented Apr 24, 2019

andrewsykim commented Apr 24, 2019 •

edited

Loading

LexCC commented Apr 24, 2019

thockin left a comment

k8s-ci-robot commented Apr 26, 2019

LexCC commented Apr 27, 2019

LexCC commented Apr 27, 2019

andrewsykim commented Apr 27, 2019

LexCC commented Apr 27, 2019

tedyu Apr 27, 2019

andrewsykim Apr 29, 2019

	go func() {
	err := o.proxyServer.Run()
	o.errCh <- err
	}()

	for {
	select {
	case err := <-o.errCh:
	if err != nil {
	return err
	}
	}
	}

	if s.CleanupAndExit {
	encounteredError := userspace.CleanupLeftovers(s.IptInterface)
	encounteredError = iptables.CleanupLeftovers(s.IptInterface) \|\| encounteredError
	encounteredError = ipvs.CleanupLeftovers(s.IpvsInterface, s.IptInterface, s.IpsetInterface, s.CleanupIPVS) \|\| encounteredError
	if encounteredError {
	return errors.New("encountered an error while tearing down rules")
	}
	return nil
	}

	// run the proxy in goroutine
	go func() {
	err := o.proxyServer.Run()
	o.errCh <- err
	}()

os exit when option is true #76732

os exit when option is true #76732

Conversation

LexCC commented Apr 17, 2019 • edited Loading

k8s-ci-robot commented Apr 17, 2019

neolit123 left a comment

Choose a reason for hiding this comment

vllry commented Apr 19, 2019

dixudx left a comment

Choose a reason for hiding this comment

LexCC commented Apr 19, 2019

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

vllry commented Apr 19, 2019

LexCC commented Apr 23, 2019

LexCC commented Apr 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LexCC commented Apr 24, 2019

andrewsykim commented Apr 24, 2019 • edited Loading

LexCC commented Apr 24, 2019

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 26, 2019

LexCC commented Apr 27, 2019

LexCC commented Apr 27, 2019

andrewsykim commented Apr 27, 2019

LexCC commented Apr 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LexCC commented Apr 17, 2019 •

edited

Loading

andrewsykim commented Apr 24, 2019 •

edited

Loading