Skip to content

feat(opennebula): support ETHx_ROUTES static routes in network config#6810

Merged
blackboxsw merged 1 commit into
canonical:mainfrom
mcanevet:feat/opennebula-eth-routes
May 5, 2026
Merged

feat(opennebula): support ETHx_ROUTES static routes in network config#6810
blackboxsw merged 1 commit into
canonical:mainfrom
mcanevet:feat/opennebula-eth-routes

Conversation

@mcanevet
Copy link
Copy Markdown
Contributor

Proposed Commit Message

feat(opennebula): support ETHx_ROUTES static routes in network config

Add `get_routes()` to `OpenNebulaNetwork` to parse the `ETHx_ROUTES`
context variable (format: "NETWORK via GATEWAY, ...") and emit the
resulting routes into the Netplan v2 `routes:` list in `gen_conf()`.

Malformed entries are skipped with a warning. No `routes` key is
emitted when the variable is absent or empty, preserving backward
compatibility.

Additional Context

The OpenNebula context-linux package (one-apps) supports ETHx_ROUTES
to configure static routes per interface. cloud-init's OpenNebula
datasource was silently ignoring this variable, meaning any static
routes defined in the VM template were never applied.

Test Steps

On an OpenNebula VM, add to the VM template context section:

ETH0_ROUTES="10.0.0.0/8 via 192.168.1.1, 172.16.0.0/12 via 192.168.1.254"

After boot, verify the routes were applied:

ip route show
# expected: routes to 10.0.0.0/8 and 172.16.0.0/12 via 192.168.1.x

Also verify that a VM with no ETHx_ROUTES set continues to boot and
configure networking without any change in behaviour.

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@github-actions github-actions Bot added the documentation This Pull Request changes documentation label Mar 30, 2026
@mcanevet mcanevet force-pushed the feat/opennebula-eth-routes branch 2 times, most recently from 8c4e264 to e5efa6e Compare March 30, 2026 16:16
@holmanb
Copy link
Copy Markdown
Member

holmanb commented Mar 30, 2026

Hey @mcanevet. What is your goal and intent with these PRs? It doesn't look like you are trying to a single missing feature. Are you employed by Open Nebula?

@mcanevet
Copy link
Copy Markdown
Contributor Author

I'm just trying to add support for context variables that are honored by Open Nebula's official agent. And I'm not working for them, just a simple user that would like to use cloud-init with complex network configuration. Do you prefer a single PR for all of this?

@mcanevet mcanevet force-pushed the feat/opennebula-eth-routes branch 2 times, most recently from eff0ece to 3d4c3f9 Compare April 1, 2026 08:28
@mcanevet
Copy link
Copy Markdown
Contributor Author

mcanevet commented Apr 1, 2026

@holmanb do you prefer a single PR to implement this feature that reduces the gap between the OpenNebula agent (https://github.com/OpenNebula/one-apps/tree/master/context-linux) and Cloud-init? We basically need this to use cloud-init with standard OpenNebula context variables that the official agent support.

@holmanb
Copy link
Copy Markdown
Member

holmanb commented Apr 1, 2026

I have a few concerns:

  1. The proposed PR would change existing behavior, so stable distros would need to patch these changes out.
  2. The OpenNebula Python datasource code type checking is disabled (see pyproject.toml). If the existing code first passed mypy's checks, it would be easier to have confidence in subsequent changes to this code.
  3. Not something that you are responsible for or that I expect you to change, but Opennebula's design choice to provide a bash script as the source of platform information is not ideal for consuming that information from Python.

Additionally, I would like to see the the full cloud-init logs on the system that you tested these changes on as well as from an instance without these changes applied - as well as a context.sh file content.

mcanevet added a commit to mcanevet/cloud-init that referenced this pull request Apr 2, 2026
- Add type annotations to DataSourceOpenNebula.py
- Add type annotations to read_context_disk_dir() function
- Add type annotation for gen_conf() return type
- Use assertions for type narrowing where needed
- Add type: ignore for mock assignment in tests

Addresses concern from PR canonical#6810 about type checking being disabled.
mcanevet added a commit to mcanevet/cloud-init that referenced this pull request Apr 2, 2026
Add type annotations to DataSourceOpenNebula.py to address concerns
raised in PR canonical#6810 about type checking being disabled for the
OpenNebula datasource.

Refs canonicalGH-6810
@mcanevet
Copy link
Copy Markdown
Contributor Author

mcanevet commented Apr 2, 2026

Hi @holmanb,

regarding first concern about changing existing behavior:

Looking at the diff, I believe this change is purely additive. The get_routes() method is new, and in gen_conf() the routes key is only added to the device config when ETHx_ROUTES is explicitly present in the context variables:

extra_routes = self.get_routes(c_dev)
if extra_routes:
  devconf["routes"] = extra_routes

If ETHx_ROUTES is absent (as it would be in all existing deployments), get_routes() returns an empty list, the condition is false, and gen_conf() output is identical to today's. No existing context variables are reinterpreted or removed.

Could you clarify what specific behavior you see changing? I want to make sure I'm not missing something.

Regarding the second concern, I enabled type checking in #6827.

@mcanevet
Copy link
Copy Markdown
Contributor Author

mcanevet commented Apr 2, 2026

Here is a gist with context.sh, cloud-init.log and cloud-init-output.log:
https://gist.github.com/mcanevet/7561c97acd64c8ebd980843bc8ca90ee

@mcanevet
Copy link
Copy Markdown
Contributor Author

mcanevet commented Apr 2, 2026

you can see in the logs that before my patch, the ETH0_ROUTES context variable is not used.
here is the official documentation for the context variables: https://docs.opennebula.io/6.10/management_and_operations/references/template.html#context-section

mcanevet added a commit to mcanevet/cloud-init that referenced this pull request Apr 3, 2026
Add type annotations to DataSourceOpenNebula.py to address concerns
raised in PR canonical#6810 about type checking being disabled for the
OpenNebula datasource.

Refs canonicalGH-6810
mcanevet added a commit to mcanevet/cloud-init that referenced this pull request Apr 10, 2026
Add type annotations to parse_shell_config, get_field (with @overload),
and all OpenNebulaNetwork.get_* methods as requested. Replace assert-based
type narrowing with proper guards. Remove leaked global assignment in test
and revert unrelated cosmetic changes.

Refs canonicalGH-6810
mcanevet added a commit to mcanevet/cloud-init that referenced this pull request Apr 10, 2026
Add type annotations to parse_shell_config, get_field (with @overload),
and all OpenNebulaNetwork.get_* methods as requested. Replace assert-based
type narrowing with proper guards. Remove leaked global assignment in test
and revert unrelated cosmetic changes.

Refs canonicalGH-6810
mcanevet added a commit to mcanevet/cloud-init that referenced this pull request Apr 10, 2026
Add type annotations to parse_shell_config, get_field (with @overload),
and all OpenNebulaNetwork.get_* methods as requested. Replace assert-based
type narrowing with proper guards. Remove leaked global assignment in test
and revert unrelated cosmetic changes.

Refs canonicalGH-6810
mcanevet added a commit to mcanevet/cloud-init that referenced this pull request Apr 13, 2026
Context values come exclusively from parse_shell_config which returns
Dict[str, str]. Shell variables can only be absent or hold string values,
so a None context value is impossible in production. The equivalent
realistic scenario is already covered by test_get_field_emptycontext.

Refs canonicalGH-6810
mcanevet added a commit to mcanevet/cloud-init that referenced this pull request Apr 13, 2026
…idates

When test_find_candidates was refactored from a try/finally pattern to
@mock.patch in d482353, the bare global assignment
`util.find_devs_with = my_devs_with` was left in by mistake. The
try/finally used to save and restore the original; without it, this
assignment leaks state and can cause order-dependent failures in other
tests. The @mock.patch decorator already patches the function correctly
and restores it after the test.

Refs canonicalGH-6810
mcanevet added a commit to mcanevet/cloud-init that referenced this pull request Apr 14, 2026
…idates

When test_find_candidates was refactored from a try/finally pattern to
@mock.patch in d482353, the bare global assignment
`util.find_devs_with = my_devs_with` was left in by mistake. The
try/finally used to save and restore the original; without it, this
assignment leaks state and can cause order-dependent failures in other
tests. The @mock.patch decorator already patches the function correctly
and restores it after the test.

Refs canonicalGH-6810
@github-actions
Copy link
Copy Markdown

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging blackboxsw, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag blackboxsw to reopen it.)

@github-actions github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Apr 18, 2026
@blackboxsw blackboxsw removed the stale-pr Pull request is stale; will be auto-closed soon label Apr 21, 2026
@blackboxsw
Copy link
Copy Markdown
Collaborator

Awaiting closure of #6821, then we will work through this set of PRs.

blackboxsw pushed a commit that referenced this pull request Apr 21, 2026
Remove DataSourceOpenNebula and its unit tests from the mypy
check_untyped_defs=false override list and add type annotations
to satisfy stricter checking.

Annotate parse_shell_config, OpenNebulaNetwork.__init__, get_field
(with @overload to distinguish str vs Optional[str] return),
all get_* methods, get_physical_nics_by_mac, and
read_context_disk_dir (fixing its stale docstring).

Remove test_get_field_nonecontext: shell variables cannot hold None
values so the scenario is impossible; test_get_field_emptycontext
covers the realistic equivalent.

Remove leftover bare `util.find_devs_with` assignment in
test_find_candidates, a refactoring artifact from d482353 that
leaked state between tests.

Refs GH-6810
@mcanevet mcanevet force-pushed the feat/opennebula-eth-routes branch from 3d4c3f9 to 7093a80 Compare April 22, 2026 06:16
@mcanevet
Copy link
Copy Markdown
Contributor Author

@blackboxsw rebased

@blackboxsw blackboxsw self-assigned this May 1, 2026
Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Thank you for this work @mcanevet I'd like to see if we can:

  1. improve the handling of whitespace a bit more just in case
  2. use parametrized tests to cut down on boilerplate tests
  3. update a doc link for config context

When we have this is it possible to show the expected network config being represented via sudo cat /var/lib/cloud/instance/network-config.json. or grep Applying network config /var/log/cloud-init.log

Comment thread tests/unittests/sources/test_opennebula.py Outdated
Comment thread doc/rtd/reference/datasources/opennebula.rst
Comment thread cloudinit/sources/DataSourceOpenNebula.py Outdated
Comment thread cloudinit/sources/DataSourceOpenNebula.py Outdated
Add `get_routes()` to `OpenNebulaNetwork` to parse the `ETHx_ROUTES`
context variable (format: "NETWORK via GATEWAY, ...") and emit the
resulting routes into the Netplan v2 `routes:` list in `gen_conf()`.

Malformed entries are skipped with a warning. No `routes` key is emitted
when the variable is absent or empty, preserving backward compatibility.
Copilot AI review requested due to automatic review settings May 4, 2026 06:26
@mcanevet mcanevet force-pushed the feat/opennebula-eth-routes branch from 7093a80 to 8b67644 Compare May 4, 2026 06:26
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the OpenNebula datasource’s network config generation to honor OpenNebula’s ETHx_ROUTES context variable by parsing it and emitting Netplan v2 static routes under each interface’s routes: list, bringing cloud-init behavior in line with OpenNebula’s context-linux support.

Changes:

  • Add OpenNebulaNetwork.get_routes() to parse ETHx_ROUTES ("NETWORK via GATEWAY[, ...]") into Netplan route dictionaries.
  • Update OpenNebulaNetwork.gen_conf() to include a routes key only when parsed routes are present.
  • Add unit tests for parsing and config emission, and document ETH<x>_ROUTES in the OpenNebula datasource reference.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
cloudinit/sources/DataSourceOpenNebula.py Parse ETHx_ROUTES and emit Netplan v2 routes entries per interface.
tests/unittests/sources/test_opennebula.py Add coverage for get_routes() parsing and gen_conf() route emission/absence behavior.
doc/rtd/reference/datasources/opennebula.rst Document ETH<x>_ROUTES format and provide an example; update network configuration link.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mcanevet
Copy link
Copy Markdown
Contributor Author

mcanevet commented May 4, 2026

Here is the full cloud-init.log: https://gist.github.com/mcanevet/7561c97acd64c8ebd980843bc8ca90ee#file-cloud-init-log-L691
Applyed config is:
{'version': 2, 'ethernets': {'eth0': {'match': {'macaddress': '02:00:32:2b:e9:39'}, 'addresses': ['172.23.228.105/24'], 'nameservers': {'addresses': ['172.22.255.30', '172.22.255.31']}, 'mtu': '1500', 'routes': [{'to': '172.16.0.0/12', 'via': '172.23.228.1'}, {'to': '10.0.0.0/8', 'via': '172.23.228.1'}]}}}

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Excellent, thanks for the log references and your continued contributions here @mcanevet

@blackboxsw blackboxsw merged commit e9e10ab into canonical:main May 5, 2026
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation This Pull Request changes documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants