[rhcos-4.13] overlay.d: create new 30gcp-udev-rules overlay #2336

ravanelli · 2023-03-29T19:53:06Z

This PR injects/updates the udev rules from google-guest-config package into our overlay.
It works as an workaround for the BZ #2176212. The longer term solution is to add these packages as part of the Fedora/RHEL itself, so we won't need to maintain it, as we can entirely drop it.

For more info:
https://github.com/GoogleCloudPlatform/guest-configs/tree/master/src/lib/udev

dustymabe · 2023-03-29T21:30:47Z

A few things:

Let's rename the commit to not mention the bug but rather say what we are doing. We can mention the bug in the commit message.
Do you mind adding a first commit that breaks out the existing udev rule into it's own module (i.e. look at 25azure-udev-rules). This helps us in the future evaluate our "layers" and work to eliminate them over time.
In the commit message of the second commit can you mention what commit hash you used when you synced over these 3 files from https://github.com/GoogleCloudPlatform/guest-configs ? I think it will be useful for historical purposes. We could/should also link to https://bugzilla.redhat.com/show_bug.cgi?id=2182865 as an opportunity for us to drop this when the new subpackage exists that we can pull in.

jlebon · 2023-03-30T14:17:52Z

* In the commit message of the second commit can you mention what commit hash you used when you synced over these 3 files from [GoogleCloudPlatform/guest-configs](https://github.com/GoogleCloudPlatform/guest-configs) ?

I would go further and include that information directly as comments at the top of those files.

dustymabe · 2023-03-30T14:29:35Z

I would go further and include that information directly as comments at the top of those files.

I wouldn't - since the files are currently diff-able with what is upstream, but I would include it in the README for the module. i.e. look at what we did for azure: https://github.com/coreos/fedora-coreos-config/blob/testing-devel/overlay.d/README.md#25-azure-udev-rules

jlebon · 2023-03-30T14:55:52Z

I would go further and include that information directly as comments at the top of those files.

I wouldn't - since the files are currently diff-able with what is upstream, but I would include it in the README for the module. i.e. look at what we did for azure: testing-devel/overlay.d/README.md#25-azure-udev-rules

Personally, when copy/pasting stuff like this directly from other places, I want to be really clear that happened and so document it at the top of the file. It doesn't have to be the full context; just a "Copied from $permalink" is enough. Having that provenance be inline is helpful not only to us but in this case also to users perusing those files on the host too.

dustymabe · 2023-03-30T15:07:23Z

Personally, when copy/pasting stuff like this directly from other places, I want to be really clear that happened and so document it at the top of the file. It doesn't have to be the full context; just a "Copied from $permalink" is enough. Having that provenance be inline is helpful not only to us but in this case also to users perusing those files on the host too.

I think that's fair. Maybe we can do both?

dustymabe · 2023-03-30T15:49:17Z

Nice commit messages @ravanelli!

I think we need to still update the README and also add a line at the top of the copied files mentioning where exactly they were copied from (permalink to github file would be nice).

ravanelli · 2023-03-30T15:51:35Z

Nice commit messages @ravanelli!

I think we need to still update the README and also add a line at the top of the copied files mentioning where exactly they were copied from (permalink to github file would be nice).

Done =)

ravanelli · 2023-03-30T16:16:41Z

The CI still failing, seems we need a backport to rhcos-413 from: b0ffe7c

dustymabe · 2023-03-30T17:23:55Z

overlay.d/30gcp-udev-rules/usr/lib/udev/google_nvme_id

+# ATTENTION: It is a copy from https://github.com/GoogleCloudPlatform/guest-configs/blob/master/src/lib/udev/google_nvme_id
+# This file is synced with the source using the last update: Latest commit 3841945 on Feb 17 2023


For these let's move this directly under the #!/bin/bash above and also just use the permalink which is self explanatory:

Suggested change

# ATTENTION: It is a copy from https://github.com/GoogleCloudPlatform/guest-configs/blob/master/src/lib/udev/google_nvme_id

# This file is synced with the source using the last update: Latest commit 3841945 on Feb 17 2023

# ATTENTION: This is a copy from https://github.com/GoogleCloudPlatform/guest-configs/blob/18fbc050b135461879e631a5ec2dd2cd7259d8e2/src/lib/udev/google_nvme_id

dustymabe · 2023-03-30T17:24:34Z

overlay.d/README.md

+Add udev rules and scripts needed from google-guest-configs [1] for disk
+configuration in GCP, such as local SSD controllers (nvme and scsi).


let's also mention here the open BZ to get this into the RPM and that we can drop this and just include the RPM when the BZ is fixed.

We previously had the 65-gce-disk-naming.rules under the 05core overlay. This commit makes a new overlay.d entry specifically for the GCP rules, in this way we can track it and have a better maintenance Signed-off-by: Renata Ravanelli <rravanel@redhat.com>

- This PR injects/updates the udev rules from google-guest-config package into our overlay (GoogleCloudPlatform/guest-configs@18fbc05). - It works as an workaround for the BZ #2176212. The longer term solution is to add these packages as part of the Fedora/RHEL itself, so we won't need to maintain it, as we can entirely drop it. For this matter the BZ https://bugzilla.redhat.com/show_bug.cgi?id=2182865 was opened requesting a subpackage of google-compute-engine-guest-configs containing only what is needed. For more info: https://bugzilla.redhat.com/show_bug.cgi?id=2176212 https://github.com/GoogleCloudPlatform/guest-configs/tree/master/src/lib/udev Signed-off-by: Renata Ravanelli <rravanel@redhat.com>

jlebon

LGTM!

dustymabe

LGTM - The only other thing that might be nice is adding a test for this (only runs on GCP) that validates some of the rules are acting appropriately.

jlebon · 2023-04-04T19:15:34Z

Wait, I just noticed this was targeting rhcos-4.13. I think we want this in FCOS proper too though. Can you open the same PR against testing-devel too?

dustymabe · 2023-04-04T20:19:51Z

oh yeah. I thought this was targetting the testing-devel branch.

ravanelli · 2023-04-05T12:44:27Z

LGTM - The only other thing that might be nice is adding a test for this (only runs on GCP) that validates some of the rules are acting appropriately.

I created a new issue to work on it:
coreos/fedora-coreos-tracker#1457

ravanelli · 2023-04-05T12:45:05Z

Wait, I just noticed this was targeting rhcos-4.13. I think we want this in FCOS proper too though. Can you open the same PR against testing-devel too?

A my bad, probably opened it first against 413 since the issue was in this branch.

travier · 2023-04-05T13:41:44Z

New PR in #2350

ravanelli changed the base branch from testing-devel to rhcos-4.13 March 29, 2023 19:53

ravanelli requested review from dustymabe, jlebon and travier March 29, 2023 19:55

ravanelli force-pushed the bz_2176212 branch from d6c47af to 81f5b6f Compare March 30, 2023 15:27

ravanelli force-pushed the bz_2176212 branch from 81f5b6f to 2490ad5 Compare March 30, 2023 15:50

ravanelli changed the title ~~Bug 2176212 - Add/Update udev rules for GCP~~ overlay.d: create new 30gcp-udev-rules overlay Mar 30, 2023

dustymabe reviewed Mar 30, 2023

View reviewed changes

ravanelli force-pushed the bz_2176212 branch 2 times, most recently from 338bdf4 to 38f9eff Compare March 30, 2023 18:17

ravanelli added 2 commits March 30, 2023 15:19

ravanelli force-pushed the bz_2176212 branch from 38f9eff to 0d14b7f Compare March 30, 2023 18:20

jlebon approved these changes Mar 30, 2023

View reviewed changes

dustymabe approved these changes Mar 30, 2023

View reviewed changes

ravanelli mentioned this pull request Mar 30, 2023

[rhcos-4.13] tests/kola: stabilize Butane 1.5.0 spec #2339

Merged

ravanelli merged commit b944dd4 into coreos:rhcos-4.13 Apr 4, 2023

This was referenced Apr 5, 2023

overlay.d: create new 30gcp-udev-rules overlay #2350

Merged

Create a new GCP udev rules test coreos/fedora-coreos-tracker#1457

Closed

bgilbert changed the title ~~overlay.d: create new 30gcp-udev-rules overlay~~ [rhcos-4.13] overlay.d: create new 30gcp-udev-rules overlay Apr 11, 2023

ravanelli deleted the bz_2176212 branch January 2, 2024 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rhcos-4.13] overlay.d: create new 30gcp-udev-rules overlay #2336

[rhcos-4.13] overlay.d: create new 30gcp-udev-rules overlay #2336

ravanelli commented Mar 29, 2023

dustymabe commented Mar 29, 2023

jlebon commented Mar 30, 2023

dustymabe commented Mar 30, 2023 •

edited

Loading

jlebon commented Mar 30, 2023

dustymabe commented Mar 30, 2023

dustymabe commented Mar 30, 2023

ravanelli commented Mar 30, 2023

ravanelli commented Mar 30, 2023 •

edited

Loading

dustymabe Mar 30, 2023

dustymabe Mar 30, 2023

jlebon left a comment

dustymabe left a comment

jlebon commented Apr 4, 2023

dustymabe commented Apr 4, 2023

ravanelli commented Apr 5, 2023

ravanelli commented Apr 5, 2023

travier commented Apr 5, 2023

		# ATTENTION: It is a copy from https://github.com/GoogleCloudPlatform/guest-configs/blob/master/src/lib/udev/google_nvme_id
		# This file is synced with the source using the last update: Latest commit 3841945 on Feb 17 2023

	# ATTENTION: It is a copy from https://github.com/GoogleCloudPlatform/guest-configs/blob/master/src/lib/udev/google_nvme_id
	# This file is synced with the source using the last update: Latest commit 3841945 on Feb 17 2023
	# ATTENTION: This is a copy from https://github.com/GoogleCloudPlatform/guest-configs/blob/18fbc050b135461879e631a5ec2dd2cd7259d8e2/src/lib/udev/google_nvme_id

		Add udev rules and scripts needed from google-guest-configs [1] for disk
		configuration in GCP, such as local SSD controllers (nvme and scsi).

[rhcos-4.13] overlay.d: create new 30gcp-udev-rules overlay #2336

[rhcos-4.13] overlay.d: create new 30gcp-udev-rules overlay #2336

Conversation

ravanelli commented Mar 29, 2023

dustymabe commented Mar 29, 2023

jlebon commented Mar 30, 2023

dustymabe commented Mar 30, 2023 • edited Loading

jlebon commented Mar 30, 2023

dustymabe commented Mar 30, 2023

dustymabe commented Mar 30, 2023

ravanelli commented Mar 30, 2023

ravanelli commented Mar 30, 2023 • edited Loading

dustymabe Mar 30, 2023

Choose a reason for hiding this comment

dustymabe Mar 30, 2023

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

dustymabe left a comment

Choose a reason for hiding this comment

jlebon commented Apr 4, 2023

dustymabe commented Apr 4, 2023

ravanelli commented Apr 5, 2023

ravanelli commented Apr 5, 2023

travier commented Apr 5, 2023

dustymabe commented Mar 30, 2023 •

edited

Loading

ravanelli commented Mar 30, 2023 •

edited

Loading