feat(hetzner): enable hotplug support and prepare IPv6 integration#6445
Conversation
|
One test is failing, because of the new Attributes to I'm not sure where I would need to make the changes for the test to pass and would be happy if you could point me to the right direction. |
aciba90
left a comment
There was a problem hiding this comment.
Thanks for the feature!
Drive-by comment: The testing issue is that the urls code gets evaluated at module import-time which happens prior to mocking, to fix it something in the lines of:
diff --git a/cloudinit/sources/DataSourceHetzner.py b/cloudinit/sources/DataSourceHetzner.py
index 09a0eb938..c29f03515 100644
--- a/cloudinit/sources/DataSourceHetzner.py
+++ b/cloudinit/sources/DataSourceHetzner.py
@@ -16,10 +16,11 @@ from cloudinit.net.ephemeral import EphemeralIPNetwork
LOG = logging.getLogger(__name__)
-BASE_URLS_V1 = [
- f"http://[fe80::a9fe:a9fe%25{net.find_fallback_nic()}]/hetzner/v1/",
- "http://169.254.169.254/hetzner/v1/",
-]
+def base_urls_v1():
+ return (
+ f"http://[fe80::a9fe:a9fe%25{net.find_fallback_nic()}]/hetzner/v1/",
+ "http://169.254.169.254/hetzner/v1/",
+ )
BUILTIN_DS_CONFIG = {
@@ -85,6 +86,7 @@ class DataSourceHetzner(sources.DataSource):
if not on_hetzner:
return False
+ base_urls = base_urls_v1()
try:
with EphemeralIPNetwork(
self.distro,
@@ -97,13 +99,13 @@ class DataSourceHetzner(sources.DataSource):
url, "metadata/instance-id"
)
}
- for url in BASE_URLS_V1
+ for url in base_urls
],
):
url, contents = hc_helper.get_metadata(
[
url_helper.combine_url(url, self.metadata_path)
- for url in BASE_URLS_V1
+ for url in base_urls
],
max_wait=self.max_wait,
timeout=self.timeout,
@@ -116,7 +118,7 @@ class DataSourceHetzner(sources.DataSource):
url_helper.combine_url(
url, self.metadata_private_networks_path
)
- for url in BASE_URLS_V1
+ for url in base_urls
],
max_wait=self.max_wait,
timeout=self.timeout,
@@ -129,7 +131,7 @@ class DataSourceHetzner(sources.DataSource):
url, ud = hc_helper.get_metadata(
[
url_helper.combine_url(url, self.userdata_path)
- for url in BASE_URLS_V1
+ for url in base_urls
],
max_wait=self.max_wait,
timeout=self.timeout,|
I will peak at the other testing issue later and a full review. |
aciba90
left a comment
There was a problem hiding this comment.
As we are introducing new attributes to the datasource class, we need to introduce un unpickle method to restore them for cached data sources:
diff --git a/cloudinit/sources/DataSourceHetzner.py b/cloudinit/sources/DataSourceHetzner.py
index 09a0eb938..4c2e91549 100644
--- a/cloudinit/sources/DataSourceHetzner.py
+++ b/cloudinit/sources/DataSourceHetzner.py
@@ -79,12 +80,24 @@ class DataSourceHetzner(sources.DataSource):
self.extra_hotplug_udev_rules = EXTRA_HOTPLUG_UDEV_RULES
+ def _unpickle(self, ci_pkl_version: int) -> None:
+ super()._unpickle(ci_pkl_version)
+ self.extra_hotplug_udev_rules = EXTRA_HOTPLUG_UDEV_RULES
+ self.max_wait = self.ds_cfg.get("max_wait", MD_MAX_WAIT)
+ self.metadata_path = self.ds_cfg.get("metadata_path")
+ self.metadata_private_networks_path = self.ds_cfg.get(
+ "metadata_private_networks_path"
+ )
+ self.sleep_time = self.ds_cfg.get("sleep_time", MD_SLEEP_TIME)
+ self.userdata_path = self.ds_cfg.get("userdata_path")
+
def _get_data(self):
(on_hetzner, serial) = get_hcloud_data()|
Which test is your first comment referring to? The
|
The mock in the line:
doesn't get applied because the function is called at import-time.
That's another one, the |
How did you catch that one? 😅 I don't see a failed test for this |
31aa713 to
390ee22
Compare
390ee22 to
f63e84b
Compare
|
@aciba90 Should the docs be updated in this PR too? I noticed, that you state the currently supported DataSources for hotplug support in
|
…ieval, and improve timeout handling
…works, and improve mocking consistency
…zner datasource and tests for consistency and readability # Conflicts: # tests/unittests/sources/test_hetzner.py
cea4152 to
8c7fed5
Compare
|
@aciba90 |
aciba90
left a comment
There was a problem hiding this comment.
The changes look great, thanks for the improvements. I left in-line comments to improve the code, in addition to that could you please:
- Provide logs of a machine that has exercised this code (attaching a NIC to a running instance), if feasible.
- Give more information of what this PR accomplishes in the proposed PR commit message.
The mock in the line:
m_fallback_nic.return_value = "eth0"doesn't get applied because the function is called at import-time.
How did you catch that one? 😅 I don't see a failed test for this
My laptop doesn't have an ethernet connection, so my default nic name was different from eth0. :)
I also wanted to ask if you can see which CLA the job found. My personal or the company CLA?
It looks like your username is detected there, but I not completely sure which one is picked: https://github.com/canonical/cloud-init/actions/runs/17413361821/job/49575901996
|
I have a hard time to install cloud-init to the system, since I merged the main branch, because you changed the build-system in the meantime 😅 . Nonetheless, I manually triggered the hotplug event for now, to generate the log: |
aciba90
left a comment
There was a problem hiding this comment.
LGTM, thanks!
Thanks for attaching logs of a manual run.
I have a hard time to install cloud-init to the system, since I merged the main branch, because you changed the build-system in the meantime 😅 . Maybe you can provide more insight how I could install it now, while testing.
Doesn't ./packages/bddeb -d from https://cloudinit.readthedocs.io/en/latest/development/package_testing.html#package-testing work for you?
|
Unfortunately not. Even when I explicitly use I worked around this by using But this isn't working anymore, because you removed the Should I maybe open an issue regarding the |
|
Thanks for the information. That error is the same as #6141, I think. I believe there is something going on in diff --git a/packages/bddeb b/packages/bddeb
index 4e06378c7..77f81d4c7 100755
--- a/packages/bddeb
+++ b/packages/bddeb
@@ -361,6 +361,7 @@ def main():
"Running 'debuild %s' in %r" % (" ".join(args.debuild_args), xdir)
)
with util.chdir(xdir):
+ import pdb; pdb.set_trace()
cmd = ["debuild", "--preserve-envvar", "INIT_SYSTEM"]
if args.debuild_args:
cmd.extend(args.debuild_args) |
|
@DarkPhily, is there anything else to be done here? Or are good to merge? Thanks. |
|
We are good to go :) PS: I can't merge it myself |
…anonical#6445) This PR tries to accomplish hotplug support for the Hetzner cloud. cloud-init should pick up the newly attached private network and configure the interface accordingly. Furthermore, this PR also prepares cloud-init to use the IPv6 Hetzner metadata-service
…anonical#6445) This PR tries to accomplish hotplug support for the Hetzner cloud. cloud-init should pick up the newly attached private network and configure the interface accordingly. Furthermore, this PR also prepares cloud-init to use the IPv6 Hetzner metadata-service
Proposed Commit Message
Merge type