Catch termination signal #62

martinkennelly · 2021-01-19T17:01:50Z

In order for termination signal to be captured, two goroutines
are created which handle TLS keypair reloading and HTTP server
instantiation.

Signed-off-by: Kennelly, Martin [email protected]

Fixes: #48

martinkennelly · 2021-01-25T11:52:27Z

I want to update this patch to make it easier to test.
I will add test cases.

In order to catch termination signals, changes to how we handle cert/key watcher & HTTP server are needed. Refactor code to use goroutines for TLS cert/key watcher and HTTP server. Add Channel to safely manage signals from goroutines. Add interfaces to aid testing with mockery. Add tests to cover new code changes. Signed-off-by: Kennelly, Martin <[email protected]>

martinkennelly · 2021-02-19T12:03:29Z

A lot of changes here I know. I improved the test coverage to 33%. I am hoping to add e2e tests in a separate PR using kind and this will help validate this patch.

martinkennelly · 2021-04-10T12:04:02Z

@zshi-redhat Do you see value with this patch? I was going to push for it once E2E test cases got to a sufficient level where we can do regression testing of the features. @MichalGuzieniuk is working on adding these right now.

I want this patch for the following reasons but it will need to be reworked to accommodate your user defined injections work but that is no biggie once we have tests for it.

Reasons:

NRI will catch term signals - currently it doesn't handle them.
NRI will end if HTTP server fails - currently nothing happens if it fails.
NRI will end if keypair reloader fails - current we just keep retrying.
Improved unit test coverage from ~20% to 33%
Improved separation of code instead of it all bundled into webhook.go. We need to stop that going forward imo.

I propose adding user injections golang routine as well and if it fails, NRI should end, and be re-provisioned.

Anyway, Its up to you if you see any value with this patch long term. Let me know when you have time. No rush as I think its good to fix the existing problem with user injection first, and increase the e2e test cases (thanks to @MichalGuzieniuk ) to ensure we have regression testing when this major change occurs.

zshi-redhat · 2021-04-12T10:41:26Z

@zshi-redhat Do you see value with this patch? I was going to push for it once E2E test cases got to a sufficient level where we can do regression testing of the features. @MichalGuzieniuk is working on adding these right now.

+1, yes, absolutely.

I want this patch for the following reasons but it will need to be reworked to accommodate your user defined injections work but that is no biggie once we have tests for it.

Reasons:
* NRI will catch term signals - currently it doesn't handle them.

* NRI will end if HTTP server fails - currently nothing happens if it fails.

* NRI will end if keypair reloader fails - current we just keep retrying.

* Improved unit test coverage from ~20% to 33%

* Improved separation of code instead of it all bundled into webhook.go. We need to stop that going forward imo.

I think these are positive moves.
I had the same idea of separating giant webhook.go into multiple files, didn't get chance to implement it :-)

I propose adding user injections golang routine as well and if it fails, NRI should end, and be re-provisioned.

User-defined-injections is an optional feature for now, We probably don't want to end the NRI process for the absense of configMap or so.

Anyway, Its up to you if you see any value with this patch long term. Let me know when you have time. No rush as I think its good to fix the existing problem with user injection first, and increase the e2e test cases (thanks to @MichalGuzieniuk ) to ensure we have regression testing when this major change occurs.

Thanks for working on these improvements!

pperiyasamy · 2021-06-16T12:23:30Z

pkg/webhook/channel.go

+		if c.IsClosed() {
+			return nil
+		}
+		time.Sleep(interval)


can we derive sleep interval from limit ?

pperiyasamy · 2021-06-16T12:32:32Z

pkg/webhook/channel.go

+
+const (
+	interval = time.Millisecond * 2
+	chBuffer = 1


can move buffer size into Channel and configurable from NewChannel API?

pperiyasamy · 2021-06-16T12:34:03Z

pkg/webhook/channel.go

@@ -0,0 +1,92 @@
+package webhook


can we move this into another package pkg/tools/channel.go ?

I think in golang, its an antipattern to put funcs in packages named "tools" or "utils".
"Name your packages after what they provide, not what they contain."
However, having everything in package webhook is not good either and something I want to improve. I will move the channel stuff into a channel package. Thanks @pperiyasamy

good info! I will follow this convention ;)

pperiyasamy · 2021-06-16T12:42:00Z

pkg/webhook/webhook.go

@@ -832,3 +835,44 @@ func SetInjectHugepageDownApi(hugepageFlag bool) {
 func SetHonorExistingResources(resourcesHonorFlag bool) {
 	honorExistingResources = resourcesHonorFlag
 }
+
+//Watch blocks until either TLS cert & key watcher or HTTP server or termination signal generated
+func Watch(server nri.Service, watcher nri.Service, term chan os.Signal) (err error) {


can we make this method more generic by making nri.Service as var-args ?
this would also enable nad cache service for handling termination signal.

@pperiyasamy Good idea but I don't know how I could use select to watch all the service signals. If you can think of a pattern let me know.

can we try something like this (not tested, though) ?

ch := make(chan int) for idx := range services { go func(serv Service, servIdx int) { <-serv.StatusSignal() for otherSrvIdx, otherService := range services { if otherSrvIdx == servIdx { continue } otherService.Quit() } close(ch) }(services[idx], idx) } <-ch return

pperiyasamy · 2021-06-16T12:49:01Z

pkg/webhook/webhook.go

+	case <-watcher.StatusSignal(): // when TLS cert & key watcher finishes unexpectedly
+		glog.Error("TLS key & cert watcher ended")
+		err = server.Quit()
+	case <-server.StatusSignal(): // when HTTP server finishes unexpectedly


a question: when write happens on this server channel ? i couldn't find it anywhere in the code :)

@pperiyasamy see server.go: within method with pointer receiver Run() -> defer mSrv.Status.Close(). When the channel closes:

After calling close, and after any previously sent values have been received, receive operations will return the zero value for the channel's type without blocking.

ref: https://golang.org/ref/spec#Close

.. this causes this case to trigger

ah! ok, that means server.go'smSrv.instance.Start()is blocking call, right ?

zshi-redhat · 2021-06-18T03:55:32Z

@martinkennelly Would you mind rebase this PR? Thank you!

martinkennelly · 2021-09-28T08:16:09Z

Closing this as I don't have time to work on it. Anyone feel free to carry it on.

MichalGuzieniuk · 2021-09-28T12:35:29Z

@martinkennelly if you don't mind I will take codebase done by you in this PR, make the rebase and I will continue work on it.

martinkennelly · 2021-09-28T12:54:31Z

@MichalGuzieniuk Don't mind at all. I got stuck continuously refactoring NRI with this PR. Here is the outcome: https://github.com/martinkennelly/network-resources-injector/commits/refactor_webhook

The refactor went too far and I was concerned with that... I hope you can do a better job limiting the refactor scope than I did..
I think that refactor is good long term, just the scope of changes...

martinkennelly · 2021-09-28T13:04:38Z

I converted everything into services which could be watched, killed. I separated out all the code into pkgs as well.

martinkennelly changed the title ~~Catch termination signal~~ WIP: Catch termination signal Jan 25, 2021

martinkennelly force-pushed the term_sig_fix branch from 46ed783 to 3dcad43 Compare February 16, 2021 14:40

martinkennelly changed the title ~~WIP: Catch termination signal~~ Catch termination signal Feb 16, 2021

martinkennelly force-pushed the term_sig_fix branch from 3dcad43 to b6c8ce8 Compare February 16, 2021 14:50

martinkennelly force-pushed the term_sig_fix branch from b6c8ce8 to 19dc191 Compare February 16, 2021 15:03

pperiyasamy mentioned this pull request Jun 14, 2021

make webhook to use local nad cache #106

Merged

pperiyasamy reviewed Jun 16, 2021

View reviewed changes

martinkennelly closed this Sep 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch termination signal #62

Catch termination signal #62

martinkennelly commented Jan 19, 2021

martinkennelly commented Jan 25, 2021

martinkennelly commented Feb 19, 2021

martinkennelly commented Apr 10, 2021 •

edited

Loading

zshi-redhat commented Apr 12, 2021

pperiyasamy Jun 16, 2021

pperiyasamy Jun 16, 2021

pperiyasamy Jun 16, 2021

martinkennelly Jun 23, 2021 •

edited

Loading

pperiyasamy Jun 23, 2021

pperiyasamy Jun 16, 2021

martinkennelly Jun 23, 2021

pperiyasamy Jun 23, 2021 •

edited

Loading

pperiyasamy Jun 16, 2021

martinkennelly Jun 23, 2021

pperiyasamy Jun 23, 2021

zshi-redhat commented Jun 18, 2021

martinkennelly commented Sep 28, 2021

MichalGuzieniuk commented Sep 28, 2021

martinkennelly commented Sep 28, 2021 •

edited

Loading

martinkennelly commented Sep 28, 2021

Catch termination signal #62

Catch termination signal #62

Conversation

martinkennelly commented Jan 19, 2021

martinkennelly commented Jan 25, 2021

martinkennelly commented Feb 19, 2021

martinkennelly commented Apr 10, 2021 • edited Loading

zshi-redhat commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinkennelly Jun 23, 2021 • edited Loading

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

pperiyasamy Jun 23, 2021 • edited Loading

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

zshi-redhat commented Jun 18, 2021

martinkennelly commented Sep 28, 2021

MichalGuzieniuk commented Sep 28, 2021

martinkennelly commented Sep 28, 2021 • edited Loading

martinkennelly commented Sep 28, 2021

martinkennelly commented Apr 10, 2021 •

edited

Loading

martinkennelly Jun 23, 2021 •

edited

Loading

pperiyasamy Jun 23, 2021 •

edited

Loading

martinkennelly commented Sep 28, 2021 •

edited

Loading