Refactor daemonset check #515
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonnydawg
requested changes
Jun 8, 2020
cmd/daemonset-check-refactor/main.go
Outdated
| // If there is a cancellation interrupt signal. | ||
| log.Infoln("Canceling removing daemonset and daemonset pods and shutting down from interrupt.") | ||
| return | ||
| default: |
There was a problem hiding this comment.
wouldn't this always hit the default case? I think there are only three cases here -- your first three w/o default. Especially since you aren't using a for-loop over the select statement, it will just skip straight to the default case
cmd/daemonset-check-refactor/main.go
Outdated
| log.Infoln("Worker: waitForPodRemoval started") | ||
| defer close(outChan) | ||
| outChan <- waitForPodRemoval() | ||
| wg.Wait() |
cmd/daemonset-check-refactor/main.go
Outdated
| log.Infoln("Received an interrupt signal from the signal channel.") | ||
| log.Debugln("Signal received was:", sig.String()) | ||
|
|
||
| go shutdown(doneChan) |
There was a problem hiding this comment.
i think it would be prudent to pass the context to this function and call the ctxCancel func here -- most of your check run steps respect a ctx.Done() which would return a result if the context was canceled
| log.Infoln("Worker: waitForAllDaemonsetsToClear started") | ||
| defer close(outChan) | ||
| outChan <- waitForAllDaemonsetsToClear() | ||
| wg.Wait() |
There was a problem hiding this comment.
same with the wg.Done() here
| go func() { | ||
| defer close(outChanDS) | ||
| outChanDS <- waitForDSRemoval() | ||
| wg.Wait() |
|
Before we merge this to master, lets move the paths back to |
jonnydawg
reviewed
Jun 12, 2020
cmd/daemonset-check/run_check.go
Outdated
| defer wg.Done() | ||
| defer close(outChan) | ||
| outChan <- waitForAllDaemonsetsToClear() | ||
| wg.Wait() |
There was a problem hiding this comment.
I think this wg.Wait() will always block here since you deferd the wg.Done() within the same function. wg.Wait() is going to block until all wg.Done()s are called, but since you defer it here, it would always be called after the wg.Wait()
|
I have the build as of ecf5d86 running locally without errors: |
jonnydawg
reviewed
Jun 18, 2020
cmd/daemonset-check/main.go
Outdated
|
|
||
| // daemonset does not exist, return false | ||
| return false, nil | ||
| os.Exit(0) |
There was a problem hiding this comment.
I think you might want to defer the os.Exit() calls to allow shutdownCtxCancel() to be called before the process exits since shutdownCtxCancel() was deferred
jonnydawg
approved these changes
Jun 18, 2020
joshulyne
commented
Jun 19, 2020
| } | ||
| log.Infoln("Finished cleanup. No rogue daemonsets or daemonset pods exist") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
instead of having a whole separate runCheckCleanup function like before, we just use the remove function to remove any daemonset created by the khcheck since there shouldn't be any left around. before there was a ton of logic to determine whether or not the daemonset was rogue but any daemonset left after a check run should be removed i believe
There was a problem hiding this comment.
this simplifies a LOT of the cleanUp stuff we were doing before
joshulyne
commented
Jun 19, 2020
cmd/daemonset-check/main.go
Outdated
| log.Infoln("Done running daemonset check") | ||
| case <-signalChan: | ||
| log.Infoln("Received shutdown signal. Canceling context and proceeding directly to cleanup.") | ||
| reportOKToKuberhealthy() |
There was a problem hiding this comment.
added a reportOKToKuberhealthy since if no report is made, kuberhealthy defaults the error to: Check execution error: kuberhealthy/daemonset: timed out waiting for checker pod to report in . not sure if we want to report an error instead
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.