Skip to content

Only wait for config waitGroup when configManager is used#1508

Open
chdxD1 wants to merge 1 commit into
k8snetworkplumbingwg:masterfrom
chdxD1:fix/wg-config
Open

Only wait for config waitGroup when configManager is used#1508
chdxD1 wants to merge 1 commit into
k8snetworkplumbingwg:masterfrom
chdxD1:fix/wg-config

Conversation

@chdxD1
Copy link
Copy Markdown

@chdxD1 chdxD1 commented May 6, 2026

reopen of #1398 with rebase on master

When MultusConfigFile is not set to auto (configManager is nil) the WaitGroup is initialised and then waited for, immediately exiting Multus. This PR only waits when configManager is not nil, otherwise it waits for the context to complete (which happens from the SIGINT / SIGTERM handlers)

Signed-off-by: Christopher Dziomba <christopher.dziomba@telekom.de>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request modifies the multus-daemon entry point to prevent the process from exiting immediately when the configuration manager is not initialized. The reviewer suggested a more idiomatic implementation that simplifies the control flow by waiting for context completion before synchronizing background tasks, which ensures a more robust shutdown sequence.

Comment thread cmd/multus-daemon/main.go
Comment on lines 131 to 140
if configManager != nil {
var wg sync.WaitGroup
if err := configManager.Start(ctx, &wg); err != nil {
_ = logging.Errorf("failed to start config manager: %v", err)
os.Exit(3)
}
wg.Wait()
} else {
<-ctx.Done()
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current implementation correctly fixes the issue where the daemon exits immediately when configManager is nil. However, the logic can be made more idiomatic and robust by explicitly waiting for the context completion in all cases, followed by waiting for any background tasks (like the configuration manager) to finish their cleanup. This also avoids the if/else branching for the wait logic.

Additionally, note that the CNI server started in startMultusDaemon also performs a graceful shutdown when the context is cancelled, but there is currently no synchronization to ensure it finishes before the process exits. While out of scope for this specific fix, consider using a shared WaitGroup for both the server and the config manager in the future.

Suggested change
if configManager != nil {
var wg sync.WaitGroup
if err := configManager.Start(ctx, &wg); err != nil {
_ = logging.Errorf("failed to start config manager: %v", err)
os.Exit(3)
}
wg.Wait()
} else {
<-ctx.Done()
}
var wg sync.WaitGroup
if configManager != nil {
if err := configManager.Start(ctx, &wg); err != nil {
_ = logging.Errorf("failed to start config manager: %v", err)
os.Exit(3)
}
}
<-ctx.Done()
wg.Wait()

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.

1 participant