Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(grub_dpkg): Don't set grub install target by default #5292

Closed
wants to merge 1 commit into from

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented May 14, 2024

fix(grub_dpkg): Don't set grub install target by default

This is no longer the best default behavior for all use cases.
Since some users may be use this we want to keep this feature
available by opt-in.

Reasons:
- This is no longer required by default.
- This has broken some cloud users.
- This behavior can be slow.

Related to LP: #2054103

BREAKING_CHANGE: Change the default value for grub_dpkg.enabled to False.

Note: The impetus of this PR was correctness, but a side-effect is performance. This module sometimes takes absurd amounts of time to run, a recently launched Noble image took ~5s run this module (which is not uncommon). See the logs below.

2024-06-07 17:41:31,205 - modules.py[DEBUG]: Running module grub_dpkg (<module 'cloudinit.config.cc_grub_d
pkg' from '/usr/lib/python3/dist-packages/cloudinit/config/cc_grub_dpkg.py'>) with frequency once-per-inst
ance
2024-06-07 17:41:31,205 - handlers.py[DEBUG]: start: modules-config/config-grub_dpkg: running config-grub_
dpkg with frequency once-per-instance
2024-06-07 17:41:31,205 - util.py[DEBUG]: Writing to /var/lib/cloud/instances/6638723470694742222/sem/config_grub_dpkg - wb: [644] 24 bytes
2024-06-07 17:41:31,206 - helpers.py[DEBUG]: Running config-grub_dpkg using lock (<FileLock using file '/var/lib/cloud/instances/6638723470694742222/sem/config_grub_dpkg'>)
2024-06-07 17:41:31,206 - subp.py[DEBUG]: Running command ['grub-probe', '-t', 'device', '/boot/efi'] with allowed return codes [0] (shell=False, capture=True)
2024-06-07 17:41:31,611 - subp.py[DEBUG]: command ['grub-probe', '-t', 'device', '/boot/efi'] took 0.4s to run
2024-06-07 17:41:31,612 - subp.py[DEBUG]: Running command ['udevadm', 'info', '--root', '--query=symlink', '/dev/sda15'] with allowed return codes [0] (shell=False, capture=True)
2024-06-07 17:41:31,617 - cc_grub_dpkg.py[DEBUG]: considering these device symlinks: /dev/disk/by-id/google-persistent-disk-0-part15,/dev/disk/by-diskseq/9-part15,/dev/disk/by-id/scsi-0Google_PersistentDisk_persistent-disk-0-part15,/dev/disk/by-label/UEFI,/dev/disk/by-partuuid/6f30e8db-9171-4c49-8ced-4f63a2c18d35,/dev/disk/by-path/pci-0000:00:03.0-scsi-0:0:1:0-part15,/dev/disk/by-uuid/A3EC-8A73
2024-06-07 17:41:31,617 - cc_grub_dpkg.py[DEBUG]: filtered to these disk/by-id symlinks: /dev/disk/by-id/google-persistent-disk-0-part15,/dev/disk/by-id/scsi-0Google_PersistentDisk_persistent-disk-0-part15
2024-06-07 17:41:31,617 - cc_grub_dpkg.py[DEBUG]: selected /dev/disk/by-id/google-persistent-disk-0-part15
2024-06-07 17:41:31,617 - cc_grub_dpkg.py[DEBUG]: Setting grub debconf-set-selections with 'grub-pc grub-efi/install_devices string /dev/disk/by-id/google-persistent-disk-0-part15
'
2024-06-07 17:41:31,617 - subp.py[DEBUG]: Running command ['debconf-set-selections'] with allowed return codes [0] (shell=False, capture=True)
2024-06-07 17:41:35,929 - subp.py[DEBUG]: command ['debconf-set-selections'] took 4.3s to run
2024-06-07 17:41:35,930 - handlers.py[DEBUG]: finish: modules-config/config-grub_dpkg: SUCCESS: config-grub_dpkg ran successfully

Related to LP: #2054103

BREAKING_CHANGE: Changes grub_dpkg.enabled to False by default.
@holmanb
Copy link
Member Author

holmanb commented May 14, 2024

fyi @BirknerAlex

@TheRealFalcon
Copy link
Member

We'll want to patch this out of Mantic and below.

@BirknerAlex
Copy link
Contributor

Thanks for the heads up! We will add the flag to vendor data instead.

Copy link
Contributor

@BirknerAlex BirknerAlex left a comment

Choose a reason for hiding this comment

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

LGTM

@TheRealFalcon TheRealFalcon self-assigned this May 21, 2024
Copy link
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.

@kukrimate is aware of a couple of potential failure paths for non-cloud images where this grub config setup is still required and cloud_style_initialization=false. A potential approach we may want to consider is for cloud-init to check debconf settings cloud_sytle_initialization for a True value and maybe on Debian the presence of /etc/grub.d/enable_cloud to determine whether or not to NOOP.

@@ -247,7 +247,7 @@ def test_handle(
"""Test setting of correct debconf database entries"""
m_is_efi_booted.return_value = is_uefi
m_fetch_idevs.return_value = fetch_idevs_output
cfg = {"grub_dpkg": {}}
cfg = {"grub_dpkg": {"enabled": True}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It appears that non-cloud livecd-rootfs images still rely on this cloud-init grub boot traget setup per-boot. This behavior should not land yet until we have sorted expected behavior in those environments.

Copy link
Member Author

Choose a reason for hiding this comment

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

It appears that non-cloud livecd-rootfs images still rely on this cloud-init grub boot traget setup per-boot

Those are images, and therefore should be able to enable this as needed in cloud.cfg.d/.

Copy link
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.

Do not merge yet until an approach that minimally works for images where cloud_style_initialization=false.

@holmanb
Copy link
Member Author

holmanb commented May 28, 2024

@kukrimate is aware of a couple of potential failure paths for non-cloud images

Where is this conversation happening? @blackboxsw @kukrimate can we please continue the conversation in this PR?

non-cloud livecd-rootfs images still rely on this cloud-init grub boot traget setup per-boot

This change should not be incompatible with those needs. They can opt in to using a configuration file in cloud.cfg.d if they still need this behavior.

That said, wouldn't the same mis-detection issue exist there?

A potential approach we may want to consider is for cloud-init to check debconf settings cloud_sytle_initialization for a True value

That could work but I'd still rather make this behavior a default opt-in rather than one that is dynamically decided at runtime. As we've observed, debconf is slow, and dynamic runtime behavior is more complex and error prone.

and maybe on Debian the presence of /etc/grub.d/enable_cloud to determine whether or not to NOOP.

What creates this file? I'm missing some context about the significance of it and why cloud-init would want to use that to decide runtime behavior on debian.

@holmanb
Copy link
Member Author

holmanb commented Jun 12, 2024

@kukrimate is aware of a couple of potential failure paths for non-cloud images

Where is this conversation happening? @blackboxsw @kukrimate can we please continue the conversation in this PR?

It's not externally visible, but to answer my own question it looks like the conversation took place here.

@holmanb
Copy link
Member Author

holmanb commented Jun 13, 2024

Closing for now until we have confirmed that this is a safe change to make.

@holmanb holmanb closed this Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants