feat(ds-identify): Don't run unnecessary systemd-detect-virt by holmanb · Pull Request #4633 · canonical/cloud-init

holmanb · 2023-11-28T14:04:14Z

Additional Context

feat(ds-identify): Don't run unnecessary systemd-detect-virt

Running systemd-detect-virt is expensive and, on systemd >=251,
it is unnecessary.

The environment variable SYSTEMD_VIRTUALIZATION contains the
virtualization type that we currently manually get from
systemd-detect-virt.

https://www.freedesktop.org/software/systemd/man/latest/systemd.generator.html#Environment

Test Steps

shell stuff

$ /bin/sh -c 'VALUE="container:lxc";echo ${VALUE#*:}'
lxc

this branch: focal (systemd 245)

# cat /run/cloud-init/ds-identify.log 
[up 0.50s] ds-identify 
policy loaded: mode=search report=false found=all maybe=all notfound=disabled
/etc/cloud/cloud.cfg.d/90_dpkg.cfg set datasource_list: [ NoCloud, ConfigDrive, OpenNebula, DigitalOcean, Azure, AltCloud, OVF, MAAS, GCE, OpenStack, CloudSigma, SmartOS, Bigstep, Scaleway, AliYun, Ec2, CloudStack, Hetzner, IBMCloud, Oracle, Exoscale, RbxCloud, UpCloud, VMware, Vultr, LXD, NWCS, Akamai, None ]
DMI_PRODUCT_NAME=MS-7B79
DMI_SYS_VENDOR=Micro-Star International Co., Ltd.
DMI_PRODUCT_SERIAL=unavailable
DMI_PRODUCT_UUID=unavailable
PID_1_PRODUCT_NAME=unavailable
DMI_CHASSIS_ASSET_TAG=To be filled by O.E.M.
DMI_BOARD_NAME=X470 GAMING PLUS (MS-7B79)
FS_LABELS=unavailable:container
ISO9660_DEVS=unavailable:container
KERNEL_CMDLINE=/sbin/init 
VIRT=lxc
UNAME_KERNEL_NAME=Linux
UNAME_KERNEL_RELEASE=6.2.0-37-generic
UNAME_KERNEL_VERSION=#38-Ubuntu SMP PREEMPT_DYNAMIC Mon Oct 30 21:04:52 UTC 2023
UNAME_MACHINE=x86_64
UNAME_NODENAME=me
UNAME_OPERATING_SYSTEM=GNU/Linux
DSNAME=
DSLIST=NoCloud ConfigDrive OpenNebula DigitalOcean Azure AltCloud OVF MAAS GCE OpenStack CloudSigma SmartOS Bigstep Scaleway AliYun Ec2 CloudStack Hetzner IBMCloud Oracle Exoscale RbxCloud UpCloud VMware Vultr LXD NWCS Akamai None
MODE=search
ON_FOUND=all
ON_MAYBE=all
ON_NOTFOUND=disabled
pid=47 ppid=27
is_container=true
check for 'NoCloud' returned found
is_ds_enabled(IBMCloud) = true.
is_ds_enabled(IBMCloud) = true.
Found single datasource: NoCloud
[up 0.53s] returning 0

this branch: mantic (systemd 253)

# cat /run/cloud-init/ds-identify.log 
[up 0.43s] ds-identify 
policy loaded: mode=search report=false found=all maybe=all notfound=disabled
/etc/cloud/cloud.cfg.d/90_dpkg.cfg set datasource_list: [ NoCloud, ConfigDrive, OpenNebula, DigitalOcean, Azure, AltCloud, OVF, MAAS, GCE, OpenStack, CloudSigma, SmartOS, Bigstep, Scaleway, AliYun, Ec2, CloudStack, Hetzner, IBMCloud, Oracle, Exoscale, RbxCloud, UpCloud, VMware, Vultr, LXD, NWCS, Akamai, None ]
DMI_PRODUCT_NAME=MS-7B79
DMI_SYS_VENDOR=Micro-Star International Co., Ltd.
DMI_PRODUCT_SERIAL=unavailable
DMI_PRODUCT_UUID=unavailable
PID_1_PRODUCT_NAME=unavailable
DMI_CHASSIS_ASSET_TAG=To be filled by O.E.M.
DMI_BOARD_NAME=X470 GAMING PLUS (MS-7B79)
FS_LABELS=unavailable:container
ISO9660_DEVS=unavailable:container
KERNEL_CMDLINE=/sbin/init 
VIRT=lxc
UNAME_KERNEL_NAME=Linux
UNAME_KERNEL_RELEASE=6.2.0-37-generic
UNAME_KERNEL_VERSION=#38-Ubuntu SMP PREEMPT_DYNAMIC Mon Oct 30 21:04:52 UTC 2023
UNAME_MACHINE=x86_64
UNAME_NODENAME=me
UNAME_OPERATING_SYSTEM=GNU/Linux
DSNAME=
DSLIST=NoCloud ConfigDrive OpenNebula DigitalOcean Azure AltCloud OVF MAAS GCE OpenStack CloudSigma SmartOS Bigstep Scaleway AliYun Ec2 CloudStack Hetzner IBMCloud Oracle Exoscale RbxCloud UpCloud VMware Vultr LXD NWCS Akamai None
MODE=search
ON_FOUND=all
ON_MAYBE=all
ON_NOTFOUND=disabled
pid=47 ppid=29
is_container=true
is_ds_enabled(IBMCloud) = true.
is_ds_enabled(IBMCloud) = true.
No ds found [mode=search, notfound=disabled]. Disabled cloud-init [1]
[up 0.47s] returning 1

compared to main branch: mantic (systemd 253)

# cat /run/cloud-init/ds-identify.log 
[up 0.48s] ds-identify 
policy loaded: mode=search report=false found=all maybe=all notfound=disabled
/etc/cloud/cloud.cfg.d/90_dpkg.cfg set datasource_list: [ NoCloud, ConfigDrive, OpenNebula, DigitalOcean, Azure, AltCloud, OVF, MAAS, GCE, OpenStack, CloudSigma, SmartOS, Bigstep, Scaleway, AliYun, Ec2, CloudStack, Hetzner, IBMCloud, Oracle, Exoscale, RbxCloud, UpCloud, VMware, Vultr, LXD, NWCS, Akamai, None ]
DMI_PRODUCT_NAME=MS-7B79
DMI_SYS_VENDOR=Micro-Star International Co., Ltd.
DMI_PRODUCT_SERIAL=unavailable
DMI_PRODUCT_UUID=unavailable
PID_1_PRODUCT_NAME=unavailable
DMI_CHASSIS_ASSET_TAG=To be filled by O.E.M.
DMI_BOARD_NAME=X470 GAMING PLUS (MS-7B79)
FS_LABELS=unavailable:container
ISO9660_DEVS=unavailable:container
KERNEL_CMDLINE=/sbin/init 
VIRT=lxc
UNAME_KERNEL_NAME=Linux
UNAME_KERNEL_RELEASE=6.2.0-37-generic
UNAME_KERNEL_VERSION=#38-Ubuntu SMP PREEMPT_DYNAMIC Mon Oct 30 21:04:52 UTC 2023
UNAME_MACHINE=x86_64
UNAME_NODENAME=me
UNAME_OPERATING_SYSTEM=GNU/Linux
DSNAME=
DSLIST=NoCloud ConfigDrive OpenNebula DigitalOcean Azure AltCloud OVF MAAS GCE OpenStack CloudSigma SmartOS Bigstep Scaleway AliYun Ec2 CloudStack Hetzner IBMCloud Oracle Exoscale RbxCloud UpCloud VMware Vultr LXD NWCS Akamai None
MODE=search
ON_FOUND=all
ON_MAYBE=all
ON_NOTFOUND=disabled
pid=46 ppid=29
is_container=true
is_ds_enabled(IBMCloud) = true.
is_ds_enabled(IBMCloud) = true.
No ds found [mode=search, notfound=disabled]. Disabled cloud-init [1]
[up 0.52s] returning 1

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

Nice! I left an inline comment.

Additionally, I think you're mixing tabs and spaces, but I didn't check out the code, so it could just be rendering weird.

Also, unit tests please.

holmanb · 2023-11-28T19:31:07Z

            fp.write("\n".join(head + mocklines + endlines) + "\n")

+        detect_virt = None
+        for mock in [*mocks, *default_mocks]:


aside:

a dict of dicts would have been the perfect data structure for a collection of unique mocks, but instead this test uses a list of dicts so here we are iterating over the list (like a bunch of other places in the code). I'm not going to refactor this giant test suite, so for now I just complain <rant/>

holmanb · 2023-11-28T19:35:40Z

Nice! I left an inline comment.

Thanks for the review - addressed!

Additionally, I think you're mixing tabs and spaces, but I didn't check out the code, so it could just be rendering weird.

Oops, fixed.

Also, unit tests please.

It was annoying to write, since this function was previously mocked, but I got us some test coverage by un-mocking in cases that systemd-detect-virt won't get called.

TheRealFalcon

LGTM. I re-kicked the integration test job as it appeared to fail for unrelated reasons.

github-actions · 2024-01-04T00:21:16Z

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 TheRealFalcon, 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 TheRealFalcon to reopen it.)

TheRealFalcon · 2024-01-25T17:29:27Z

I noticed some more recent activity. Is this ready for review now?

holmanb · 2024-01-26T03:30:37Z

I noticed some more recent activity. Is this ready for review now?

Not quite. Integration tests fail and I was having trouble reproducing failure locally so I just rebased on other ds-identify changes to keep it current with plans to debug it later.

github-actions · 2024-02-10T00:19:19Z

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 TheRealFalcon, 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 TheRealFalcon to reopen it.)

github-actions · 2024-03-04T00:20:39Z

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 TheRealFalcon, 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 TheRealFalcon to reopen it.)

Running systemd-detect-virt is expensive and, on systemd >=251, unnecessary. The environment variable SYSTEMD_VIRTUALIZATION contains the virtualization type that we currently manually get from systemd-detect-virt. Update tests to exercise this code path. https://www.freedesktop.org/software/systemd/man/latest/systemd.generator.html#Environment

holmanb · 2024-03-05T21:59:13Z

@TheRealFalcon this one is ready for review now

TheRealFalcon

LGTM! I don't envy the work you had to do on test_ds_identify.

…al#4633) Running systemd-detect-virt is expensive and on systemd >=251 it is unnecessary. The environment variable SYSTEMD_VIRTUALIZATION contains the virtualization type that we currently manually get from systemd-detect-virt[1]. Also fix a minor FreeBSD virtualization bug. [1] https://www.freedesktop.org/software/systemd/man/latest/systemd.generator.html#Environment

TheRealFalcon suggested changes Nov 28, 2023

View reviewed changes

Comment thread tools/ds-identify Outdated

holmanb force-pushed the holmanb/more-generator-improvements branch 2 times, most recently from a371e6e to 4a57716 Compare November 28, 2023 19:26

TheRealFalcon self-assigned this Nov 28, 2023

holmanb commented Nov 28, 2023

View reviewed changes

holmanb requested a review from TheRealFalcon November 28, 2023 19:34

holmanb added the wip Work in progress, do not land label Nov 30, 2023

TheRealFalcon approved these changes Dec 1, 2023

View reviewed changes

github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 4, 2024

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Jan 5, 2024

holmanb force-pushed the holmanb/more-generator-improvements branch 2 times, most recently from 21d8979 to 45d8633 Compare January 8, 2024 20:01

holmanb removed the wip Work in progress, do not land label Jan 22, 2024

holmanb force-pushed the holmanb/more-generator-improvements branch from 45d8633 to 5c0ff57 Compare January 23, 2024 23:12

github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Feb 10, 2024

github-actions Bot closed this Feb 17, 2024

holmanb reopened this Feb 18, 2024

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Feb 18, 2024

github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Mar 4, 2024

holmanb force-pushed the holmanb/more-generator-improvements branch from 5c0ff57 to c44d938 Compare March 5, 2024 16:04

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Mar 5, 2024

holmanb added 2 commits March 5, 2024 12:31

fix ds-identify

0379093

fmt

4695074

holmanb added 2 commits March 5, 2024 14:14

cleanup tests

c4862a4

cleanup

7667ecb

holmanb requested a review from TheRealFalcon March 5, 2024 21:58

TheRealFalcon approved these changes Mar 6, 2024

View reviewed changes

Comment thread tools/ds-identify

holmanb merged commit 69da05c into canonical:main Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ds-identify): Don't run unnecessary systemd-detect-virt#4633

feat(ds-identify): Don't run unnecessary systemd-detect-virt#4633
holmanb merged 5 commits into
canonical:mainfrom
holmanb:holmanb/more-generator-improvements

holmanb commented Nov 28, 2023 •

edited

Loading

Uh oh!

TheRealFalcon left a comment •

edited

Loading

Uh oh!

Uh oh!

holmanb Nov 28, 2023 •

edited

Loading

Uh oh!

holmanb commented Nov 28, 2023

Uh oh!

TheRealFalcon left a comment

Uh oh!

github-actions Bot commented Jan 4, 2024

Uh oh!

TheRealFalcon commented Jan 25, 2024

Uh oh!

holmanb commented Jan 26, 2024

Uh oh!

github-actions Bot commented Feb 10, 2024

Uh oh!

github-actions Bot commented Mar 4, 2024

Uh oh!

holmanb commented Mar 5, 2024

Uh oh!

TheRealFalcon left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

holmanb commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional Context

Test Steps

shell stuff

this branch: focal (systemd 245)

this branch: mantic (systemd 253)

compared to main branch: mantic (systemd 253)

Merge type

Uh oh!

TheRealFalcon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

holmanb Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holmanb commented Nov 28, 2023

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jan 4, 2024

Uh oh!

TheRealFalcon commented Jan 25, 2024

Uh oh!

holmanb commented Jan 26, 2024

Uh oh!

github-actions Bot commented Feb 10, 2024

Uh oh!

github-actions Bot commented Mar 4, 2024

Uh oh!

holmanb commented Mar 5, 2024

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

holmanb commented Nov 28, 2023 •

edited

Loading

TheRealFalcon left a comment •

edited

Loading

holmanb Nov 28, 2023 •

edited

Loading