Add ability to set cleanupConfigOnExit in the thick client config

[astro-stan]

Adds ability to set cleanupConfigOnExit in the thick client config.

Closes #1468

Summary by CodeRabbit

• New Features
  • Added a configurable option to control whether the old Multus configuration file is removed on exit; enabled by default to keep the system clean.
• Documentation
  • New documentation section explaining the cleanup-on-exit option, default behavior, risks of disabling it, and when disabling may be useful.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Warning

Rate limit exceeded

@astro-stan has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 34 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 2961cff and 64a3720.

📒 Files selected for processing (2)

• docs/configuration.md
• pkg/server/config/manager.go

Walkthrough

Adds a new configurable cleanupConfigOnExit option to Multus configuration and the Manager, making deletion of the Multus CNI config on exit conditional (default true) instead of unconditional.

Changes

Cohort / File(s)	Summary
Configuration field addition pkg/server/config/generator.go	Added CleanupConfigOnExit *bool to MultusConf with JSON tag cleanupConfigOnExit. ParseMultusConfig initializes this field to true by default and assigns its address into the struct.
Manager integration pkg/server/config/manager.go	Added cleanupConfigOnExit bool to Manager. newManager now reads the value from config.CleanupConfigOnExit and Start conditionally deletes the old multus config on exit only when this flag is true.
Documentation docs/configuration.md	Documented new cleanupConfigOnExit boolean option, its default (true), behavior, caveats, and scenarios where disabling cleanup may be desirable or harmful.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding the ability to configure cleanupConfigOnExit in the thick client config.
Linked Issues check	✅ Passed	All code changes directly address issue #1468: introducing cleanupConfigOnExit field to config, propagating it through Manager, and making cleanup conditional on this flag.
Out of Scope Changes check	✅ Passed	All changes are scoped to the stated objective: adding configuration support for cleanupConfigOnExit with documentation. No unrelated modifications detected.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

pkg/server/config/generator.go (1)

130-143: Consider clearing CleanupConfigOnExit in the generated config for consistency.

The Generate() method clears daemon-only fields like ReadinessIndicatorFile before generating the multus-shim configuration. Since CleanupConfigOnExit is consumed only by the Manager (daemon) and not by the shim's CmdAdd/CmdDel, consider clearing it here for consistency.

🔎 Proposed fix

 	// Readiness indicator file existence is already handled by the
 	// ConfigManager via an fsnotify watch, so CmdAdd/CmdDel don't need to.
 	mc.ReadinessIndicatorFile = ""
+	mc.CleanupConfigOnExit = nil
 
 	data, err := json.Marshal(mc)
 	return string(data), err

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 39d6a8f and fc34275.

📒 Files selected for processing (2)

• pkg/server/config/generator.go
• pkg/server/config/manager.go

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/server/config/manager.go (1)

pkg/logging/logging.go (1)

• Verbosef (131-133)

🔇 Additional comments (4)

pkg/server/config/generator.go (2)

55-55: LGTM! Field definition is appropriate.

The pointer type *bool correctly supports JSON omitempty semantics, distinguishing between unset, true, and false values.

---

75-83: LGTM! Default initialization ensures backward compatibility.

The initialization pattern correctly provides a default value of true while allowing JSON unmarshaling to override it. This ensures CleanupConfigOnExit is never nil when constructed via ParseMultusConfig.

pkg/server/config/manager.go (2)

46-46: LGTM! Field addition is straightforward.

The non-pointer bool type is appropriate for the Manager struct's internal state.

---

164-167: LGTM! Conditional cleanup logic correctly implements the feature.

The implementation properly gates config file deletion behind the cleanupConfigOnExit flag, preserving the default behavior (cleanup on exit) while allowing users to disable it. The verbose logging aids troubleshooting.

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc34275 and 2961cff.

📒 Files selected for processing (1)

• docs/configuration.md

🧰 Additional context used

🪛 markdownlint-cli2 (0.18.1)

docs/configuration.md

64-64: Link fragments should be valid
Expected: #cleanup-config-on-exit; Actual: #Cleanup-Config-On-Exit

(MD051, link-fragments)

🔇 Additional comments (1)

docs/configuration.md (1)

148-156: Documentation section is well-structured and informative.

The new section clearly explains the feature's default behavior, the motivation for disabling it (preventing missing interfaces when primary CNI starts first), and the trade-off (potential crash-loops). The caveats are appropriately highlighted.

[astro-stan]

Looking at the test log, I don't think the failing tests are related to my changes.