Skip to content

Only wait for config waitGroup when configManager is used #1508

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.

Sign up for GitHub

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

Open
chdxD1 wants to merge 1 commit into
base: master
Choose a base branch
from
+4 −2
Open

Only wait for config waitGroup when configManager is used #1508

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions cmd/multus-daemon/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,15 +128,17 @@ func main() {
}
}()

var wg sync.WaitGroup
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()
}
Comment on lines 131 to 140
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 6, 2026

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()


wg.Wait()
logging.Verbosef("multus daemon is exited")
}

Expand Down