Skip to content

Only run udevadm if it exists#36

Merged
paride merged 2 commits intocanonical:mainfrom
oittaa:patch-1
Apr 21, 2022
Merged

Only run udevadm if it exists#36
paride merged 2 commits intocanonical:mainfrom
oittaa:patch-1

Conversation

@oittaa
Copy link
Contributor

@oittaa oittaa commented Apr 16, 2022

For example OpenWRT doesn't have udevadm. This tool works perfectly otherwise, but it prints and an error message, when it tries to execute udevadm.

For example OpenWRT doesn't have `udevadm`. This tool works perfectly otherwise, but it prints and an error message, when it tries to execute `udevadm`.
@dermotbradley
Copy link
Contributor

dermotbradley commented Apr 17, 2022

Hi. I was intending to submit a MR to address this and also a similar change to mount-image-callback, both of which I have been using on Alpine Linux for a while, once #35 was merged.

So how do you want to handle this? Go with this MR and I raise a MR for mount-image-callback? Or you close this and I submit a single MR for both growpart and mount-image-callback?

BTW I also have another MR yet to submit for patches (that again I've been using for months with Alpine Linux) for Busybox compatibility in cloud-localds, mount-image-callback, and resize-part-image.

@oittaa
Copy link
Contributor Author

oittaa commented Apr 17, 2022

Well, to me it's fine either way. But since this pull request is already here, maybe you could just make a pull request for mount-image-callback?

@dermotbradley
Copy link
Contributor

But since this pull request is already here, maybe you could just make a pull request for mount-image-callback?

Ok, then I'd suggest you modify the comment on line 176 to add "(if installed)" after "run udevadm settle".

@dermotbradley
Copy link
Contributor

dermotbradley commented Apr 17, 2022

@paride
Copy link
Collaborator

paride commented Apr 18, 2022

Hi @oittaa and thanks for this PR. The change itself LGTM, but I wonder how safe it is. It's probably safe to assume that when udevadm is not available the system is not using udev, so we don't need to settle it, and we leave the burden of making sure that the kernel events have been processed to whatever tooling is calling growpart on non-udev/non-systemd systems. Is my understanding correct here?

It would be nice to run CI on Alpine, but that's not supported by GitHub Actions (https://github.com/actions/virtual-environments).

@oittaa
Copy link
Contributor Author

oittaa commented Apr 18, 2022

Is my understanding correct here?

Correct.

It would be nice to run CI on Alpine, but that's not supported by GitHub Actions (https://github.com/actions/virtual-environments).

A workaround could be to run in an Alpine Linux docker container?

@dermotbradley
Copy link
Contributor

dermotbradley commented Apr 18, 2022

I wonder how safe it is. It's probably safe to assume that when udevadm is not available the system is not using udev, so we don't need to settle it, and we leave the burden of making sure that the kernel events have been processed to whatever tooling is calling growpart on non-udev/non-systemd systems. Is my understanding correct here?

Hi. I applied basically the same change to the Alpine packaged cloud-utils (which I'm the maintainer of) back in Nov 2020.

I have tested Alpine VMs using mdev, rather than eudev/udev, with such a patched growpart via a 1st-time boot init.d script and have not encountered any issues.

Copy link
Collaborator

@paride paride left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks!

@paride paride merged commit 79bb715 into canonical:main Apr 21, 2022
@oittaa oittaa deleted the patch-1 branch April 21, 2022 19:33
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.

3 participants