-
Notifications
You must be signed in to change notification settings - Fork 21
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
Start and stop the Gatekeeper status sync controller dynamically #130
Start and stop the Gatekeeper status sync controller dynamically #130
Conversation
32ed675
to
56a9b2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this generally works. You mentioned this is causing slow GK tests: do you have numbers to compare the before/after here to check if it's actually helping?
f9c4714
to
3f805f0
Compare
@JustinKuli could you please take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's just had more time to settle in my head, but I think this is looking great
select { | ||
case <-ctx.Done(): | ||
// Stop the retry watcher if the parent context is canceled. It likely already is stopped, but this is not | ||
// documented behavior. | ||
watcher.Stop() | ||
|
||
// Satisfy the lostcancel linter but if this code is reached, then we know mgrCtx is cancelled since the | ||
// parent context (ctx) is cancelled. | ||
if mgrCtxCancel != nil { | ||
mgrCtxCancel() | ||
} | ||
|
||
return | ||
case <-watcher.Done(): | ||
// Restart the watcher on the next loop since the context wasn't closed which indicates it was not stopped | ||
// on purpose. | ||
watcher = nil | ||
case result := <-watcher.ResultChan(): | ||
// If the CRD is added, then Gatekeeper is installed. | ||
if result.Type == apiWatch.Added { | ||
gatekeeperInstalled = true | ||
} else if result.Type == apiWatch.Deleted { | ||
gatekeeperInstalled = false | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎 It's beautiful! A select statement where every case is a real thing to wait for!
Rather than rely on a health probe failing if the Gatekeeper installation status changes, just dynamically start and stop a new manager. Relates: https://issues.redhat.com/browse/ACM-10966 Signed-off-by: mprahl <mprahl@users.noreply.github.com>
Thanks @JustinKuli! I addressed your comments and I had forgotten about the use of the wait group in the main function, so I took that into account when the Gatekeeper sync manager is running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, mprahl 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:
Approvers can indicate their approval by writing |
3bc0a9e
into
open-cluster-management-io:main
Rather than rely on a health probe failing if the Gatekeeper installation status changes, just dynamically start and stop a new manager.
Relates:
https://issues.redhat.com/browse/ACM-10966