Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only restartChecks if pod is master for configReloader #942

Merged
merged 9 commits into from
May 19, 2021

Conversation

joshulyne
Copy link
Collaborator

Fixes #925

@joshulyne joshulyne requested a review from integrii May 13, 2021 22:55
@integrii
Copy link
Collaborator

Great find! I feel like this one should go into a different place, though.

Check out the control code here. This control loop is responsible for doing various check starts and stops and reloads. It feels like the configReloader func should be refactored to return a channel, then after reloading configuration, it should notify the channel that configuration has changed. That signal would go into the switch in the control code loop and result in this reload (but only if master!).

This way we have a central place to manage all reloading and check start/stopping control flow...

@integrii
Copy link
Collaborator

You could probably move the config reloader goroutine to be just above the control flow... up where there call to monitorKHJobs() is...

@joshulyne
Copy link
Collaborator Author

Great find! I feel like this one should go into a different place, though.

Check out the control code here. This control loop is responsible for doing various check starts and stops and reloads. It feels like the configReloader func should be refactored to return a channel, then after reloading configuration, it should notify the channel that configuration has changed. That signal would go into the switch in the control code loop and result in this reload (but only if master!).

This way we have a central place to manage all reloading and check start/stopping control flow...

makes sense! will refactor to do so!

// if we are master, stop, reconfigure our khchecks, and start again with the new configuration
if isMaster {
log.Infoln("control: Reloading external check configurations due to kuberhealthy configuration update")
k.RestartChecks(ctx)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would restart the reaper just in case. This way we are more likely to make any new configuration options that have been set for it take effect.

cmd/kuberhealthy/config.go Show resolved Hide resolved
Copy link
Collaborator

@jonnydawg jonnydawg left a comment

Choose a reason for hiding this comment

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

thanks!

Eric Greer and others added 9 commits May 19, 2021 09:21
Signed-off-by: joshulyne <joshpark118@gmail.com>
Signed-off-by: joshulyne <joshpark118@gmail.com>
update maintainers

Signed-off-by: joshulyne <joshpark118@gmail.com>
update maintainers

Signed-off-by: joshulyne <joshpark118@gmail.com>
Signed-off-by: joshulyne <joshpark118@gmail.com>
Signed-off-by: joshulyne <joshpark118@gmail.com>
Signed-off-by: joshulyne <joshpark118@gmail.com>
Signed-off-by: joshulyne <joshpark118@gmail.com>
Signed-off-by: joshulyne <joshpark118@gmail.com>
@joshulyne joshulyne dismissed integrii’s stale review May 19, 2021 16:22

resolved changes!

@joshulyne joshulyne merged commit 5be84e8 into master May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checks are getting scheduled at nearly the same time.
4 participants