GoogleCloudPlatform / guest-configs

Apache License 2.0
31 stars 41 forks source link

Replace `xxd` to `cut` for google_nvme_id #49

Closed HuijingHei closed 1 year ago

HuijingHei commented 1 year ago

google_nvme_id script currently uses xxd to parse nvme device info, but we need to install additional package xxd for fedora, vim-common and vim-filesystem for centos (or rhel) before using it. Replace it with cut and we do not need to install additional packages. See https://github.com/coreos/fedora-coreos-config/pull/2412#issuecomment-1544715333

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-oss-prow[bot] commented 1 year ago

Hi @HuijingHei. Thanks for your PR.

I'm waiting for a GoogleCloudPlatform member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
vorakl commented 1 year ago

@HuijingHei, thank you for bringing this up to our attention!

This change looks reasonable to me. I checked on a VM with nvme device and everything works as expected with no additional modifications. If this change helps you with your case, I don't see any reasons why not to merge it.

If you could fix that "CLA missing" thing, we can merge it.

# a1="$(/usr/sbin/nvme id-ns -b /dev/nvme0n1 | xxd -p -seek 384 | xxd -p -r)"
bash: warning: command substitution: ignored null byte in input
# a2="$(/usr/sbin/nvme id-ns -b /dev/nvme0n1 | cut -b 384-)"
bash: warning: command substitution: ignored null byte in input

# echo $a1
{"device_name":"persistent-disk-0","disk_type":"PERSISTENT"}
# echo $a2
{"device_name":"persistent-disk-0","disk_type":"PERSISTENT"}

# ls /dev/vnme*
ls: cannot access '/dev/vnme*': No such file or directory
root@vm3:/home/vorakl# ls /dev/nvme*
/dev/nvme0  /dev/nvme0n1  /dev/nvme0n1p1  /dev/nvme0n1p15
zmarano commented 1 year ago

/ok-to-test

HuijingHei commented 1 year ago

Thanks @vorakl for the review and testing, "CLA missing" is fixed.

vorakl commented 1 year ago

/lgtm

vorakl commented 1 year ago

/approve

google-oss-prow[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HuijingHei, vorakl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/GoogleCloudPlatform/guest-configs/blob/master/OWNERS)~~ [vorakl] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
tytso commented 1 year ago

Note to @HuijingHei , I was looking into why this change had to be reverted, and I believe I see the problem. In fact, I'm quite surprised that this worked when you tested it on Fedora.

The problem is that "nvme id-ns -b /dev/nvme0n1" requested the information in a binary form; this means that there are no newline (\n) characters in the file, unless one appears by accident; it is a binary file, not a text file. But the "cut" program works on text files. Specifically, "cut -b 384" will interpret the binary file as a text file, and return the last 384 bytes in each "line" (where a line is terminated by a newline character) as a line in the output --- and if the "last line" in the file is terminated by an EOF, and does not have a trailing \n character at the end of the file, cut will happily add that trailing newline. So "cut -b 384" does something quite different from "xxd -p --seek 384 | xxd -p -r". This can been seen by running a test such as:

nvme id-ns -b /dev/nvme0n1 | xxd -p --seek 384 | xxd -p -r | od -x

and comparing it with:

nvme id-ns -b /dev/nvme0n1 | cut -b 384 | od -x

Now, if you really want to avoid using xxd, you could try something like this instead:

nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x

And since dd and cut are in the coreutils package, which pretty much every single distro is going to ship as part of their minimal package set, this should address the needs of this original PR. Creating a patch which does this, and then doing a full set of tests to make darned sure that the replacement is identical is left as an exercise to the reader. (I don't particularly care, because I'm using Debian for gce-xfstests, and as far as I'm concerned, installing xxd isn't a big deal. But if you really want to get rid of this package dependency, this is probably the way to go.)

HuijingHei commented 1 year ago

Thanks @tytso , do testing on Fedora CoreOS, looks dd is more sane to replace xxd.

]# nvme id-ns -b /dev/nvme0n1 | xxd -p --seek 384 | xxd -p -r | od -x
0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a
0000020 6570 7372 7369 6574 746e 642d 7369 2d6b
0000040 2230 222c 6964 6b73 745f 7079 2265 223a
0000060 4550 5352 5349 4554 544e 7d22 0000 0000
0000100 0000 0000 0000 0000 0000 0000 0000 0000
*
0007200

]# nvme id-ns -b /dev/nvme0n1 | cut -b 384- | od -x
0000000 7b00 6422 7665 6369 5f65 616e 656d 3a22
0000020 7022 7265 6973 7473 6e65 2d74 6964 6b73
0000040 302d 2c22 6422 7369 5f6b 7974 6570 3a22
0000060 5022 5245 4953 5453 4e45 2254 007d 0000
0000100 0000 0000 0000 0000 0000 0000 0000 0000
*
0007200 0a00
0007202

]# nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null  | od -x
0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a
0000020 6570 7372 7369 6574 746e 642d 7369 2d6b
0000040 2230 222c 6964 6b73 745f 7079 2265 223a
0000060 4550 5352 5349 4554 544e 7d22 0000 0000
0000100 0000 0000 0000 0000 0000 0000 0000 0000
*
0007200

~]# nvme id-ns -b /dev/nvme0n1 | xxd -p --seek 384 | xxd -p -r
{"device_name":"persistent-disk-0","disk_type":"PERSISTENT"}[root@hhei-fcos ~]# 

~]# nvme id-ns -b /dev/nvme0n1 | cut -b 384-
{"device_name":"persistent-disk-0","disk_type":"PERSISTENT"}

~]# nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null
{"device_name":"persistent-disk-0","disk_type":"PERSISTENT"}[root@hhei-fcos ~]#