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

Change ec2nvme-nsid to use Bash built-ins #32

Merged

Conversation

ziggythehamster
Copy link
Contributor

This simple change cuts the runtime by more than half and CPU by almost a third. It also makes the script more reliable when resources are constrained (the sort of situation where you might suddenly attach more EBS NVMe volumes) and helps improve the performance of the early boot process (initrd/initramfs), should you make these scripts part of that.

Performance before:

$ time bash -c 'for i in $(seq 1 10000); do bash ./ec2nvme-nsid nvme0n1234234p2 > /dev/null; done'
bash -c   38.00s user 90.67s system 92% cpu 2:18.43 total

Performance after:

$ time bash -c 'for i in $(seq 1 10000); do bash ./ec2nvme-nsid nvme0n1234234p2 > /dev/null; done'
bash -c   12.61s user 27.56s system 66% cpu 1:00.26 total

This simple change cuts the runtime by more than half and CPU by almost a third. It also makes
the script more reliable when resources are constrained (the sort of situation where you might
suddenly attach more EBS NVMe volumes) and helps improve the performance of the early boot
process (initrd/initramfs), should you make these scripts part of that.

Performance before:

$ time bash -c 'for i in $(seq 1 10000); do bash ./ec2nvme-nsid nvme0n1234234p2 > /dev/null; done'
bash -c   38.00s user 90.67s system 92% cpu 2:18.43 total

Performance after:

$ time bash -c 'for i in $(seq 1 10000); do bash ./ec2nvme-nsid nvme0n1234234p2 > /dev/null; done'
bash -c   12.61s user 27.56s system 66% cpu 1:00.26 total

Signed-off-by: Keith Gable <gablk@amazon.com>
@ziggythehamster
Copy link
Contributor Author

I've got a few other changes that I'll submit PRs for (I've been maintaining copies of these scripts for years - long before we became part of Amazon :)), and will submit them shortly. The .spec file will have conflicts because of that, so let me know the least friction way to get the changes merged. Some projects prefer small PRs with atomic changes, where others prefer a bigger PR - let me know which is easiest for y'all. Also feel free to Slack me on-corp.

@ziggythehamster
Copy link
Contributor Author

ziggythehamster commented Jan 18, 2024

It actually appears that the only change I can really safely land was #33 - the other things I have ready are:

  • Use nvme-cli >= 1.13 to have a Bash-only implementation of ebsnvme-id (nvme amzn id-ctrl returns the same information). This eliminates the dependency on Python, which is a necessity if you require the udev rules to work in early-boot. I can't open a PR for this because AL2023 ships nvme-cli 1.11 and would thus be incompatible. (EL8 ships nvme-cli 1.16 and EL9 ships nvme-cli 2.4, for context.) I'll open an issue for it. udev rules and scripts should be (optionally) available in early-boot #34
  • Introduce a Dracut module that installs the udev rules and necessary executables so that the block device mapping names become available in early-boot - e.g. if you have your root volume on something like ZFS, Btrfs, or LVM, and want to be able to mount the volume from the cluster of disks that are named according to their block device mapping names (or single disk, even).

ziggythehamster added a commit to art19/amazon-ec2-utils that referenced this pull request Jan 24, 2024
This consolidates the changes from amazonlinux#32 and amazonlinux#33
into one nice PR

---------

Signed-off-by: Keith Gable <gablk@amazon.com>
@nmeyerhans nmeyerhans requested a review from vigh-m March 2, 2024 01:36
@nmeyerhans nmeyerhans assigned nmeyerhans and vigh-m and unassigned nmeyerhans Mar 2, 2024
@nmeyerhans nmeyerhans requested review from nmeyerhans and removed request for vigh-m March 2, 2024 01:36
Copy link
Contributor

@nmeyerhans nmeyerhans left a comment

Choose a reason for hiding this comment

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

LGTM. Going to leave this for @vigh-m to handle the rest of the merge activities, as he'll be taking over as maintainer.

@vigh-m vigh-m merged commit 37726e7 into amazonlinux:main Mar 6, 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.

3 participants