Skip to content
This repository was archived by the owner on Oct 27, 2022. It is now read-only.

rely on udev to manage symlinks, closes #2 #3

Merged
merged 2 commits into from
Feb 15, 2020

Conversation

brahman81
Copy link
Contributor

  • uses xvd notation to match Ubuntu default for Xen virtual devices
  • uses [:space:] character set to delete all whitespace
  • move script to /usr/local/sbin as this is a system script

 * uses `xvd` notation to match Ubuntu default for Xen virtual devices
 * uses [:space:] character set to delete all whitespace
 * move script to `/usr/local/sbin` as this is a system script
@@ -1,10 +1,21 @@
#!/bin/bash

Choose a reason for hiding this comment

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

The name of this file doesn't match the udev rule reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Just submitted a fix.

@Kasama
Copy link

Kasama commented Feb 12, 2019

It would probably be nice to update the README as well to make sure the explanation match the actual provided files.

@b-dean
Copy link
Contributor

b-dean commented Mar 9, 2019

Using this PR I still get the wrong devices for the root device:

[root@ip-10-75-0-84 ec2-user]# ls -l /dev/xv*
lrwxrwxrwx 1 root root 7 Mar  9 20:07 /dev/xvda -> nvme1n1
lrwxrwxrwx 1 root root 9 Mar  9 20:07 /dev/xvda1 -> nvme0n1p1
lrwxrwxrwx 1 root root 7 Mar  9 20:07 /dev/xvdb -> nvme2n1

when it should be /dev/nvme0n1p2

[root@ip-10-75-0-84 ec2-user]# df -h
Filesystem      Size  Used Avail Use% Mounted on
/dev/nvme0n1p2   10G  3.2G  6.9G  32% /
devtmpfs        3.7G     0  3.7G   0% /dev
tmpfs           3.7G     0  3.7G   0% /dev/shm
tmpfs           3.7G   17M  3.7G   1% /run
tmpfs           3.7G     0  3.7G   0% /sys/fs/cgroup
tmpfs           753M     0  753M   0% /run/user/1000

(note that I don't have the xvdb mounted anywhere, was just adding another block device)

The vendor specific info says sda1 for the device that ends up on /. This is a RHEL-7 AMI.

Should there be a /dev/xvda2?

@@ -1,10 +1,21 @@
#!/bin/bash
# Inspired by https://github.com/oogali/ebs-automatic-nvme-mapping
Copy link
Owner

Choose a reason for hiding this comment

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

I assume this comment block was from your gist, I don't think this needs to be incorporated upstream.

( test -b "${blkdev}" && test -L "${mapping}" ) || ln -s "${blkdev}" "${mapping}"
fi
done
if [[ -x /usr/sbin/nvme ]] && [[ -b ${1} ]]; then
Copy link
Owner

Choose a reason for hiding this comment

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

If your test conditions are not matched, this script will silently fail (with a return value of zero).

A better option would be to invert the test conditions, as to be loud and verbose that we can't find the nvme program and that a required argument is missing.

# use `xvd` prefix instead of `sd`
# remove all trailing space
nvme_link=$( \
/usr/sbin/nvme id-ctrl --raw-binary "${1}" | \
Copy link
Owner

Choose a reason for hiding this comment

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

If the required argument passed is not a valid path to a block device -- should we complain? Or should we let nvme complain and have the user decipher its error message?

@oogali oogali merged commit 56e2eb5 into oogali:master Feb 15, 2020
b-dean added a commit to b-dean/ebs-automatic-nvme-mapping that referenced this pull request Feb 9, 2021
this had previously been fixed in oogali#3 but it was changed before it was merged.

then I tried to fix it again in oogali#8 but missed some of the digits
b-dean added a commit to b-dean/ebs-automatic-nvme-mapping that referenced this pull request Feb 9, 2021
also fix paths in `ebs-nvme-mapping.sh`

this had previously been fixed in oogali#3 but it was changed before it was merged.

then I tried to fix it again in oogali#8 but missed some of the digits
b-dean added a commit to b-dean/ebs-automatic-nvme-mapping that referenced this pull request Feb 9, 2021
also fix paths in `ebs-nvme-mapping.sh`

this had previously been fixed in oogali#3 but it was changed before it was merged.

then I tried to fix it again in oogali#8 but missed some of the digits
b-dean added a commit to b-dean/ebs-automatic-nvme-mapping that referenced this pull request Feb 9, 2021
also fix paths in `ebs-nvme-mapping.sh`

this had previously been fixed in oogali#3 but it was changed before it was merged.

then I tried to fix it again in oogali#8 but missed some of the digits

rebased on top of @pforman-zymergen's PR oogali#13 because
I want that binary check but udev needs the full paths
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants