NVMe symlinks broken by devices with spaces in model or serial strings

Bug #1647485 reported by Dan Streetman
50
This bug affects 7 people
Affects Status Importance Assigned to Milestone
maas-images
Fix Released
Undecided
Unassigned
systemd
Fix Released
Unknown
systemd (Debian)
Fix Released
Unknown
systemd (Ubuntu)
Fix Released
High
Unassigned
Trusty
Fix Released
High
Dimitri John Ledkov
Xenial
Fix Released
High
Unassigned
Yakkety
Fix Released
High
Unassigned
Zesty
Fix Released
High
Unassigned

Bug Description

[Impact]

After including the patch from bug 1642903, NVMe devices that include spaces in their model or serial strings result in incorrect symlinks, e.g. if the model string is "XYZ Corp NVMe drive" then instead of creating:
/dev/disk/by-id/nvme-XYZ Corp NVMe drive_SERIAL -> ../../nvme0n1
it creates:
/dev/disk/by-id/nvme-XYZ -> ../../nvme0n1
/dev/Corp -> nvme0n1
/dev/NVMe -> nvme0n1
/dev/drive_SERIAL -> nvme0n1

This is because of the way udev handles the SYMLINK value strings; by default, it does not do any whitespace replacement. To enable whitespace replacement of a symlink value, the rule must also include OPTIONS+="string_escape=replace". This is done for 'md' and 'dm' devices in their rules. However, there are no rules that actually want to specify multiple symlinks, and defaulting to not replacing whitespace makes no sense; instead, the default should be to replace all whitespace in each symlink value, unless the rule explicitly specifies OPTIONS+="string_escape=none".

[Test Case]

This assumes using udev with the patch from bug 1642903.

Without this patch, when using a NVMe drive that contains spaces in its model and/or serial strings, check the /dev/disk/by-id/ directory. It should contain a partially-correct symlink to the NVMe drive, with the name up to the first space. All following space-separated parts of the mode/serial string should have symlinks in the /dev/ directory. This is the incorrect behavior.

With this patch, check the /dev/disk/by-id/ directory. It should contain a fully-correct symlink to the NVMe drive, and no part of the drive's model/serial number string should be a link in the /dev directory.

An example of the correct/incorrect naming is in the Impact section.

There should be no other changes to any of the symlinks under /dev before and after this patch. Typical locations for symlinks are /dev/, /dev/disk/by-name/, /dev/disk/by-id/, /dev/disk/by-uuid/, /dev/disk/by-label/

[Regression Potential]

Errors in udev rules can lead to an unbootable or otherwise completely broken system if they unintentionally break or clobber existing /dev/disks/ symlinks.

[Other Info]

This is also tracked with upstream systemd (udev) bug 4833:
https://github.com/systemd/systemd/issues/4833

Also note, this can be worked around in individual rules ONLY (i.e. not fixed for all rules) by appending OPTIONS+="string_escape=replace" to each of the NVMe rules with SYMLINK+="..." assignment, e.g.:

KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ATTRS{model}=="?*", ENV{ID_SERIAL_SHORT}=="?*", ENV{ID_SERIAL}="$attr{model}_$env{ID_SERIAL_SHORT}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}", OPTIONS+="string_escape=replace"

Related bugs:
 * bug 1642903: introduce disk/by-id (model_serial) symlinks for NVMe drives
 * bug 1651602: NVMe driver regression for non-smp/1-cpu systems
 * bug 1649635: export nvme drive model/serial strings via sysfs (trusty)

Dan Streetman (ddstreet)
description: updated
Dan Streetman (ddstreet)
tags: added: sts sts-sponsor sts-sru
Dan Streetman (ddstreet)
description: updated
Revision history for this message
Dan Streetman (ddstreet) wrote :

debdiffs added for t/x/y/z that add all 3 patches proposed in the upstream bug, that:
-change udev to replace spaces in all SYMLINK values by default (unless option string_escape=none is used by a rule)
-update test/udev-test.pl test case to use string_escape=none option for test cases that use spaces in their SYMLINK value, and expect the spaces to not be replaced
-update man page to clarify that spaces in SYMLINK values will be replaced with underscore, unless string_escape=none option is used

Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

The attachment "lp1647485-zesty.debdiff" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

tags: added: patch
Revision history for this message
Dan Streetman (ddstreet) wrote :

This tarball contains debdiffs for x/y/z that *workaround* - not fix - the problem, by adding the string_escape=replace option to the NVMe rules that create the model/serial symlinks. This is possible as a temporary alternative to the real fix to udev. Unlike the real fix, which will change the default behavior for all udev rules that create symlinks (i.e. change the default to replace whitespace with underscores), this workaround only will change the behavior of the NVMe symlink rules (to replace whitespace with underscores).

As the previous NVMe udev rules patch (from bug 1642903) isn't yet in the proposed trusty package, this tarball also includes the plain patch, which should apply to the trusty udev git codebase.

Revision history for this message
Dan Streetman (ddstreet) wrote :

debdiffs updated to include (LP: #1647485) in changelog

Revision history for this message
Dan Streetman (ddstreet) wrote :
Revision history for this message
Martin Pitt (pitti) wrote :

This is still being discussed upstream, so let's get to an agreement there first, land it in upstream master, and then backport the change. Note that there is not much point with debdiffs, it's easier to cherry-pick the fix directly into the packaging branches with gbp pq and debian/git-cherry-pick.

Changed in systemd:
status: Unknown → New
Revision history for this message
Dan Streetman (ddstreet) wrote :

> Note that there is not much point with debdiffs, it's easier to cherry-pick the fix directly into
> the packaging branches with gbp pq and debian/git-cherry-pick.

ok, I removed the debdiffs as they're not needed.

I did attach a single patch, that works around the bug in a simple way, by updating only the NVMe udev rules to force whitespace replacement with underscores. This allows the NVMe symlinks to work even for devices with whitespace in their model/serial strings, while leaving the rest of udev unchanged. This patch would allow the ubuntu udev pkgs to work correctly, while the bug is discussed upstream. Note that this workaround will not break anything even if upstream uses an alternate way of fixing this.

Revision history for this message
Dan Streetman (ddstreet) wrote :
Revision history for this message
Dan Streetman (ddstreet) wrote :

patch filename and Subject: had 'string_escape=release', fixed to 'string_escape=replace'. Actual patch contents were correct and aren't changed in this patch.

Revision history for this message
Dan Streetman (ddstreet) wrote :

Note: the workaround patch from comment 17 *will not* conflict with whatever fix is implemented upstream. The patch changes *only* the NVMe rules to force whitespace replacement. If upstream changes the default behavior, to replace whitespace, then this workaround patch becomes redundant/unneeded, but it will not conflict.

The issue is the behavior of udev for SYMLINK values, depending on the "string_escape" option setting for each SYMLINK rule. There are 3 possible settings, 1) unset (the default), 2) "none", and 3) "replace". As expected, when set to "none", udev does not replace any characters (neither whitespace nor any other characters) in the SYMLINK value; and when set to "replace", udev replaces all whitespace, as well as invalid characters, with underscores. The default behavior (when unset) is what may change upstream - currently, it replaces all invalid characters but *does not* replace whitespace. This is a problem for SYMLINK values, since the SYMLINK string is a whitespace-separated list of strings. No rules that are included with udev currently want this behavior, but there may be some custom udev rules that expect it. The NVMe rules do not expect or want it. For reference, the NVMe udev rules are:

SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}"
SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}-part%n"

these rules expect one, and only one, symlink to be created for each new NVMe drive (or NVMe partition) that is detected. Under no circumstances do these rules ever want multiple symlinks created for a single execution of the rule. So this workaround, to set "string_escape=replace" only for these 2 NVMe rules, is appropriate. If the default behavior upstream is changed later, so that any whitespace in the ID_SERIAL string is replaced when "string_escape" is unset, then the behavior of udev for these specific rules will be identical whether "string_escape" is unset or set to "replace", and so at that point this workaround patch can be left or reverted, as the udev behavior will be identical either way. Until then, this workaround patch is needed for the correct behavior.

Revision history for this message
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in systemd (Ubuntu):
status: New → Confirmed
Scott Moser (smoser)
description: updated
Dan Streetman (ddstreet)
description: updated
Scott Moser (smoser)
Changed in systemd (Ubuntu Trusty):
status: New → Confirmed
Changed in systemd (Ubuntu Xenial):
status: New → Confirmed
Changed in systemd (Ubuntu Yakkety):
status: New → Confirmed
Changed in systemd (Ubuntu Trusty):
importance: Undecided → High
Changed in systemd (Ubuntu Xenial):
importance: Undecided → High
Changed in systemd (Ubuntu Yakkety):
importance: Undecided → High
Changed in systemd (Ubuntu Zesty):
importance: Undecided → High
Revision history for this message
Dan Streetman (ddstreet) wrote :

Regarding the workaround patch from comment 17, I should note that the workaround involves replacing all whitespace in the NVMe model or serial string with underscores. This matches what is done for the by-id symlinks for scsi, ata, and all other buses. However, another possibility is instead of underscore-replacing the whitespace, it is 'encoded' by replacing it with literal '\x20' (i.e. the characters '\', 'x', '2', '0'). This is done for whitespace in some other symlinks, e.g. LVM and UUID.

Because all existing bus types use underscore replacement already for their symlinks, I think it seems extremely likely that underscore replacement will be used upstream for NVMe as well. However if they don't, then this workaround would conflict with upstream, meaning the workaround would generate symlinks different than upstream udev without this workaround; to use the example from the description, this workaround would create:
/dev/disk/by-id/nvme-XYZ_Corp_NVMe_drive_SERIAL -> ../../nvme0n1
while upstream (if the literal-encoding approach is used) would create:
/dev/disk/by-id/nvme-XYZ\x20Corp\x20NVMe\x20drive_SERIAL -> ../../nvme0n1

until my patch to upstream is either accepted or rejected (and some other fix used), it's impossible to tell with 100% certainty what approach upstream will decide on.

Revision history for this message
Dan Streetman (ddstreet) wrote :

My pull request has been merged upstream, the commits are:

0a10235 udev-rules: perform whitespace replacement for symlink subst values
e20a917 udev-event: add replace_whitespace param to udev_event_apply_format
a9d99b3 libudev-util: change util_replace_whitespace to return number of chars in dest

from https://github.com/systemd/systemd.git

Revision history for this message
Dan Streetman (ddstreet) wrote :
Revision history for this message
Dan Streetman (ddstreet) wrote :

Pull request for trusty:
https://code.launchpad.net/~ddstreet/+git/systemd/+ref/trusty

Pull request for xenial:
https://code.launchpad.net/~ddstreet/+git/systemd/+ref/xenial

Yakkety can use the same patches as Xenial.

Zesty can use the same patches as upstream (from github, listed in comment 22).

Revision history for this message
Robie Basak (racb) wrote :

12:26 <slangasek> can I get expedited SRU review of the above systemd SRU? It's thought to address an emergent regression introduced by changes in NVME support in the kernel, and blocks being able to run MAAS-based CI against machines with NVME

12:26 <slangasek> there's an existing SRU in xenial-proposed which hasn't been verified; we should just stack them

12:28 <sil2100> I could, but I am but a newb so someone would anyone have to double-check before I can approve it

12:28 <sil2100> Since bdmurray said I still need to do some coordinated reviews for now

12:31 <sil2100> slangasek: are all those patches present in systemd 323-10 from zesty? Or is that one not affected?

12:35 <rbasak> Is rharper happy to have the aging clock and verification reset?

12:41 <rbasak> slangasek: I'm not convinced about the reason for the urgency here. It looks like NVMe support was added in an SRU in November, and was done wrong. So surely we need to take more time and care this time, not less?

12:42 <rbasak> How long has this had time to bake in Zesty? It's not marked Fix Released yet.

12:47 <slangasek> rbasak: the NVME support was added in the kernel publication cycle that released to -updates at the beginning of this year

Changed in systemd (Debian):
status: Unknown → New
Revision history for this message
Robie Basak (racb) wrote :
Download full text (3.5 KiB)

There was further discussion in #ubuntu-devel and #ubuntu-release on
this review. See irclogs.ubuntu.com for details.

I have reviewed the patch carefully. As far as I can tell, it does what
it is supposed to do, and I don't see any bugs.

However, this code, particularly the udev_event_apply_format function,
seems to have grown organically and could be significantly improved
IMHO. This patch makes the state of the code worse, and increasingly
fragile. It took hours for me to review the patches for correctness as a
consequence, and I think that this and future changes and reviews are
far more prone to error than they need to be.

Some specifics:

1) The mechanism employed of redirecting s and l (destination buffer and
length remaining count) means that the code inside the redirection is
incredibly fragile. The code inside the switch should be pulled out into
a separate function. Then the redirection would become unnecessary. On
IRC, Dan acknowledged that this may require a large number of parameters
to be provided to the factored out function, but this is in part my
point. Unintended interactions could happen through any of those
parameters. Isolating them would be helpful, since that would identify
and reduce the interactions that need to be checked.

2) IMHO, the patch should use the sizeof(x)/sizeof(x[0]) idiom to get
the buffer lengths, rather than assuming that they are defined by the
same macro. I think likely that someone will attempt to change the array
length by assuming it is only necessary to change it in one place. Dan
suggested on IRC that the opposite point is that changing it from a
buffer to a pointer would cause the sizeof idiom to break, but I think
it's far more likely that a developer will check how a variable is used
before changing its type.

3) The upstream PR refers to tests, but does not add a test for this bug
being fixed. Given the fragility of this fix, I think we should do that.

We have to maintain this code for another four years in Xenial. If we
have to touch this function again in that time, I think we'll need to
spend even more time fixing and reviewing it.

I realise that the fix has already been accepted upstream. But as we (in
Ubuntu) originated the patch, I think we have a responsibility to make
it cleaner. Please refactor it and send that upstream.

I understand that this SRU is necessary to fix an important regression
in Ubuntu, so I don't think it's appropriate to block this given that I
can't find a bug in it. However, I've written this up to register my
unhappiness with this patch.

I appreciate the previous state of the code made it difficult to fix in
a better way; IMHO, this is a reason to refactor it first when sending
the development fix upstream. Perhaps the SRU would then need to have
been a different, more minimal and hacky patch, but that could be
focused on minimising regression risk and fixing just the problem at
hand rather than the general case.

If you add a test for this bug, please bundle it with the next SRU of
this package in Xenial.

I note that Zesty is not Fix Released. However, since this is a
regression fix, I don't think it would be appropriate to wait. Please
get the fix land...

Read more...

Changed in systemd (Ubuntu Xenial):
status: Confirmed → Fix Committed
tags: added: verification-needed
tags: added: regression-update
removed: verification-needed
Revision history for this message
Robie Basak (racb) wrote : Please test proposed package

Hello Dan, or anyone else affected,

Accepted systemd into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/systemd/229-4ubuntu15 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in systemd (Ubuntu Yakkety):
status: Confirmed → Fix Committed
tags: added: verification-needed
Revision history for this message
Robie Basak (racb) wrote :

Hello Dan, or anyone else affected,

Accepted systemd into yakkety-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/systemd/231-9ubuntu3 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Revision history for this message
Dan Streetman (ddstreet) wrote :

Xenial:

with udev 229-4ubuntu13, the /dev/disk/by-id/ NVMe symlinks were not correctly created:

$ ls -l /dev/disk/by-id/
total 0
lrwxrwxrwx 1 root root 13 Jan 13 22:01 nvme-Amazon -> ../../nvme0n1

and with udev 229-4ubuntu15 from -proposed, the /dev/disk/by-id/ NVMe symlinks were correctly created for all NVMe drives:

$ ls -l /dev/disk/by-id/
total 0
lrwxrwxrwx 1 root root 13 Jan 13 22:08 nvme-Amazon_EC2_NVMe_Instance_Storage_AWS1318514BBE1749B07 -> ../../nvme2n1
lrwxrwxrwx 1 root root 13 Jan 13 22:08 nvme-Amazon_EC2_NVMe_Instance_Storage_AWS14A8A28D6E51FDF6D -> ../../nvme0n1
lrwxrwxrwx 1 root root 13 Jan 13 22:08 nvme-Amazon_EC2_NVMe_Instance_Storage_AWS6318514BBE1749B07 -> ../../nvme3n1
lrwxrwxrwx 1 root root 13 Jan 13 22:08 nvme-Amazon_EC2_NVMe_Instance_Storage_AWS64A8A28D6E51FDF6D -> ../../nvme1n1

Revision history for this message
Dan Streetman (ddstreet) wrote :

Yakkety:

with udev 231-9ubuntu2, the /dev/disk/by-id/ NVMe symlinks were not created correctly:

$ ls -l /dev/disk/by-id/
total 0
lrwxrwxrwx 1 root root 13 Jan 13 22:34 nvme-Amazon -> ../../nvme2n1

and with udev 231-9ubuntu3, the /dev/disk/by-id/ NVMe symlinks are created correctly:

$ ls -l /dev/disk/by-id/
total 0
lrwxrwxrwx 1 root root 13 Jan 13 22:38 nvme-Amazon_EC2_NVMe_Instance_Storage_AWS1AB7AEFBFB76BA92D -> ../../nvme2n1
lrwxrwxrwx 1 root root 13 Jan 13 22:38 nvme-Amazon_EC2_NVMe_Instance_Storage_AWS1D210FA708BA29450 -> ../../nvme0n1
lrwxrwxrwx 1 root root 13 Jan 13 22:38 nvme-Amazon_EC2_NVMe_Instance_Storage_AWS6AB7AEFBFB76BA92D -> ../../nvme3n1
lrwxrwxrwx 1 root root 13 Jan 13 22:38 nvme-Amazon_EC2_NVMe_Instance_Storage_AWS6D210FA708BA29450 -> ../../nvme1n1

tags: added: verification-done-xenial verification-done-yakkety
removed: verification-needed
tags: added: verification-done
Revision history for this message
Steve Langasek (vorlon) wrote :

the fix for zesty is in -proposed, currently only held up by autopkgtest flakiness.

Changed in systemd (Ubuntu Zesty):
status: Confirmed → Fix Committed
Revision history for this message
Steve Langasek (vorlon) wrote :

Hello Dan, or anyone else affected,

Accepted systemd into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/systemd/229-4ubuntu16 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

tags: removed: verification-done
tags: added: verification-needed
Revision history for this message
Dan Streetman (ddstreet) wrote :

Verified with udev 229-4ubuntu16, all /dev/disk/by-id/ NVMe symlinks are present and correct.

tags: removed: verification-needed
tags: added: verification-done
Revision history for this message
Steve Langasek (vorlon) wrote : Update Released

The verification of the Stable Release Update for systemd has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package systemd - 229-4ubuntu16

---------------
systemd (229-4ubuntu16) xenial; urgency=medium

  * d/p/0001-libudev-util-change-util_replace_whitespace-to-retur.patch,
    d/p/0002-udev-event-add-replace_whitespace-param-to-udev_even.patch,
    d/p/0003-udev-rules-perform-whitespace-replacement-for-symlin.patch:
    Cherry-pick upstream fixes from Dan Streetman <email address hidden> to
    fix by-id symlinks for devices whose IDs contain whitespace.
    LP: #1647485.

 -- Steve Langasek <email address hidden> Wed, 18 Jan 2017 13:37:19 -0800

Changed in systemd (Ubuntu Xenial):
status: Fix Committed → Fix Released
Revision history for this message
Robie Basak (racb) wrote :

Could someone please look at the autopkgtest regressions in http://people.canonical.com/~ubuntu-archive/pending-sru.html against systemd please?

Revision history for this message
Dan Streetman (ddstreet) wrote :

> Could someone please look at the autopkgtest regressions in http://people.canonical.com/~ubuntu-
> archive/pending-sru.html against systemd please?

I'll ignore the no-longer-supported LTS kernel tests, so that leaves:

> Regression in autopkgtest for network-manager (ppc64el): test log

This has been failing since late 2015, with the same failure each time.
http://autopkgtest.ubuntu.com/packages/n/network-manager/trusty/ppc64el

> Regression in autopkgtest for linux-lts-xenial (ppc64el): test log
> Regression in autopkgtest for linux-lts-xenial (i386): test log
> Regression in autopkgtest for linux-lts-xenial (armhf): test log

These are failing either because the running and test kernel versions don't match, or because the test timed out. Again, they appear to have been failing for a long time; no new failures for the latest systemd.

> Regression in autopkgtest for ubuntu-drivers-common (i386): test log

another test that's failed for a long time in the same way each time. nothing new.

> Regression in autopkgtest for umockdev (armhf): test log

this fails with:
ERROR:tests/test-umockdev-record.c:706:t_system_single: assertion failed (_tmp10_ == ""): ("Cannot access device /dev/loop0: No such file or directory\n" == "")

it's not clear why there's no /dev/loop0, and previous test runs didn't fail.

> Regression in autopkgtest for open-iscsi (armhf): test log

this has failed the same way for the last 6 tests, because open-iscsi isn't running. Not sure what introduced that, but it happened before the last systemd update.

> Regression in autopkgtest for upstart (amd64): test log

this fails with:
test_state: tests/test_state.c:4048: test_upstart_with_apparmor_upgrade: Assertion `(state_from_string (json_string)) == 0' failed.

obviously not a helpful test failure message.

Revision history for this message
Dan Streetman (ddstreet) wrote :

So the only new autopkgtest failures are:

> Regression in autopkgtest for umockdev (armhf): test log

this fails with:
ERROR:tests/test-umockdev-record.c:706:t_system_single: assertion failed (_tmp10_ == ""): ("Cannot access device /dev/loop0: No such file or directory\n" == "")

this is the first test failure on trusty/armhf:
http://autopkgtest.ubuntu.com/packages/umockdev/trusty/armhf

however, it's failed for xenial, yakkety, and zesty on armhf for a long time, with the same failure:
http://autopkgtest.ubuntu.com/packages/umockdev/xenial/armhf
http://autopkgtest.ubuntu.com/packages/umockdev/yakkety/armhf
http://autopkgtest.ubuntu.com/packages/umockdev/zesty/armhf

so it seems like whatever was causing the failures for X/Y/Z on armhf is now also causing the failure for trusty on armhf; I think it's unlikely a systemd change caused the failure.

> Regression in autopkgtest for upstart (amd64): test log

this fails with:
test_state: tests/test_state.c:4048: test_upstart_with_apparmor_upgrade: Assertion `(state_from_string (json_string)) == 0' failed.

Closer looks shows it's complaining about the JSON data, the test was expecting a comment:

(null): Detected invalid serialisation data: expected comment
test_state: tests/test_state.c:4048: test_upstart_with_apparmor_upgrade: Assertion `(state_from_string (json_string)) == 0' failed.

I can't tell why it's failing now but not before, but it's also hard to see any relation between a systemd/udev update and upstart JSON serialization failure.

Changed in maas-images:
status: New → Fix Released
Revision history for this message
Dan Streetman (ddstreet) wrote :

> 1) The mechanism employed of redirecting s and l (destination buffer and
> length remaining count) means that the code inside the redirection is
> incredibly fragile. The code inside the switch should be pulled out into
> a separate function. Then the redirection would become unnecessary.

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

> 2) IMHO, the patch should use the sizeof(x)/sizeof(x[0]) idiom to get
> the buffer lengths, rather than assuming that they are defined by the
> same macro.

that code was removed/refactored by pull request 5168

> 3) The upstream PR refers to tests, but does not add a test for this bug
> being fixed. Given the fragility of this fix, I think we should do that.

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

> If you add a test for this bug, please bundle it with the next SRU of
> this package in Xenial.

once all of my pull requests for this are merged I'll open a new lp bug to track SRUing them.

Revision history for this message
Dan Streetman (ddstreet) wrote :

Has this been committed into trusty yet? I rebased my trusty pull request from comment 24; those patches will apply cleaning into the trusty systemd repository. I'm not sure if there is any other bug holding up trusty.

Changed in systemd (Ubuntu Trusty):
assignee: nobody → Dimitri John Ledkov (xnox)
milestone: none → trusty-updates
Changed in systemd (Ubuntu Trusty):
status: Confirmed → In Progress
Revision history for this message
Andy Whitcroft (apw) wrote : Please test proposed package

Hello Dan, or anyone else affected,

Accepted systemd into trusty-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/systemd/204-5ubuntu20.24 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in systemd (Ubuntu Trusty):
status: In Progress → Fix Committed
tags: removed: verification-done
tags: added: verification-needed
Revision history for this message
Dan Streetman (ddstreet) wrote :

with udev version 204-5ubuntu20.22 the /dev/disk/by-id/ symlinks are incorrect:

ubuntu@ip-172-31-27-212:/dev/disk/by-id$ apt list --installed udev
Listing... Done
udev/trusty-updates,now 204-5ubuntu20.22 amd64 [installed,upgradable to: 204-5ubuntu20.24]

ubuntu@ip-172-31-27-212:/dev/disk/by-id$ ls -l
total 0
lrwxrwxrwx 1 root root 13 Feb 7 20:05 nvme-Amazon -> ../../nvme0n1

with udev version 204-5ubuntu20.24, symlinks are created correctly:

ubuntu@ip-172-31-27-212:/dev/disk/by-id$ apt list --installed udev
Listing... Done
udev/trusty-proposed,now 204-5ubuntu20.24 amd64 [installed]
ubuntu@ip-172-31-27-212:/dev/disk/by-id$ ls -l
total 0
lrwxrwxrwx 1 root root 13 Feb 7 20:10 nvme-Amazon_EC2_NVMe_Instance_Storage_AWS1A53C9B72B4551070 -> ../../nvme0n1
lrwxrwxrwx 1 root root 13 Feb 7 20:10 nvme-Amazon_EC2_NVMe_Instance_Storage_AWS6A53C9B72B4551070 -> ../../nvme1n1

tags: added: verification-done verification-done-trusty
removed: verification-needed
Revision history for this message
Robie Basak (racb) wrote :

It doesn't look like 231-9ubuntu3 in Yakkety has been verified?

tags: removed: verification-done
Revision history for this message
Steve Langasek (vorlon) wrote :

yakkety was already covered in comment #30.

tags: added: verification-done
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package systemd - 204-5ubuntu20.24

---------------
systemd (204-5ubuntu20.24) trusty; urgency=medium

  [ Thomas Voß ]
  * Do not create /run/nologin, and thus make sure deputy systemd does not
    prevent system logins. LP: #1660573.

systemd (204-5ubuntu20.23) trusty; urgency=medium

  * d/p/0001-libudev-util-change-util_replace_whitespace-to-retur.patch,
    d/p/0002-udev-event-add-replace_whitespace-param-to-udev_even.patch,
    d/p/0003-udev-rules-perform-whitespace-replacement-for-symlin.patch:
    Cherry-pick upstream fixes from Dan Streetman <email address hidden> to
    fix by-id symlinks for devices whose IDs contain whitespace.
    LP: #1647485.

 -- Dimitri John Ledkov <email address hidden> Thu, 02 Feb 2017 10:29:19 +0000

Changed in systemd (Ubuntu Trusty):
status: Fix Committed → Fix Released
Revision history for this message
Martin Pitt (pitti) wrote :

I cherry-picked the patches into the Debian packaging branch, so that on next upload zesty can be synced again.

Changed in systemd (Debian):
status: New → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package systemd - 232-17ubuntu1

---------------
systemd (232-17ubuntu1) zesty; urgency=medium

  * debian/patches/0001-resolved-follow-CNAMES-for-DNS-stub-
    replies.patch: cherry-pick upstream fix for following CNAMEs in DNS
    stub replies. Closes LP: #1647031.

 -- Steve Langasek <email address hidden> Sun, 12 Feb 2017 22:54:55 -0800

Changed in systemd (Ubuntu Zesty):
status: Fix Committed → Fix Released
Louis Bouchard (louis)
tags: removed: sts-sponsor
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package systemd - 231-9ubuntu3

---------------
systemd (231-9ubuntu3) yakkety; urgency=medium

  * d/p/0001-libudev-util-change-util_replace_whitespace-to-retur.patch,
    d/p/0002-udev-event-add-replace_whitespace-param-to-udev_even.patch,
    d/p/0003-udev-rules-perform-whitespace-replacement-for-symlin.patch:
    Cherry-pick upstream fixes from Dan Streetman <email address hidden> to
    fix by-id symlinks for devices whose IDs contain whitespace.
    LP: #1647485.

 -- Steve Langasek <email address hidden> Fri, 13 Jan 2017 16:22:48 +0200

Changed in systemd (Ubuntu Yakkety):
status: Fix Committed → Fix Released
Louis Bouchard (louis)
tags: added: sts-sru-done
removed: sts-sru
Changed in systemd:
importance: Unknown → Undecided
importance: Undecided → Unknown
status: New → Unknown
Changed in systemd:
status: Unknown → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.