PoC: auto-detect NetworkManager and use nmstatectl kernel mode#1455
PoC: auto-detect NetworkManager and use nmstatectl kernel mode#1455qinqon wants to merge 1 commit intonmstate:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubevirt-bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This Proof of Concept pull request introduces a significant enhancement to kubernetes-nmstate by enabling it to operate effectively in environments lacking NetworkManager. By automatically detecting NetworkManager's absence and transparently switching to Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
qinqon
force-pushed
the
nmstate-kernel-mode
branch
from
3c80bc6 to
635900b
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a proof-of-concept for running kubernetes-nmstate in a 'kernel mode' when NetworkManager is not available. The changes include auto-detection of NetworkManager at handler startup, a global flag to enable kernel mode, and modifications to nmstatectl commands to use the -k flag and bypass checkpointing features. The liveness probe is also updated to work in both modes. The changes are logical and well-contained for a PoC. I have a few suggestions to improve maintainability and robustness, mainly around the use of a hardcoded flag file path, global state management, and the complexity of the liveness probe command.
| detectAndSetKernelMode() | ||
|
|
||
| if nmstatectl.IsKernelMode() { | ||
| if err := file.Touch("/tmp/kernel-mode"); err != nil { |
There was a problem hiding this comment.
The file path /tmp/kernel-mode is hardcoded. To improve maintainability and avoid magic strings, it would be better to define this path as a constant in a shared package (e.g., in pkg/environment or a new pkg/constants). This path is also used in the liveness probe definition in deploy/handler/operator.yaml and related tests, so a constant would ensure consistency.
| - bash | ||
| - -c | ||
| - "nmstatectl show {{ .HandlerReadinessProbeExtraArg }} 2>&1" | ||
| - "if [ -f /tmp/kernel-mode ]; then nmstatectl show -k {{ .HandlerReadinessProbeExtraArg }} 2>&1; else nmstatectl show {{ .HandlerReadinessProbeExtraArg }} 2>&1; fi" |
There was a problem hiding this comment.
The shell command for the liveness probe is complex and is duplicated in tests, which can be brittle and hard to maintain.
if [ -f /tmp/kernel-mode ]; then nmstatectl show -k {{ .HandlerReadinessProbeExtraArg }} 2>&1; else nmstatectl show {{ .HandlerReadinessProbeExtraArg }} 2>&1; fiConsider adding a small wrapper script to the container image (e.g., /usr/bin/liveness-probe.sh) that encapsulates this logic. The liveness probe would then simply execute this script. This would make the YAML manifest cleaner and centralize the probe logic.
| var ( | ||
| debugMode bool | ||
| kernelMode bool | ||
| log = logf.Log.WithName("nmstatectl") | ||
| ) |
There was a problem hiding this comment.
The use of global variables kernelMode and debugMode makes this package stateful, which can lead to issues with concurrency and makes testing more complex (requiring state manipulation with defer). While this might be acceptable for a PoC where it's set once at startup, for a production-ready feature, it would be more robust to pass configuration through a context or a configuration struct to the functions in this package. This would make the functions stateless and easier to reason about.
qinqon
force-pushed
the
nmstate-kernel-mode
branch
5 times, most recently
from
a70b99c to
191a8e0
Compare
qinqon
force-pushed
the
nmstate-kernel-mode
branch
3 times, most recently
from
b758985 to
3f5bdb5
Compare
qinqon
force-pushed
the
nmstate-kernel-mode
branch
5 times, most recently
from
b379d69 to
e4c4a50
Compare
qinqon
force-pushed
the
nmstate-kernel-mode
branch
from
e4c4a50 to
034c590
Compare
qinqon
force-pushed
the
nmstate-kernel-mode
branch
11 times, most recently
from
d97feb1 to
6d0a5b3
Compare
qinqon
force-pushed
the
nmstate-kernel-mode
branch
2 times, most recently
from
fcf4e98 to
006f178
Compare
When NetworkManager is not available (e.g., kind containers, minimal hosts), the handler now auto-detects this at startup and transparently switches to nmstatectl kernel mode (-k flag). This affects both applying network config (NNCP) and reporting state (NNS). Key changes: - Add kernelMode flag to nmstatectl package with -k flag for show/apply - Skip checkpoint/rollback/commit/probes in kernel mode (not supported) - Auto-detect NM absence at handler startup via nm.Version() - Guard NM version reporting in node controller - Update liveness probe to use -k flag when kernel-mode file exists - Add init container to ensure dbus socket path exists on nodes without dbus - Add feature Labels to all e2e handler tests for mode-based filtering - Make BeforeSuite work in kernel mode (no DHCP reset) - Add test-e2e-handler-kernel Makefile target with label filter - Add GitHub Actions CI workflow for kernel mode e2e on kind Kernel mode limitations: no checkpoint/rollback, no DHCP, volatile changes, cannot create interfaces via nmstatectl apply -k (nispor plugin limitation). Signed-off-by: Enrique Llorente <ellorent@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Enrique Llorente <ellorent@redhat.com>
qinqon
force-pushed
the
nmstate-kernel-mode
branch
from
006f178 to
f17650e
Compare
2 participants
Summary
This is a Proof of Concept — not intended to merge as-is.
Auto-detect NetworkManager presence at handler startup and transparently switch to
nmstatectlkernel mode (-kflag) when NM is absent. This enables kubernetes-nmstate to run on nodes without NetworkManager (e.g., kind containers, minimal hosts).nm.Version()at startup; if NM is unavailable, setskernelMode = truenmstatectl show/applyuse-kflag in kernel mode-kwhen/tmp/kernel-modeflag file existsLabel()decorators for mode-based filtering (--label-filter)make test-e2e-handler-kerneltarget runs only kernel-supported testsKernel mode limitations: no checkpoint/rollback, no DHCP, changes are volatile (lost on reboot).
Depends-on:
Verified on kind
hostNetworkManagerVersionreportsN/A (kernel mode)Test plan
go test ./pkg/... ./controllers/...)make test-e2e-handler-kernel)make test-e2e-handler)🤖 Generated with Claude Code