feat: support this role in container builds #814
Conversation
Reviewer's GuideEnables offline operation of the network role in buildah environments by introducing an nm_offline provider and boot detection, refactors library methods for command execution and file header handling, updates defaults and main tasks for offline support, and restructures the integration tests and CI workflow with a unified run_test harness to handle build and boot scenarios. Class diagram for new and updated command provider classesclassDiagram
class Cmd {
+managed_file_header
+run_command(argv, encoding=None, check_rc=False)
+log(...)
}
class Cmd_nm {
+managed_file_header
+run_command(argv, encoding=None, check_rc=False)
}
class Cmd_nm_offline {
+profile_path(name)
+run_prepare()
+connection_create(connections, idx)
+run_action_present(idx)
+run_action_absent(idx)
+run_action_up(idx)
+run_action_down(idx)
}
class Cmd_initscripts {
+managed_file_header
+run_command(argv, encoding=None, check_rc=False)
}
Cmd <|-- Cmd_nm
Cmd <|-- Cmd_nm_offline
Cmd <|-- Cmd_initscripts
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `library/network_connections.py:3120-3121` </location>
<code_context>
+ return
+
+ path = self.profile_path(connection["name"])
+ if self.check_mode == CheckMode.REAL_RUN:
+ with open(path, "w") as f:
+ f.write(self.run_env.managed_file_header)
+ f.write(out.decode("UTF-8"))
+ os.chmod(path, stat.S_IRUSR | stat.S_IWUSR)
+ os.chown(path, 0, 0)
+
+ # nmcli always generates a new UUID, so comparing with existing file is moot
</code_context>
<issue_to_address>
**suggestion:** os.chown may fail in non-root or container environments.
Catch PermissionError from os.chown or skip this step if not running as root to avoid failures in restricted environments.
```suggestion
os.chmod(path, stat.S_IRUSR | stat.S_IWUSR)
# Only attempt chown if running as root, and catch PermissionError
if hasattr(os, "geteuid") and os.geteuid() == 0:
try:
os.chown(path, 0, 0)
except PermissionError:
# Log or silently skip if chown fails due to permissions
pass
```
</issue_to_address>
### Comment 2
<location> `library/network_connections.py:3099-3102` </location>
<code_context>
+ self.log_error(idx, "Connection 'type' not specified")
+ return
+
+ # DEBUG, drop for final commit
+ self.log_info(
+ idx, "XXX nm_offline provider action_present connection: %r" % connection
</code_context>
<issue_to_address>
**suggestion:** Remove debug logging before final merge.
Please remove the 'log_info' statements marked as debug to prevent extra log output in production.
```suggestion
```
</issue_to_address>
### Comment 3
<location> `library/network_connections.py:3109` </location>
<code_context>
+ self.connection_create(self.connections, idx), check_rc=True
+ )
+ # Should Not Happen™, but make sure
+ if not out.strip():
+ self.log_error(
+ idx, "nmcli --offline returned no output (rc %d); err: %s" % (rc, err)
</code_context>
<issue_to_address>
**suggestion:** Consider handling out as bytes or str consistently.
Since out is used with both strip() and decode('UTF-8'), ensure run_command consistently returns bytes, or add type checks to prevent type errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| os.chmod(path, stat.S_IRUSR | stat.S_IWUSR) | ||
| os.chown(path, 0, 0) |
There was a problem hiding this comment.
suggestion: os.chown may fail in non-root or container environments.
Catch PermissionError from os.chown or skip this step if not running as root to avoid failures in restricted environments.
| os.chmod(path, stat.S_IRUSR | stat.S_IWUSR) | |
| os.chown(path, 0, 0) | |
| os.chmod(path, stat.S_IRUSR | stat.S_IWUSR) | |
| # Only attempt chown if running as root, and catch PermissionError | |
| if hasattr(os, "geteuid") and os.geteuid() == 0: | |
| try: | |
| os.chown(path, 0, 0) | |
| except PermissionError: | |
| # Log or silently skip if chown fails due to permissions | |
| pass |
| # DEBUG, drop for final commit | ||
| self.log_info( | ||
| idx, "XXX nm_offline provider action_present connection: %r" % connection | ||
| ) |
There was a problem hiding this comment.
suggestion: Remove debug logging before final merge.
Please remove the 'log_info' statements marked as debug to prevent extra log output in production.
| # DEBUG, drop for final commit | |
| self.log_info( | |
| idx, "XXX nm_offline provider action_present connection: %r" % connection | |
| ) |
| self.connection_create(self.connections, idx), check_rc=True | ||
| ) | ||
| # Should Not Happen™, but make sure | ||
| if not out.strip(): |
There was a problem hiding this comment.
suggestion: Consider handling out as bytes or str consistently.
Since out is used with both strip() and decode('UTF-8'), ensure run_command consistently returns bytes, or add type checks to prevent type errors.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| class Cmd_nm_offline(Cmd): | ||
| def __init__(self, **kwargs): | ||
| Cmd.__init__(self, **kwargs) | ||
| self.validate_one_type = ArgValidator_ListConnections.VALIDATE_ONE_MODE_NM |
Check warning
Code scanning / CodeQL
Overwriting attribute in super-class or sub-class Warning
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #814 +/- ##
==========================================
- Coverage 43.11% 39.90% -3.22%
==========================================
Files 12 12
Lines 3124 3408 +284
==========================================
+ Hits 1347 1360 +13
- Misses 1777 2048 +271 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
richm
force-pushed
the
bootc
branch
2 times, most recently
from
0e4baf0 to
3dca657
Compare
| if "nm.exists" not in connection: | ||
| connection["nm.exists"] = True | ||
| argv = [ | ||
| "nmcli", |
There was a problem hiding this comment.
Instead of spawning nmcli, did you consider using libnm to create the connection in memory (similarly to NMUtil.connection_create) and then using nm_keyfile_write() to write it to disk? Probably that would allow some code reuse.
There was a problem hiding this comment.
Instead of spawning nmcli, did you consider using libnm to create the connection in memory (similarly to
NMUtil.connection_create) and then using nm_keyfile_write() to write it to disk? Probably that would allow some code reuse.
I tried this - I got this error - fatal error: failure loading libnm library: g-io-error-quark: Could not connect: No such file or directory (1) - I'm assuming in order to do it the way you suggested, dbus needs to be running? and/or NetworkMananger? If so - that's not going to work during image build or bootc build - is there a way to tell the api to work in offline mode?
There was a problem hiding this comment.
I suppose you called NM.Client.new(None)? That creates a client object that tries to connect to NM over D-Bus.
For offline operations, don't create a client object. Instead, build the NM.SimpleConnection object and then write it with NM.keyfile_write(). Like in this sample code:
#!/usr/bin/env python
# SPDX-License-Identifier: LGPL-2.1-or-later
import sys
import os
import uuid
import gi
gi.require_version("NM", "1.0")
from gi.repository import NM, GLib
def print_values(setting, key, value, flags, data):
print(" %s.%s: %s" % (setting.get_name(), key, value))
def create_profile(name):
profile = NM.SimpleConnection.new()
s_con = NM.SettingConnection.new()
s_con.set_property(NM.SETTING_CONNECTION_ID, name)
s_con.set_property(NM.SETTING_CONNECTION_UUID, str(uuid.uuid4()))
s_con.set_property(NM.SETTING_CONNECTION_TYPE, "802-3-ethernet")
s_wired = NM.SettingWired.new()
s_ip4 = NM.SettingIP4Config.new()
s_ip4.set_property(NM.SETTING_IP_CONFIG_METHOD, "auto")
s_ip6 = NM.SettingIP6Config.new()
s_ip6.set_property(NM.SETTING_IP_CONFIG_METHOD, "auto")
profile.add_setting(s_con)
profile.add_setting(s_ip4)
profile.add_setting(s_ip6)
profile.add_setting(s_wired)
print(" *** Created connection profile:")
profile.for_each_setting_value(print_values, None)
return profile
def kf_handler_write(connection, keyfile, handler_type, handler_data, user_data):
globals()["kf_handler_write_cnt"] = kf_handler_write_cnt
[kf_group, kf_key, cur_setting, cur_property] = handler_data.get_context()
print(f" - handler: {kf_group} {kf_key} {cur_setting}.{cur_property}\n");
if handler_type == NM.KeyfileHandlerType.WRITE_CERT:
# TODO handle writing certificates
return False
return False
if __name__ == "__main__":
profile = create_profile("test")
try:
print("")
print(" *** Writing keyfile...")
keyfile = NM.keyfile_write(profile, NM.KeyfileHandlerFlags.NONE, kf_handler_write, 0)
except Exception as e:
print("Write failed: %r" % (e))
raise
[kf_string,_] = keyfile.to_data()
print("")
print(f" *** Keyfile:\n{kf_string}")Or have a look at https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/main/examples/python/gi/nm-keyfile.py
There was a problem hiding this comment.
I suppose you called
NM.Client.new(None)? That creates a client object that tries to connect to NM over D-Bus.
Yes - https://github.com/linux-system-roles/network/blob/main/module_utils/network_lsr/nm/client.py#L47
def get_client():
return NM.Client.new()For offline operations, don't create a client object. Instead, build the
NM.SimpleConnectionobject and then write it withNM.keyfile_write(). Like in this sample code:
ok - I'll try it
#!/usr/bin/env python # SPDX-License-Identifier: LGPL-2.1-or-later import sys import os import uuid import gi gi.require_version("NM", "1.0") from gi.repository import NM, GLib def print_values(setting, key, value, flags, data): print(" %s.%s: %s" % (setting.get_name(), key, value)) def create_profile(name): profile = NM.SimpleConnection.new() s_con = NM.SettingConnection.new() s_con.set_property(NM.SETTING_CONNECTION_ID, name) s_con.set_property(NM.SETTING_CONNECTION_UUID, str(uuid.uuid4())) s_con.set_property(NM.SETTING_CONNECTION_TYPE, "802-3-ethernet") s_wired = NM.SettingWired.new() s_ip4 = NM.SettingIP4Config.new() s_ip4.set_property(NM.SETTING_IP_CONFIG_METHOD, "auto") s_ip6 = NM.SettingIP6Config.new() s_ip6.set_property(NM.SETTING_IP_CONFIG_METHOD, "auto") profile.add_setting(s_con) profile.add_setting(s_ip4) profile.add_setting(s_ip6) profile.add_setting(s_wired) print(" *** Created connection profile:") profile.for_each_setting_value(print_values, None) return profile def kf_handler_write(connection, keyfile, handler_type, handler_data, user_data): globals()["kf_handler_write_cnt"] = kf_handler_write_cnt [kf_group, kf_key, cur_setting, cur_property] = handler_data.get_context() print(f" - handler: {kf_group} {kf_key} {cur_setting}.{cur_property}\n"); if handler_type == NM.KeyfileHandlerType.WRITE_CERT: # TODO handle writing certificates return False return False if __name__ == "__main__": profile = create_profile("test") try: print("") print(" *** Writing keyfile...") keyfile = NM.keyfile_write(profile, NM.KeyfileHandlerFlags.NONE, kf_handler_write, 0) except Exception as e: print("Write failed: %r" % (e)) raise [kf_string,_] = keyfile.to_data() print("") print(f" *** Keyfile:\n{kf_string}")Or have a look at https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/main/examples/python/gi/nm-keyfile.py
|
[citest] |
|
[citest] |
|
[citest] |
Feature: Support running the timesync role during container builds. Reason: This is particularly useful for building bootc derivative OSes. Result: These flags enable running the bootc container scenarios in CI, which ensures that the role works in buildah build environment. This allows us to officially support this role for image mode builds. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
|
[citest] |
1 similar comment
|
[citest] |
|
Thank you for your contribution! There was no activity in this pull request recently. To avoid pull requests to pile up, an automated process marked this pull request as stale. It will close this pull request if no further activity occurs. The current policy is available at: https://github.com//linux-system-roles/network/blob/main/.github/stale.yml |
3 participants
Feature: Support running the timesync role during container builds.
Reason: This is particularly useful for building bootc derivative OSes.
Result: These flags enable running the bootc container scenarios in CI,
which ensures that the role works in buildah build environment. This
allows us to officially support this role for image mode builds.
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Enable offline NetworkManager support to allow the role to run in container build environments, refactor the command API and provider switching logic, and overhaul the test suite with a reusable harness and helper tasks to streamline CI testing
New Features:
nm_offline) for container build environmentsEnhancements:
check_rcflag torun_commandand rename theifcfg_headerproperty tomanaged_file_header__network_is_booted) and switch tonm_offlinewhen necessaryCI:
Tests:
run_test.ymlharness with modular setup, assert, and cleanup tasksassert_connection_settings.yml,assert_command_output.yml,assert_nm_connection_status.yml, and handlers)buildahenvironments and increase the default test timeout