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

Only run udevadm if it exists #36

Merged
merged 2 commits into from
Apr 21, 2022
Merged

Only run udevadm if it exists #36

merged 2 commits into from
Apr 21, 2022

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

Congrats, now it looks exactly like the patch I've been using for some time: https://git.alpinelinux.org/aports/tree/community/cloud-utils/02-make-udev-optional.patch :-)

@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