Add xxd package and change google_nvme_id permission

[ravanelli]

See: https://issues.redhat.com/browse/OCPBUGS-7582

[dustymabe]

Suggested change

	# It will include xxd for FCOS and vim-common for RHCOS
	# We can drop it once we get https://bugzilla.redhat.com/show_bug.cgi?id=2182865 done
	# It will include xxd for FCOS and vim-common for RHCOS.
	# We can drop it once we get https://bugzilla.redhat.com/show_bug.cgi?id=2182865
	# done because the package should name all dependencies.

[dustymabe]

LGTM - one suggestion

[jlebon]

Before going through the whole backport process across the two repos, it would be good to sanity-check at this point that the expected symlinks show up in an FCOS booted in GCP. If the concerned NVMe devices are the default ones, then we can add a non-exclusive test for it gated on gcp that checks that the symlinks exist without any additional work needed in cosa.

[travier]

I don't want us to keep pilling workaround for this. Can we investigate if we really need xxd here of if we can use something else? We are generally very conservative when adding new user facing packages in FCOS and this change should follow the same process.

This change should land with a kola test that verifies it: coreos/fedora-coreos-tracker#1457

[HuijingHei]

I think we need xxd, check on unfixed rhcos on gcp confidential vm, do testing like this:

# chmod +x /usr/lib/udev/google_nvme_id

# /usr/lib/udev/google_nvme_id -d /dev/nvme0n2
/usr/lib/udev/google_nvme_id: line 65: xxd: command not found
/usr/lib/udev/google_nvme_id: line 65: xxd: command not found

$ ls /dev/disk/by-id/ -l | grep nvme0n2
lrwxrwxrwx. 1 root root 13 May 10 08:47 nvme-nvme.1ae0-xxx-00000002 -> ../../nvme0n2

After install vim-common and run, check we have /dev/disk/by-id/google-persistent-disk-1 (the format might be needed by ocp).

[root@hhei-rhcos ~]# /usr/lib/udev/google_nvme_id -s -d /dev/nvme0n2
/usr/lib/udev/google_nvme_id: line 65: warning: command substitution: ignored null byte in input
[root@hhei-rhcos ~]# ls /dev/disk/by-id/ -l | grep nvme0n2
lrwxrwxrwx. 1 root root 12 May 10 08:50 google-persistent-disk-1 -> /dev/nvme0n2
lrwxrwxrwx. 1 root root 13 May 10 08:47 nvme-nvme.1ae0-xxx-00000002 -> ../../nvme0n2

It would be better if do testing with FCOS that includes this patch. @ravanelli is there any guidance I can build gcp image and do testing?

[jlebon]

I don't want us to keep pilling workaround for this. Can we investigate if we really need xxd here of if we can use something else? We are generally very conservative when adding new user facing packages in FCOS and this change should follow the same process.

It's a dep of the script. Once https://bugzilla.redhat.com/show_bug.cgi?id=2182865 happens, it would've been pulled in via the new subpackage that holds google_nvme_id. We're short-circuiting that process here by manually shipping the script ourselves, so we have to add its dep manually too. Unless we get a PR merged against https://github.com/GoogleCloudPlatform/guest-configs to drop the dep, I'd rather not change the script on our own since the plan is to drop it eventually.

[travier]

We generally look at dependencies for packages that we add and consider whether we want them included as well. For example right now we have the audit package inclusion blocked on it depending on the service binary coming as a dependency. It's the same case here: we would be adding xxd to the image as a user facing binary.

---

According to https://manpages.ubuntu.com/manpages/xenial/man1/nvme-id-ns.1.html, this udev rule is parsing JSON output and that is likely better done with jq. I'd prefer we look into using jq and make a PR to the upstream project.

[HuijingHei]

According to https://manpages.ubuntu.com/manpages/xenial/man1/nvme-id-ns.1.html, this udev rule is parsing JSON output and that is likely better done with jq. I'd prefer we look into using jq and make a PR to the upstream project.

Directly using jq to parse the json output failed, need to skip the first 384 characters. Maybe can replace xxd with cut to skip first 384 character, then use jq, does this make sense?

# nvme_json="$(nvme id-ns /dev/nvme0n1 -b -o json)"
-bash: warning: command substitution: ignored null byte in input
# echo $nvme_json
@@����P��^���� {"device_name":"persistent-disk-0","disk_type":"PERSISTENT"}
# echo $nvme_json | jq -r .device_name
parse error: Invalid numeric literal at line 1, column 20

update

nvme_json="$("$nvme_cli_bin" id-ns -b "$1" | xxd -p -seek 384 | xxd -p -r)"
device_name="$(echo "$nvme_json" | grep device_name | sed -e 's/.*"device_name":[  \t]*"\([a-zA-Z0-9_-]\+\)".*/\1/')"

to

nvme_json="$("$nvme_cli_bin" id-ns -b "$1" | cut -c 384-)"
device_name="$(echo "$nvme_json" | jq -r .device_name)"

Test works like this:

# which xxd
/usr/bin/which: no xxd in ...

# ls /dev/disk/by-id/ -l   <---- can not find /dev/disk/by-id/google-<device name> links
lrwxrwxrwx. 1 root root 13 May 11 03:21 google-nvme_card-pd -> ../../nvme0n1
lrwxrwxrwx. 1 root root 15 May 11 03:21 google-nvme_card-pd-part1 -> ../../nvme0n1p1
lrwxrwxrwx. 1 root root 15 May 11 03:21 google-nvme_card-pd-part2 -> ../../nvme0n1p2
lrwxrwxrwx. 1 root root 15 May 11 03:21 google-nvme_card-pd-part3 -> ../../nvme0n1p3
lrwxrwxrwx. 1 root root 15 May 11 03:21 google-nvme_card-pd-part4 -> ../../nvme0n1p4
lrwxrwxrwx. 1 root root 13 May 11 03:21 nvme-nvme.1ae0-6e766d655f636172642d7064-6e766d655f636172642d7064-00000001 -> ../../nvme0n1
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme.1ae0-6e766d655f636172642d7064-6e766d655f636172642d7064-00000001-part1 -> ../../nvme0n1p1
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme.1ae0-6e766d655f636172642d7064-6e766d655f636172642d7064-00000001-part2 -> ../../nvme0n1p2
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme.1ae0-6e766d655f636172642d7064-6e766d655f636172642d7064-00000001-part3 -> ../../nvme0n1p3
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme.1ae0-6e766d655f636172642d7064-6e766d655f636172642d7064-00000001-part4 -> ../../nvme0n1p4
lrwxrwxrwx. 1 root root 13 May 11 03:21 nvme-nvme.1ae0-6e766d655f636172642d7064-6e766d655f636172642d7064-00000002 -> ../../nvme0n2
lrwxrwxrwx. 1 root root 13 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd -> ../../nvme0n1
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd-part1 -> ../../nvme0n1p1
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd-part2 -> ../../nvme0n1p2
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd-part3 -> ../../nvme0n1p3
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd-part4 -> ../../nvme0n1p4
lrwxrwxrwx. 1 root root 13 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd_1 -> ../../nvme0n1
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd_1-part1 -> ../../nvme0n1p1
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd_1-part2 -> ../../nvme0n1p2
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd_1-part3 -> ../../nvme0n1p3
lrwxrwxrwx. 1 root root 15 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd_1-part4 -> ../../nvme0n1p4
lrwxrwxrwx. 1 root root 13 May 11 03:21 nvme-nvme_card-pd_nvme_card-pd_2 -> ../../nvme0n2

# rpm-ostree usroverlay 

# chmod +x /usr/lib/udev/google_nvme_id
# grep cut /usr/lib/udev/google_nvme_id
  nvme_json="$("$nvme_cli_bin" id-ns -b "$1" | cut -c 384-)"
# grep jq /usr/lib/udev/google_nvme_id
  device_name="$(echo "$nvme_json" | jq -r .device_name)"
# udevadm trigger 
# ls /dev/disk/by-id/ -l
...
lrwxrwxrwx. 1 root root 13 May 11 02:57 google-persistent-disk-0 -> ../../nvme0n1
lrwxrwxrwx. 1 root root 13 May 11 02:57 google-persistent-disk-1 -> ../../nvme0n2

[travier]

See https://manpages.ubuntu.com/manpages/xenial/man1/nvme-id-ns.1.html: you have to remove the -b option passed to nvme id-ns to not get binary output.

[jlebon]

Just to surface a discussion we had about this. We aren't going to ship it into 4.13 GA so we can more take our time with this.

The suggestion is to go through the New Package Request flow for google-compute-engine-guest-configs-udev and work through any issues we have there (e.g. dropping deps, cleaning things up, and adding tests).

[dustymabe]

@travier
See https://manpages.ubuntu.com/manpages/xenial/man1/nvme-id-ns.1.html: you have to remove the -b option passed to nvme id-ns to not get binary output.

I think the problem here is that we need the binary output. Basically GCP is storing information in the "vendor data" and the script is dumping the raw data out to access it.

Here are some examples:

[core@fedora-coreos-stable-1 ~]$ sudo /usr/sbin/nvme id-ns /dev/nvme1n1 -o json
{
  "nsze":20971520,
  "ncap":20971520,
  "nuse":0,
  "nsfeat":2,
  "nlbaf":0,
  "flbas":0,
  "mc":0,
  "dpc":0,
  "dps":0,
  "nmic":0,
  "rescap":0,
  "fpi":0,
  "dlfeat":0,
  "nawun":255,
  "nawupf":255,
  "nacwu":0,
  "nabsn":2047,
  "nabo":0,
  "nabspf":2047,
  "noiob":0,
  "nvmcap":"0",
  "nsattr":0,
  "nvmsetid":0,
  "mssrl":0,
  "mcl":0,
  "msrc":0,
  "nulbaf":0,
  "anagrpid":0,
  "endgid":0,
  "eui64":"0000000000000000",
  "nguid":"3cffc987baf0854c0000000000000000",
  "lbafs":[
    {
      "ms":0,
      "ds":9,
      "rp":0
    }
  ]
}

so when we pass -o json we don't get the device_name data we need. We can get it from --vendor-specific but it outputs it as a hexdump:

[core@fedora-coreos-stable-1 ~]$ sudo /usr/sbin/nvme id-ns /dev/nvme1n1 --vendor-specific                                                                                                                   
NVME Identify Namespace 1:                                                                            
nsze    : 0x1400000                                                                                   
ncap    : 0x1400000                                                                                   
nuse    : 0                                                                                           
nsfeat  : 0x2
nlbaf   : 0
flbas   : 0
mc      : 0
dpc     : 0
dps     : 0
nmic    : 0
rescap  : 0
fpi     : 0
dlfeat  : 0
nawun   : 255
nawupf  : 255
nacwu   : 0
nabsn   : 2047
nabo    : 0
nabspf  : 2047
noiob   : 0
nvmcap  : 0
mssrl   : 0
mcl     : 0
msrc    : 0
nulbaf  : 0
anagrpid: 0
nsattr  : 0
nvmsetid: 0
endgid  : 0
nguid   : 3cffc987baf0854c0000000000000000
eui64   : 0000000000000000
lbaf  0 : ms:0   lbads:9  rp:0 (in use)
vs[]:
       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
0000: 5f 6e 61 6d 65 22 3a 22 66 65 64 6f 72 61 2d 63 "_name":"fedora-c"
0010: 6f 72 65 6f 73 2d 73 74 61 62 6c 65 2d 31 22 2c "oreos-stable-1","
0020: 22 64 69 73 6b 5f 74 79 70 65 22 3a 22 50 45 52 ""disk_type":"PER"
0030: 53 49 53 54 45 4e 54 22 7d 00 00 00 00 00 00 00 "SISTENT"}......."
0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 "................"
0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 "................"
0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 "................"
0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 "................"

so that's not really useful either.

Indeed IMO the best course of action is to use cut as @HuijingHei suggested. But rather than asking upstream to include a jq dep I suggest they just keep using sed like they are today:

nvme_json="$("$nvme_cli_bin" id-ns -b "$1" | cut -c 384-)"
device_name="$(echo "$nvme_json" | grep device_name | sed -e 's/.*"device_name":[  \t]*"\([a-zA-Z0-9_-]\+\)".*/\1/')"

so we'd end up with:

[root@fedora-coreos-stable-1 ~]# set -- /dev/nvme1n1
[root@fedora-coreos-stable-1 ~]# nvme_cli_bin=/usr/sbin/nvme
[root@fedora-coreos-stable-1 ~]# nvme_json="$("$nvme_cli_bin" id-ns -b "$1" | cut -c 384-)"
-bash: warning: command substitution: ignored null byte in input
[root@fedora-coreos-stable-1 ~]# echo $nvme_json 
{"device_name":"fedora-coreos-stable-1","disk_type":"PERSISTENT"}
[root@fedora-coreos-stable-1 ~]# device_name="$(echo "$nvme_json" | grep device_name | sed -e 's/.*"device_name":[  \t]*"\([a-zA-Z0-9_-]\+\)".*/\1/')"
[root@fedora-coreos-stable-1 ~]# echo $device_name 
fedora-coreos-stable-1

I'm not sure on the bash: warning in there. Would need to look at it some more.

[dustymabe]

actually cut -b 384- would be better (bytes, not characters):

nvme_json="$("$nvme_cli_bin" id-ns -b "$1" | cut -b 384-)"
device_name="$(echo "$nvme_json" | grep device_name | sed -e 's/.*"device_name":[  \t]*"\([a-zA-Z0-9_-]\+\)".*/\1/')"

[dustymabe]

I guess the bash warning isn't new so we can ignore it:

[root@fedora-coreos-stable-1 ~]# sudo bash /usr/lib/udev/google_nvme_id -d /dev/nvme1n1
/usr/lib/udev/google_nvme_id: line 65: warning: command substitution: ignored null byte in input
ID_SERIAL_SHORT=fedora-coreos-stable-1
ID_SERIAL=Google_PersistentDisk_fedora-coreos-stable-1

[HuijingHei]

Thanks all for the suggestions, and good news is GoogleCloudPlatform/guest-configs#49 is merged. cc @ravanelli

[travier]

I think the problem here is that we need the binary output. Basically GCP is storing information in the "vendor data" and the script is dumping the raw data out to access it.

Thanks for investigation!

[jlebon]

Looks like @HuijingHei's PR has already been autoreleased: https://github.com/GoogleCloudPlatform/guest-configs/releases/tag/20230515.00.

The Fedora package is hooked to release monitoring, so it should file an RHBZ soon for it.

[HuijingHei]

Looks like @HuijingHei's PR has already been autoreleased: https://github.com/GoogleCloudPlatform/guest-configs/releases/tag/20230515.00.

The Fedora package is hooked to release monitoring, so it should file an RHBZ soon for it.

IMU, you mean https://bugzilla.redhat.com/show_bug.cgi?id=2204489 ? And @dustymabe 's PR

[ravanelli]

I think we are good to close it now that we have all the fixes the need in upstream.
The dracut module also got merged GoogleCloudPlatform/guest-configs#53

[tytso]

Please note that GoogleCloudPlatform/guest-configs#49 had to be reverted.

My analysis of the problem with GoogleCloudPlatform/guest-configs#49 which required that it be reverted can be found here. This might be helpful if you want to prepare another pull request for the guest-configs GCP repo.

Cheers!