Skip to content

Commit

Permalink
introduce a new runnable group for basic servers of the manager
Browse files Browse the repository at this point in the history
  • Loading branch information
zqzten committed May 22, 2023
1 parent e7b9407 commit 9a35806
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 2 deletions.
17 changes: 15 additions & 2 deletions pkg/manager/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,17 @@ func (cm *controllerManager) Start(ctx context.Context) (err error) {
}
}

// First start any webhook servers, which includes conversion, validation, and defaulting
// First start any basic servers, which includes health probes, metrics and profiling if enabled.
//
// WARNING: Basic servers MUST start before any cache is populated, otherwise it would block
// conversion webhooks to be ready for serving which make the cache never get ready.
if err := cm.runnables.BasicServers.Start(cm.internalCtx); err != nil {
if err != nil {
return fmt.Errorf("failed to start basic servers: %w", err)
}
}

// Start any webhook servers, which includes conversion, validation, and defaulting
// webhooks that are registered.
//
// WARNING: Webhooks MUST start before any cache is populated, otherwise there is a race condition
Expand Down Expand Up @@ -591,10 +601,13 @@ func (cm *controllerManager) engageStopProcedure(stopComplete <-chan struct{}) e
cm.logger.Info("Stopping and waiting for caches")
cm.runnables.Caches.StopAndWait(cm.shutdownCtx)

// Webhooks should come last, as they might be still serving some requests.
// Webhooks and basic servers should come last, as they might be still serving some requests.
cm.logger.Info("Stopping and waiting for webhooks")
cm.runnables.Webhooks.StopAndWait(cm.shutdownCtx)

cm.logger.Info("Stopping and waiting for basic servers")
cm.runnables.BasicServers.StopAndWait(cm.shutdownCtx)

// Proceed to close the manager and overall shutdown context.
cm.logger.Info("Wait completed, proceeding to shutdown the manager")
shutdownCancel()
Expand Down
4 changes: 4 additions & 0 deletions pkg/manager/runnable_group.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ type runnableCheck func(ctx context.Context) bool
// runnables handles all the runnables for a manager by grouping them accordingly to their
// type (webhooks, caches etc.).
type runnables struct {
BasicServers *runnableGroup
Webhooks *runnableGroup
Caches *runnableGroup
LeaderElection *runnableGroup
Expand All @@ -37,6 +38,7 @@ type runnables struct {
// newRunnables creates a new runnables object.
func newRunnables(baseContext BaseContextFunc, errChan chan error) *runnables {
return &runnables{
BasicServers: newRunnableGroup(baseContext, errChan),
Webhooks: newRunnableGroup(baseContext, errChan),
Caches: newRunnableGroup(baseContext, errChan),
LeaderElection: newRunnableGroup(baseContext, errChan),
Expand All @@ -52,6 +54,8 @@ func newRunnables(baseContext BaseContextFunc, errChan chan error) *runnables {
// The runnables added after Start are started directly.
func (r *runnables) Add(fn Runnable) error {
switch runnable := fn.(type) {
case *server:
return r.BasicServers.Add(fn, nil)
case hasCache:
return r.Caches.Add(fn, func(ctx context.Context) bool {
return runnable.GetCache().WaitForCacheSync(ctx)
Expand Down
7 changes: 7 additions & 0 deletions pkg/manager/runnable_group_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ var _ = Describe("runnables", func() {
Expect(newRunnables(defaultBaseContext, errCh)).ToNot(BeNil())
})

It("should add basic servers to the appropriate group", func() {
server := &server{}
r := newRunnables(defaultBaseContext, errCh)
Expect(r.Add(server)).To(Succeed())
Expect(r.BasicServers.startQueue).To(HaveLen(1))
})

It("should add caches to the appropriate group", func() {
cache := &cacheProvider{cache: &informertest.FakeInformers{Error: fmt.Errorf("expected error")}}
r := newRunnables(defaultBaseContext, errCh)
Expand Down

0 comments on commit 9a35806

Please sign in to comment.