fix serial datasource definition for nocloud #4949

holmanb · 2024-02-23T21:19:09Z

Proposed Commit Message

fix(nocloud): NoCloud serial datasource definition is broken

Fix it.
Deprecate the "nocloud-net" name.

Additional Context

#4945
#4948

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>)

TheRealFalcon

Changes LGTM. Can we get tests?

TheRealFalcon · 2024-02-23T21:25:01Z

cloudinit/sources/__init__.py

@@ -366,7 +366,8 @@ def override_ds_detect(self) -> bool:
        does not run, _something_ needs to detect the kernel command line
        definition.
        """
-        if self.dsname.lower() == parse_cmdline().lower():
+        dsname = self.dsname.lower()


Nit: This change isn't needed anymore (but also doesn't hurt anything)

holmanb · 2024-02-23T23:37:37Z

Changes LGTM. Can we get tests?

Good call.

FYI - The qemu tutorial is currently broken by this regression and could be used to manually test with a modified image (mount it locally, chroot into it, install a test cloud-init deb).

I assume that using qemu via pycloudlib would be best so we can set the bios serial, but I haven't tried it out yet, and I don't think that we have any Jenkins runners set up to use it yet, right?

@TheRealFalcon Is that what you were thinking of or did you have something else in mind?

TheRealFalcon · 2024-02-23T23:52:10Z

I was really only thinking about unit tests.

I was also thinking we would need QEMU, but it looks like LXD has recently added support for something like this:

$ lxc launch ubuntu:jammy jammy-test --vm -c raw.qemu="-smbios type=1,serial=HITHERE"
Creating jammy-test
Starting jammy-test
$ lxc exec jammy-test -- cat /sys/class/dmi/id/product_serial              
HITHERE

holmanb · 2024-02-24T00:27:23Z

I was really only thinking about unit tests.

Gotcha

I was also thinking we would need QEMU, but it looks like LXD has recently added support for something like this:

Oh sweet.

rvandegrift · 2024-02-24T06:38:37Z

cloudinit/sources/DataSourceNoCloud.py

@@ -362,18 +363,50 @@ def _merge_new_seed(cur, seeded):
    return ret


+def _parse_dmi() -> str:
+    serial = dmi.read_dmi_data("product_serial")


s/product_serial/system-serial-number/ I think, otherwise this always fails

@rvandegrift It looks like applying this actually broke the test case, what were you seeing that led to this suggestion?

the issued wasn't fixed, at least with ds=nocloud (not 100% sure I tested nocloud-net at that point). From a quick grep, it looked like read_dmi_data wanted keys from dmi.DMIDECODE_TO_KERNEL - see

cloud-init/cloudinit/dmi.py

Line 84 in beddd97

kmap = DMIDECODE_TO_KERNEL.get(key)

Could be I was wrong - though when I made that change, your patch started working for the ds=nocloud case. I didn't run the tests, just built a deb and tried it with a qmeu + fake imds setup.

@holmanb , I think system-serial-number is "more right" here. We use the map @rvandegrift mentioned to read the DMI value from /sys. If that fails, we fallback to reading using dmidecode. It looks like specifying product_serial as a key will trigger the fallback code, but we should be using /sys if it's available.

broke the test case

Which test case? Mine will be broken unless you apply blackboxsw's suggested fix 😬

rvandegrift · 2024-02-24T06:48:45Z

cloudinit/sources/DataSourceNoCloud.py

+            return True
+
+        serial = _parse_dmi().lower()
+        if serial in (self.dsname, "nocloud-net"):


I think s/self.dsname/self.dsname.lower()/ is needed to fix the ds=nocloud case

rvandegrift · 2024-02-24T06:49:24Z

@holmanb with the above changes, this is working!

holmanb · 2024-02-26T16:11:20Z

@holmanb with the above changes, this is working!

Thanks for the review! I just pushed the requested changes.

TheRealFalcon · 2024-02-26T17:18:07Z

@holmanb

I think something like this could work as a basic integration test. It should be hard to modify for http roots. Also, I did attempt to do the file pushing in the setup code, but LXD VMs need to be started before you can do that.

SMBIOS_USERDATA = """\
#cloud-config
runcmd:
  - touch /var/tmp/smbios_test_file
"""
SMBIOS_SEED_DIR = "/smbios_seed"


def setup_nocloud_serial(instance: LXDInstance):
    subp(
        [
            "lxc",
            "config",
            "set",
            instance.name,
            "raw.qemu=-smbios "
            f"type=1,serial=ds=nocloud;s=file://{SMBIOS_SEED_DIR};h=myhost",
        ]
    )


@pytest.mark.lxd_setup.with_args(setup_nocloud_serial)
@pytest.mark.lxd_use_exec
@pytest.mark.skipif(
    PLATFORM != "lxd_vm",
    reason="Requires NoCloud with raw QEMU serial setup",
)
def test_smbios_seed(client: IntegrationInstance):
    client.execute(f"mkdir {SMBIOS_SEED_DIR}")
    client.write_to_file(f"{SMBIOS_SEED_DIR}/user-data", SMBIOS_USERDATA)
    client.write_to_file(f"{SMBIOS_SEED_DIR }/smbios_seed/meta-data", "")
    client.execute("cloud-init clean --logs")
    client.restart()
    assert client.execute("test -f /var/tmp/smbios_test_file").ok

blackboxsw · 2024-02-26T19:02:56Z

tests/integration_tests/datasources/test_nocloud.py

+def test_smbios_seed(client: IntegrationInstance):
+    client.execute(f"mkdir {SMBIOS_SEED_DIR}")
+    client.write_to_file(f"{SMBIOS_SEED_DIR}/user-data", SMBIOS_USERDATA)
+    client.write_to_file(f"{SMBIOS_SEED_DIR }/smbios_seed/meta-data", "")


This integration test fails due to the path typo duplicating SMBIOS_SEED_DIR/smbois_seed

Suggested change

client.write_to_file(f"{SMBIOS_SEED_DIR }/smbios_seed/meta-data", "")

client.write_to_file(f"{SMBIOS_SEED_DIR}/meta-data", "")

blackboxsw

Minor test fix then approve

TheRealFalcon · 2024-02-26T19:02:26Z

cloudinit/sources/DataSourceNoCloud.py

@@ -362,18 +363,50 @@ def _merge_new_seed(cur, seeded):
    return ret


+def _parse_dmi() -> str:
+    serial = dmi.read_dmi_data("product_serial")


@holmanb , I think system-serial-number is "more right" here. We use the map @rvandegrift mentioned to read the DMI value from /sys. If that fails, we fallback to reading using dmidecode. It looks like specifying product_serial as a key will trigger the fallback code, but we should be using /sys if it's available.

broke the test case

Which test case? Mine will be broken unless you apply blackboxsw's suggested fix 😬

cloudinit/sources/DataSourceNoCloud.py

TheRealFalcon · 2024-02-27T00:16:45Z

There's an extra .swp file that got added, but otherwise LGTM! I'm still running through the test in various scenarios.

blackboxsw

Since we are spending an integration test on NoCloudNet let's also double-dip and get a check on the deprecation logic too.

Also we need to pass the value we pull out of system-serial-number /sys/class/dmi/id/product_serial through our new parse_cmdline_or_dmi otherwise ds_detect is trying to find a full ds=nocloud-net;s=http://blah in (nocloud, nocloud-net) which never matches.

--- a/cloudinit/sources/DataSourceNoCloud.py
+++ b/cloudinit/sources/DataSourceNoCloud.py
@@ -389,7 +389,9 @@ class DataSourceNoCloudNet(DataSourceNoCloud):
             log_deprecated()
             return True
 
-        serial = (dmi.read_dmi_data("product_serial") or "").lower()
+        serial = sources.parse_cmdline_or_dmi(
+            dmi.read_dmi_data("system-serial-number") or ""
+        ).lower()
         if serial in (self.dsname.lower(), "nocloud-net"):
             LOG.debug(
                 "Machine is configured by dmi serial number to run on "
diff --git a/tests/integration_tests/datasources/.test_nocloud.py.swp b/tests/integration_tests/datasources/.test_nocloud.py.swp
deleted file mode 100644
index 97bda3f6b..000000000
Binary files a/tests/integration_tests/datasources/.test_nocloud.py.swp and /dev/null differ
diff --git a/tests/integration_tests/datasources/test_nocloud.py b/tests/integration_tests/datasources/test_nocloud.py
index 2e5ea458f..9ab7ecd5b 100644
--- a/tests/integration_tests/datasources/test_nocloud.py
+++ b/tests/integration_tests/datasources/test_nocloud.py
@@ -121,7 +121,7 @@ def setup_nocloud_network_serial(instance: LXDInstance):
             "set",
             instance.name,
             "raw.qemu=-smbios "
-            "type=1,serial=ds=nocloud;s=http://0.0.0.0/;h=myhost",
+            "type=1,serial=ds=nocloud-net;s=http://0.0.0.0/;h=myhost",
         ]
     )
 
@@ -185,4 +185,7 @@ class TestSmbios:
         client.write_to_file(f"{SMBIOS_SEED_DIR}/vendor-data", "")
         assert client.execute("cloud-init clean --logs").ok
         client.restart()
+        assert "'nocloud-net' datasource name is deprecated" in client.execute(
+            "cloud-init status --format json"
+        )
         assert client.execute("test -f /var/tmp/smbios_test_file").ok

blackboxsw · 2024-02-26T23:25:13Z

tests/integration_tests/datasources/test_nocloud.py

+        assert client.execute("systemctl enable local-server.service").ok
+        client.write_to_file(
+            "/etc/cloud/cloud.cfg.d/91_do_not_use_lxd.cfg",
+            "datasource_list: [ NoCloud, None ]\n",


We can even drop this to [ NoCloud ]

If we do, we won't exercise the ds_detect logic

blackboxsw · 2024-02-26T23:26:02Z

tests/integration_tests/datasources/test_nocloud.py

+            "set",
+            instance.name,
+            "raw.qemu=-smbios "
+            "type=1,serial=ds=nocloud;s=http://0.0.0.0/;h=myhost",


Can we also suggest the deprecated nocloud-net so we can also validate the deprecation warning?

Suggested change

"type=1,serial=ds=nocloud;s=http://0.0.0.0/;h=myhost",

"type=1,serial=ds=nocloud-net;s=http://0.0.0.0/;h=myhost",

blackboxsw · 2024-02-26T23:34:52Z

cloudinit/sources/DataSourceNoCloud.py

+            log_deprecated()
+            return True
+
+        serial = (dmi.read_dmi_data("product_serial") or "").lower()


This really should be systemd-serial-number to align with the data we are pulling in DataSourceNoCloud._get_data. Otherwise we fall-through to dmidecode on Ubuntu which emits the following error in logs

2024-02-26 23:26:42,721 - dmi.py[DEBUG]: failed dmidecode cmd: ['/usr/sbin/dmidecode', '--string', 'product_serial'] Unexpected error while running command. Command: ['/usr/sbin/dmidecode', '--string', 'product_serial'] Exit code: 2 Reason: - Stdout: Stderr: Invalid string keyword: product_serial Valid string keywords are: bios-vendor bios-version bios-release-date bios-revision firmware-revision system-manufacturer system-product-name system-version system-serial-number

This causes us to fail to detect nocloud datasource and fallback to datasourcenone.

- Verify that NoCloud smbios seed gets read for file:// - Verify that NoCloud smbios seed gets read from http:// and https://

deprecate nocloud-net name

blackboxsw

LGTM! tests pass locally here too.

James' comments are now resolved and final pass is now functional.

TheRealFalcon added the 24.1 label Feb 23, 2024

TheRealFalcon reviewed Feb 23, 2024

View reviewed changes

holmanb added the security label Feb 24, 2024

rvandegrift reviewed Feb 24, 2024

View reviewed changes

holmanb mentioned this pull request Feb 26, 2024

nocloud-net not working in cloud-init >=23.4 #4945

Closed

holmanb removed the security label Feb 26, 2024

holmanb force-pushed the holmanb/fix-serial-nocloud branch from 3d07322 to 264ce88 Compare February 26, 2024 18:53

blackboxsw reviewed Feb 26, 2024

View reviewed changes

blackboxsw requested changes Feb 26, 2024

View reviewed changes

TheRealFalcon previously requested changes Feb 26, 2024

View reviewed changes

TheRealFalcon reviewed Feb 26, 2024

View reviewed changes

cloudinit/sources/DataSourceNoCloud.py Show resolved Hide resolved

holmanb force-pushed the holmanb/fix-serial-nocloud branch 3 times, most recently from 5f29713 to 6cc8de1 Compare February 26, 2024 22:56

blackboxsw requested changes Feb 27, 2024

View reviewed changes

holmanb force-pushed the holmanb/fix-serial-nocloud branch 3 times, most recently from 9c472cf to e2fc0b8 Compare February 27, 2024 00:56

holmanb added 2 commits February 26, 2024 18:07

tests: Check that smbios seed works

17816b2

- Verify that NoCloud smbios seed gets read for file:// - Verify that NoCloud smbios seed gets read from http:// and https://

fix(nocloud): smbios datasource definition

5012d4d

deprecate nocloud-net name

holmanb force-pushed the holmanb/fix-serial-nocloud branch from e2fc0b8 to 5012d4d Compare February 27, 2024 01:07

blackboxsw approved these changes Feb 27, 2024

View reviewed changes

holmanb merged commit 66b5ce9 into canonical:main Feb 27, 2024
29 checks passed

holmanb mentioned this pull request Feb 28, 2024

[docs]: qemu start command doesn't work on MacOS #4948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix serial datasource definition for nocloud #4949

fix serial datasource definition for nocloud #4949

holmanb commented Feb 23, 2024 •

edited

TheRealFalcon left a comment

TheRealFalcon Feb 23, 2024

holmanb commented Feb 23, 2024

TheRealFalcon commented Feb 23, 2024

holmanb commented Feb 24, 2024

rvandegrift Feb 24, 2024

holmanb Feb 26, 2024

rvandegrift Feb 26, 2024

TheRealFalcon Feb 26, 2024 •

edited

rvandegrift Feb 24, 2024

rvandegrift commented Feb 24, 2024

holmanb commented Feb 26, 2024

TheRealFalcon commented Feb 26, 2024

blackboxsw Feb 26, 2024

blackboxsw left a comment

TheRealFalcon Feb 26, 2024 •

edited

TheRealFalcon commented Feb 27, 2024 •

edited

blackboxsw left a comment

blackboxsw Feb 26, 2024

TheRealFalcon Feb 27, 2024

blackboxsw Feb 26, 2024

blackboxsw Feb 26, 2024 •

edited

blackboxsw left a comment

	client.write_to_file(f"{SMBIOS_SEED_DIR }/smbios_seed/meta-data", "")
	client.write_to_file(f"{SMBIOS_SEED_DIR}/meta-data", "")

	"type=1,serial=ds=nocloud;s=http://0.0.0.0/;h=myhost",
	"type=1,serial=ds=nocloud-net;s=http://0.0.0.0/;h=myhost",

fix serial datasource definition for nocloud #4949

fix serial datasource definition for nocloud #4949

Conversation

holmanb commented Feb 23, 2024 • edited

Proposed Commit Message

Additional Context

Merge type

TheRealFalcon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holmanb commented Feb 23, 2024

TheRealFalcon commented Feb 23, 2024

holmanb commented Feb 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealFalcon Feb 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvandegrift commented Feb 24, 2024

holmanb commented Feb 26, 2024

TheRealFalcon commented Feb 26, 2024

Choose a reason for hiding this comment

blackboxsw left a comment

Choose a reason for hiding this comment

TheRealFalcon Feb 26, 2024 • edited

Choose a reason for hiding this comment

TheRealFalcon commented Feb 27, 2024 • edited

blackboxsw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackboxsw Feb 26, 2024 • edited

Choose a reason for hiding this comment

blackboxsw left a comment

Choose a reason for hiding this comment

holmanb commented Feb 23, 2024 •

edited

TheRealFalcon Feb 26, 2024 •

edited

TheRealFalcon Feb 26, 2024 •

edited

TheRealFalcon commented Feb 27, 2024 •

edited

blackboxsw Feb 26, 2024 •

edited