Skip to content
This repository has been archived by the owner on Sep 18, 2020. It is now read-only.

udev: add support for AWS EBS NVMe friendly names #268

Merged
merged 1 commit into from
Jun 28, 2018

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Jun 25, 2018

Ref: coreos/bugs#2399

Note: while this is AWS-specific, the payload is so small and the lack of updates for OEM partition scares me, so I'm proposing to add it here instead of a new OEM-specific ebuild.

@cgwalters
Copy link
Member

I could imagine other cloud providers also doing this; it's not technically specific to AWS right? Or is that hardcoded offset somehow specific to AWS?

I guess my main concern boils down to: This is something we never want to do on a true physical system with real NVMe devices, as there could actually be legacy SCSI devices and we don't want to blow away their device nodes. Also I believe one could (in theory) have NVMe and SCSI devices in libvirt, although in practice people use virtio.

@lucab
Copy link
Contributor Author

lucab commented Jun 25, 2018

This is very specific to AWS volumes, see coreos/bugs#2399. I'm not aware if/which other providers are doing similar to this. For reference, NVMe devices works perfectly fine without this, it only adds support for user-customized AWS friendly names.

@bgilbert
Copy link
Contributor

I'm okay with shipping this in /usr rather than the OEM.

@lucab
Copy link
Contributor Author

lucab commented Jun 26, 2018

I manually validated this on a custom AMI on m5.large, both with the rootfs EBS and an hotplugged NVMe volume. I also fixed a minor warning reported by shellcheck. This should be ready to go, PTAL.

@cgwalters
Copy link
Member

This is very specific to AWS volumes, see coreos/bugs#2399.

OK. But one thing that isn't clear to me exactly; it looks like we're extracting some sort of raw header from the NVMe device based on a fixed offset; is there any chance that something else is there? Feels like we're at risk of creating random symlinks?

(I could be wrong)

@lucab
Copy link
Contributor Author

lucab commented Jun 26, 2018

@cgwalters this relies on parsing data out of the vendor-specific payload in the NVMe controller, and vendors can put whatever they want in there. For such reason, the udev rule is scoped to ATTRS{model}=="Amazon Elastic Block Store" as this only applies to AWS as the (virtual) NVMe provider. In order to "read something else" it means that AWS changed their vendor-specific format, which is unlikely as it will break their own compatibility. Even if unlikely that's still possible though, and we'll need to update the parsing/offsets in here in that case, which is another reason for not having this in the OEM partition.

@cgwalters
Copy link
Member

For such reason, the udev rule is scoped to ATTRS{model}=="Amazon Elastic Block Store"

Ah, I missed that. That totally addresses my concern, thanks!

@lucab
Copy link
Contributor Author

lucab commented Jun 26, 2018

@cgwalters on a second look, you are also right because not all the rules are scoped on that. I should fix that, thanks!

@cgwalters
Copy link
Member

@cgwalters on a second look, you are also right because not all the rules are scoped on that. I should fix that, thanks!

Maybe simplest to check the model in the script too?

@lucab
Copy link
Contributor Author

lucab commented Jun 27, 2018

After the last round of discussion, I amended and re-tested this on AWS. udev rules are now all matching on AWS-EBS model, and the script itself also checks that it is being run by udev on an EBS device. PTAL.

@george-angel
Copy link

How does one track which version of Container Linux this will make?

@dm0-
Copy link
Contributor

dm0- commented Jul 6, 2018

@george-angel It's already released in 1828.0.0, the current alpha.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants