Bug 1051465

Created attachment 734741 [details]
readd-format-lenght-parameter.patch

Created attachment 734743 [details]
use-truncated-sysfs-value-for-serial-id.patch

With the attached patches, I can get by-id nvme symlinks from pure sysfs values that are identical to the ones created by the scsi_id call. 
That way we can probably obsolete scsi_id.

Testpackages for SLE12SP3 are available at:

http://download.suse.de/ibs/home:/tsaupe:/branches:/SUSE:/SLE-12:/SP3:/GA:/systemd-bsc1051465/standard/

I've tested on blueshark4 and saw it working correctly, but I'd highly appreciate some more testing (unfortunately all orthos machines that have nvme devices are in use).
Johannes, could you take a look and probably give it a try?

Franck, any objection for the patches?

(In reply to Thomas Blume from comment #1)
> Created attachment 734741 [details]
> readd-format-lenght-parameter.patch

can you submit this patch upstream ?

(In reply to Franck Bui from comment #5)
> (In reply to Thomas Blume from comment #1)
> > Created attachment 734741 [details]
> > readd-format-lenght-parameter.patch
> 
> can you submit this patch upstream ?

Will do, but I'm on vacation next week and the week after and won't be able to follow the upstream communication. 
Therefore I'd prefer to submit the patch at my return.
This also gives the chance to do a little mor testing.

(In reply to Thomas Blume from comment #3)
> With the attached patches, I can get by-id nvme symlinks from pure sysfs
> values that are identical to the ones created by the scsi_id call. 
> That way we can probably obsolete scsi_id.
> 
> Testpackages for SLE12SP3 are available at:
> 
> http://download.suse.de/ibs/home:/tsaupe:/branches:/SUSE:/SLE-12:/SP3:/GA:/
> systemd-bsc1051465/standard/
> 
> I've tested on blueshark4 and saw it working correctly, but I'd highly
> appreciate some more testing (unfortunately all orthos machines that have
> nvme devices are in use).
> Johannes, could you take a look and probably give it a try?

Yes I have an Intel NVMe here as well which "just" needs to find a new home in a machine and I can/will test on qemu as well.

Thanks for looking into this.

Can you publish the patch on OBS or in a public location?

(In reply to Daniel Molkentin from comment #8)
> Can you publish the patch on OBS or in a public location?

Testpackages for Leap 42.3 are available at:

http://download.opensuse.org/repositories/home:/tsaupe:/branches:/openSUSE:/Leap:/42.3:/systemd-bsc1051465/openSUSE_Leap_42.3/

for Leap 42.2 at:

http://download.opensuse.org/repositories/home:/tsaupe:/branches:/openSUSE:/Leap:/42.3:/systemd-bsc1051465/openSUSE_Leap_42.2/

Created attachment 734940 [details]
0001-rules-use-truncated-sysfs-value-for-serial-id.patch

Sorry, there was a mistake in the udev rules. Here is an updated patch.
I have also updated the test packages.

Still no success with this new version.

(In reply to François Valenduc from comment #11)
> Still no success with this new version.

Please attach the output of the commands:

grep nvme /usr/lib/udev/rules.d/61-persistent-storage-compat.rules

udevadm test /block/$NVME

where $NVME is the devicename from:

/sys/block/

For example, if you have:

/sys/block/nvme0n1

the command is:

udevadm test /block/nvme0n1

My problem is probably different since I am not using nvme. The fact is that if I upgrade to the latest version of udev available in opensuse 42.3, the system doesn't boot any more because apparently, it doesn't find the lvm volumes. From what I can see, it finds the harddisk and all the partitions, but after that, it is completely stuck.

(In reply to François Valenduc from comment #13)
> My problem is probably different since I am not using nvme. The fact is that
> if I upgrade to the latest version of udev available in opensuse 42.3, the
> system doesn't boot any more because apparently, it doesn't find the lvm
> volumes. From what I can see, it finds the harddisk and all the partitions,
> but after that, it is completely stuck.

Indeed, the change in the udev rules is for nvme devices only.
Other devices won't be affected.
Seems we better continue processing your issue in bug 1051354.
Please attach:

/run/initramfs/rdsosreport.txt

from a failed boot there (I assume that your system boot ends in the dracut shell when the failure appears).

Got an nvme disk now. 
A comparison of the udevadm test output between my patched rules versus the rules that use scsi_id shows the following changes:

-->
@@ -116,18 +115,21 @@
 .ID_FS_TYPE_NEW=
 ACTION=add
 COMPAT_SYMLINK_GENERATION=1
-DEVLINKS=/dev/disk/by-id/nvme-INTEL_SSDPEDMD800G4_CVFT71340019800MGN /dev/disk/by-id/-INTEL_SSDPEDMD800G4_CVFT71340019800MGN /dev/disk/by-id/nvme-nvme.8086-4356465437313334303031393830304d474e-494e54454c205353445045444d443830304734-00000001
+DEVLINKS=/dev/disk/by-id/nvme-SNVMe_INTEL_SSDPEDMD80CVFT71340019800MGN /dev/disk/by-id/nvme-nvme.8086-4356465437313334303031393830304d474e-494e54454c205353445045444d443830304734-00000001 /dev/disk/by-id/nvme-INTEL_SSDPEDMD800G4_CVFT71340019800MGN
 DEVNAME=/dev/nvme0n1
 DEVPATH=/devices/pci0000:00/0000:00:04.0/0000:02:00.0/nvme/nvme0/nvme0n1
 DEVTYPE=disk
 ID_FS_TYPE=
+ID_MODEL=INTEL SSDPEDMD800G4
+ID_MODEL_TRUNCATED=INTEL SSDPEDMD80
 ID_PART_TABLE_TYPE=dos
 ID_PART_TABLE_UUID=000773dc
 ID_SERIAL=INTEL SSDPEDMD800G4_CVFT71340019800MGN
 ID_SERIAL_SHORT=CVFT71340019800MGN
+ID_SERIAL_TRUNCATED=nvme-SNVMe_INTEL SSDPEDMD80CVFT71340019800MGN
 ID_WWN=nvme.8086-4356465437313334303031393830304d474e-494e54454c205353445045444d443830304734-00000001
 MAJOR=259
 MINOR=0
 SUBSYSTEM=block
 TAGS=:systemd:
-USEC_INITIALIZED=6675380
+USEC_INITIALIZED=6442271
--<

Attaching the full test output below.
Are the symlinks ok like that?

Created attachment 735115 [details]
udevadm test with original scsi_id based rules

Created attachment 735117 [details]
udevadm test with patched rules that use sysfs information and format lenght setting

Hi Thomas,

(In reply to Thomas Blume from comment #15)
> 
> Attaching the full test output below.
> Are the symlinks ok like that?

You shouldn't see any difference in the generated symlinks, should you ?

Also please make sure to run the latest version of systemd, especially the version containing 8ea065d44c161675df2a01542889d58bbb4f850d commit.

Did you already submit your patch to upstream ?

Thanks.

(In reply to Franck Bui from comment #18)
> Hi Thomas,
> 
> (In reply to Thomas Blume from comment #15)
> > 
> > Attaching the full test output below.
> > Are the symlinks ok like that?
> 
> You shouldn't see any difference in the generated symlinks, should you ?

The patch provides symlinks with the fractional serial id, like it was shown with scsi_id used.
Still, I see a fractional serial id as kind of fault, because it might be non uniqe.
Therefore I've also created an additional symlink that provides the full serial id.
It looks like this is alo what upstream does, and I think this is the one we should use long term. We should keep the symlink with the fractional serial id only for backward compatibility.
So, the difference is an additional symlink with the full serial id.
But yes, if there are good reasons to see no difference, I can take this out.

> Also please make sure to run the latest version of systemd, especially the
> version containing 8ea065d44c161675df2a01542889d58bbb4f850d commit.
> 
> Did you already submit your patch to upstream ?

Yes, here:

https://github.com/systemd/systemd/pull/6651

It seems that this feature has been removed by purpose, see:

https://github.com/systemd/systemd/commit/a0ee5a05bb3a9a838c35e07ff7a0bb7bbd2d0c9b

But it is unclear why.
Hopefully Kay Sievers will respond in the pull request.

(In reply to Thomas Blume from comment #19)
> The patch provides symlinks with the fractional serial id, like it was shown
> with scsi_id used.
> Still, I see a fractional serial id as kind of fault, because it might be
> non uniqe.
> Therefore I've also created an additional symlink that provides the full
> serial id.
> It looks like this is alo what upstream does, and I think this is the one we
> should use long term. We should keep the symlink with the fractional serial
> id only for backward compatibility.

Well normally we already provide the "upstream" symlink so I'm not sure to understand why you needed to do it again.

> 
> It seems that this feature has been removed by purpose, see:
> 
> https://github.com/systemd/systemd/commit/
> a0ee5a05bb3a9a838c35e07ff7a0bb7bbd2d0c9b
> 
> But it is unclear why.

And sadly there weren't any explications provided when this feature was simply removed...

Let's see...

(In reply to Franck Bui from comment #20)
> (In reply to Thomas Blume from comment #19)
> > The patch provides symlinks with the fractional serial id, like it was shown
> > with scsi_id used.
> > Still, I see a fractional serial id as kind of fault, because it might be
> > non uniqe.
> > Therefore I've also created an additional symlink that provides the full
> > serial id.
> > It looks like this is alo what upstream does, and I think this is the one we
> > should use long term. We should keep the symlink with the fractional serial
> > id only for backward compatibility.
> 
> Well normally we already provide the "upstream" symlink so I'm not sure to
> understand why you needed to do it again.

Sorry, my fault, I don't add the upstream symlinks again.
The diff of the udev rule is here:

https://github.com/tblume/systemd-suse-devel/commit/78e7459c1d4daecbd4dba8b327957d05ecdc7d56

(In reply to Thomas Blume from comment #21)
> Sorry, my fault, I don't add the upstream symlinks again.
> The diff of the udev rule is here:
> 
> https://github.com/tblume/systemd-suse-devel/commit/
> 78e7459c1d4daecbd4dba8b327957d05ecdc7d56

Upstream declined re-adding the format lenght parameter, but it's rather for design reasons:

-->
I don't remember any specific reasons about this one, just that we removed a whole bunch of magic string mangling; we did not want udev provide shell-like features. People asked for more
of them, and we thought they should use a shell, if they need to do more complicated things.
[...]
I think we should have less string mangling support in udev rules, not more. hence, let's not add this, I see little point in restoring something that was removed 8 years ago and nobody
noticed or missed so far.

Closing. I hope this makes sense.
--<

Question is where we go from here. Should we add a suse-only patch or move the funbctionality to an external script/binary?
Franck, any thoughts?

(In reply to Thomas Blume from comment #22)
> Question is where we go from here. Should we add a suse-only patch or move
> the funbctionality to an external script/binary?
> Franck, any thoughts?

Well I would avoid the use of an external script if possible just for doing string mangling.

Couldn't we try to do the string mangling in the rule file directly instead, something like that instead ?

> IMPORT{program}="/bin/sh -c 'ID_SERIAL=$env{ID_SERIAL}; echo ID_SERIAL_TRUNCATED=${ID_SERIAL:0:16}'

The shell expression looks simple enough to be embedded in the rule.

(In reply to Franck Bui from comment #23)
> Couldn't we try to do the string mangling in the rule file directly instead,
> something like that instead ?
> 
> > IMPORT{program}="/bin/sh -c 'ID_SERIAL=$env{ID_SERIAL}; echo ID_SERIAL_TRUNCATED=${ID_SERIAL:0:16}'
> 
> The shell expression looks simple enough to be embedded in the rule.

That doesn't work because you can't expand ID_SERIAL within the shell.
e.g. the following rule:

KERNEL=="nvme*", ENV{DEVTYPE}=="disk", IMPORT{program}="/bin/sh -c 'ID_SERIAL=$env{ID_SERIAL}; echo ID_SERIAL_TRUNCATED=${ID_SERIAL:0:16}'"

leaves ID_SERIAL_TRUNCATED empty:

-->
IMPORT '/bin/sh -c 'ID_SERIAL=; echo ID_SERIAL_TRUNCATED=${ID_SERIAL:0:16}'' /usr/lib/udev/rules.d/60-persistent-storage-compat.rules:47
starting '/bin/sh -c 'ID_SERIAL=; echo ID_SERIAL_TRUNCATED=${ID_SERIAL:0:16}''
'/bin/sh -c 'ID_SERIAL=; echo ID_SERIAL_TRUNCATED=${ID_SERIAL:0:16}''(out) 'ID_SERIAL_TRUNCATED='
Process '/bin/sh -c 'ID_SERIAL=; echo ID_SERIAL_TRUNCATED=${ID_SERIAL:0:16}'' succeeded.
--<

because ID_SERIAL is not present in the shell environment.
But passing an udev variable/attribute to a shell script works, e.g. the rule:

KERNEL=="nvme*", ENV{DEVTYPE}=="disk", ATTRS{model}=="?*", IMPORT{program}="/tmp/echo_args.sh $attr{model}"

using a script like:

--> 
# cat /tmp/echo_args.sh 
#!/bin/sh

IN=${@/ /_}
OUT=$(echo ${IN/ /_})
echo ID_MODEL_TRUNCATED=${OUT:0:16}
--<

produces the correct output:

-->
IMPORT '/tmp/echo_args.sh INTEL SSDPEDMD800G4' /usr/lib/udev/rules.d/60-persistent-storage-compat.rules:50
starting '/tmp/echo_args.sh INTEL SSDPEDMD800G4'
'/tmp/echo_args.sh INTEL SSDPEDMD800G4'(out) 'ID_MODEL_TRUNCATED=INTEL_SSDPEDMD80'
Process '/tmp/echo_args.sh INTEL SSDPEDMD800G4' succeeded.
--<

(In reply to Thomas Blume from comment #24)
> 
> KERNEL=="nvme*", ENV{DEVTYPE}=="disk", IMPORT{program}="/bin/sh -c
> 'ID_SERIAL=$env{ID_SERIAL}; echo ID_SERIAL_TRUNCATED=${ID_SERIAL:0:16}'"
> 
> leaves ID_SERIAL_TRUNCATED empty:
> 
> because ID_SERIAL is not present in the shell environment.

Well that was an example which was supposed to show that string mangling could be inlined in the rule file.

ID_SERIAL or whatever that needs to be used, is supposed to be retrieved from sysfs by using the "$attr{xxx}" syntax.

Please note that we should make sure to not pollute the environment that is already setup by the upstream rules. Therefore setting ID_SERIAL or ID_MODEL in 61-xxx is not a good idea.

(In reply to Franck Bui from comment #25)
> (In reply to Thomas Blume from comment #24)
> > 
> > KERNEL=="nvme*", ENV{DEVTYPE}=="disk", IMPORT{program}="/bin/sh -c
> > 'ID_SERIAL=$env{ID_SERIAL}; echo ID_SERIAL_TRUNCATED=${ID_SERIAL:0:16}'"
> > 
> > leaves ID_SERIAL_TRUNCATED empty:
> > 
> > because ID_SERIAL is not present in the shell environment.
> 
> Well that was an example which was supposed to show that string mangling
> could be inlined in the rule file.

Upstream seems to prefer less string mangling in the rules and to favour using shell scripts instead (see comment#22).
I would follow them in this case.

> ID_SERIAL or whatever that needs to be used, is supposed to be retrieved
> from sysfs by using the "$attr{xxx}" syntax.

Yes, that's what the rule is doing.
 
> Please note that we should make sure to not pollute the environment that is
> already setup by the upstream rules. Therefore setting ID_SERIAL or ID_MODEL
> in 61-xxx is not a good idea.

Agreed, but can we be sure that the upstream rules always run before the compat rules?
I see it as a kind of security measure to also set ID_SERIAL in the compat rules. Since it takes the data from sysfs (just like the upstream rules do), we shouldn't overwrite something with wrong values.
But that way we can be sure that the compat rules work, even if the upstream rules fail or are executed later.
Does that make sense?

(In reply to Thomas Blume from comment #26)
> Upstream seems to prefer less string mangling in the rules and to favour
> using shell scripts instead (see comment#22).
> I would follow them in this case.

I'm not sure upstream claims that but I really prefer have the oneline shell command embedded in the rule file. It doesn't really make sense to carry a shell script that has a single command.

> 
> > ID_SERIAL or whatever that needs to be used, is supposed to be retrieved
> > from sysfs by using the "$attr{xxx}" syntax.
> 
> Yes, that's what the rule is doing.
>  
> > Please note that we should make sure to not pollute the environment that is
> > already setup by the upstream rules. Therefore setting ID_SERIAL or ID_MODEL
> > in 61-xxx is not a good idea.
> 
> Agreed, but can we be sure that the upstream rules always run before the
> compat rules?

Currently upstream rule file is named 60-xxx and ours is 61-xxx, so that should be ok.

> I see it as a kind of security measure to also set ID_SERIAL in the compat
> rules. Since it takes the data from sysfs (just like the upstream rules do),
> we shouldn't overwrite something with wrong values.
> But that way we can be sure that the compat rules work, even if the upstream
> rules fail or are executed later.
> Does that make sense?

Well if upstream doesn't set them we shouldn't either and if they set it then we can reuse them without doing any modifications.

(In reply to Franck Bui from comment #27)
> (In reply to Thomas Blume from comment #26)
> > Upstream seems to prefer less string mangling in the rules and to favour
> > using shell scripts instead (see comment#22).
> > I would follow them in this case.
> 
> I'm not sure upstream claims that but I really prefer have the oneline shell
> command embedded in the rule file. It doesn't really make sense to carry a
> shell script that has a single command.

Ok, so I will try to make this work without a shell script.
Let's see if I can find a way.

> Currently upstream rule file is named 60-xxx and ours is 61-xxx, so that
> should be ok.

Ok, so I will rely on this.
 
> Well if upstream doesn't set them we shouldn't either and if they set it
> then we can reuse them without doing any modifications.

Agreed

*** Bug 1051354 has been marked as a duplicate of this bug. ***

Created attachment 739738 [details]
0001-use-truncated-sysfs-value-for-serial-id-to-simulate-.patch

With the attached patch I can get the correct symlinks for NVMe devices without an extra script.

Hi Thomas, sorry for the delay...

Could you post your patch to @systemd-maintainers ?

Thanks.

(In reply to Franck Bui from comment #31)
> Hi Thomas, sorry for the delay...
> 
> Could you post your patch to @systemd-maintainers ?
> 

Hi Franck,

it's already there:

http://mailman.suse.de/mailman/private/systemd-maintainers/2017-September/029072.html

Really strange, I can't find your email... could you send it again ?

This has been fixed with the following commit:

261a4ef38 compat-rules: generate compat by-id symlinks with 'nvme' prefix missing (bsc#1063249)

-> closing

(In reply to Thomas Blume from comment #34)
> This has been fixed with the following commit:
> 
> 261a4ef38 compat-rules: generate compat by-id symlinks with 'nvme' prefix
> missing (bsc#1063249)
> 
> -> closing

Sorry, wrong commit, the right one is:

1805bb303 compat-rules: replace scsi_id with the use of truncated sysfs value for serial id for NVMe devices

SUSE-RU-2018:1390-1: An update that has 9 recommended fixes can now be installed.

Category: recommended (moderate)
Bug References: 1045092,1051465,1066422,1075804,1082485,1084626,1085062,1086785,1087323
CVE References: 
Sources used:
SUSE Linux Enterprise Software Development Kit 12-SP3 (src):    systemd-228-150.40.1
SUSE Linux Enterprise Server 12-SP3 (src):    systemd-228-150.40.1
SUSE Linux Enterprise Desktop 12-SP3 (src):    systemd-228-150.40.1
SUSE CaaS Platform ALL (src):    systemd-228-150.40.1
OpenStack Cloud Magnum Orchestration 7 (src):    systemd-228-150.40.1

openSUSE-RU-2018:1433-1: An update that has 9 recommended fixes can now be installed.

Category: recommended (moderate)
Bug References: 1045092,1051465,1066422,1075804,1082485,1084626,1085062,1086785,1087323
CVE References: 
Sources used:
openSUSE Leap 42.3 (src):    systemd-228-50.1, systemd-mini-228-50.1