WIP: Allow setting file system UUIDs #537

aszlig · 2017-01-14T22:58:42Z

Implements setting UUIDs via passing new_uuid="..." to the format.

This is similar to setting labels, but while labels are set by just passing the label=something argument, this uses new_uuid=something because first of all, an UUID is not optional and we also have an uuid argument that's already used by the device tree populator to pass the UUID from the udev attributes.

File systems that support setting UUIDs are now ext2/3/4, FAT, NTFS, XFS, JFS, ReiserFS, HFS+ and swap. Not supported is HFS, because it doesn't seem to have a notion for setting serial or any kind of volume ID.

I'm heading for a hybrid approach (using UUIDs and partition layout holes) in nixpart for achieving storage tree determinism, so we need to have support for setting UUIDs. Blivet currently doesn't yet support this, so I've implemented it. Upstream pull request: storaged-project/blivet#537 Signed-off-by: aszlig <[email protected]>

aszlig · 2017-01-15T05:59:07Z

This is currently lacking support for setting UUIDs on BTRFS, but I'm working on that tomorrow. So please review but don't merge yet :-)

aszlig · 2017-01-17T03:05:44Z

s/tomorrow/weekend/

vojtechtrefny · 2017-01-17T07:32:26Z

Jenkins, ok to test.

vpodzime · 2017-01-19T13:01:24Z

Jenkins, test this, please.

vpodzime · 2017-01-19T13:07:41Z

These are the failures the CI tests report:
*** Running pylint ***
PYTHONPATH=.:tests/: tests/pylint/runpylint.py
************* Module /home/jenkins/workspace/blivet-2.x-PR/blivet/formats/fs.py
W1201(logging-not-lazy):399,12: FS._create: Specify string format arguments as logging function parameters
************* Module /home/jenkins/workspace/blivet-2.x-PR/blivet/tasks/fsuuid.py
W0402(deprecated-module): 3,0: : Uses of a deprecated module 'string'
************* Module /home/jenkins/workspace/blivet-2.x-PR/blivet/tasks/fsmkfs.py
E0102(function-redefined): 57,4: FSMkfs.get_uuid_args: method already defined line 51
************* Module /home/jenkins/workspace/blivet-2.x-PR/tests/formats_test/methods_test.py
E1101(no-member):303,12: FSMethodsTestCase._test_create_backend: Instance of 'DeviceFormat' has no '_mkfs' member
E1101(no-member):305,26: FSMethodsTestCase._test_create_backend: Instance of 'DeviceFormat' has no 'relabels' member
E1101(no-member):306,25: FSMethodsTestCase._test_create_backend: Instance of 'DeviceFormat' has no 'can_assign_uuid' member
Makefile:75: recipe for target 'pylint' failed

vpodzime · 2017-01-19T13:07:57Z

The results are not yet published anywhere, sorry. This should change next week.

Reported by @vpodzine at: storaged-project#537 (comment) These are the failures the CI tests report: *** Running pylint *** PYTHONPATH=.:tests/: tests/pylint/runpylint.py ************* Module /home/jenkins/workspace/blivet-2.x-PR/blivet/formats/fs.py W1201(logging-not-lazy):399,12: FS._create: Specify string format arguments as logging function parameters ************* Module /home/jenkins/workspace/blivet-2.x-PR/blivet/tasks/fsuuid.py W0402(deprecated-module): 3,0: : Uses of a deprecated module 'string' ************* Module /home/jenkins/workspace/blivet-2.x-PR/blivet/tasks/fsmkfs.py E0102(function-redefined): 57,4: FSMkfs.get_uuid_args: method already defined line 51 ************* Module /home/jenkins/workspace/blivet-2.x-PR/tests/formats_test/methods_test.py E1101(no-member):303,12: FSMethodsTestCase._test_create_backend: Instance of 'DeviceFormat' has no '_mkfs' member E1101(no-member):305,26: FSMethodsTestCase._test_create_backend: Instance of 'DeviceFormat' has no 'relabels' member E1101(no-member):306,25: FSMethodsTestCase._test_create_backend: Instance of 'DeviceFormat' has no 'can_assign_uuid' member Makefile:75: recipe for target 'pylint' failed The warning about deprecated-module seems to be a bug in pylint: https://www.logilab.org/ticket/2481 Instead of silencing the warning, I'm using the hex digit characters directly, because string.hexdigits contains both lower-case a-f and upper-case A-F hex characters. In our case however, we only want to match case-sensitive. Signed-off-by: aszlig <[email protected]>

@vpodzime

Reported by @vpodzime at: storaged-project#537 (comment) These are the failures the CI tests report: *** Running pylint *** PYTHONPATH=.:tests/: tests/pylint/runpylint.py ************* Module /home/jenkins/workspace/blivet-2.x-PR/blivet/formats/fs.py W1201(logging-not-lazy):399,12: FS._create: Specify string format arguments as logging function parameters ************* Module /home/jenkins/workspace/blivet-2.x-PR/blivet/tasks/fsuuid.py W0402(deprecated-module): 3,0: : Uses of a deprecated module 'string' ************* Module /home/jenkins/workspace/blivet-2.x-PR/blivet/tasks/fsmkfs.py E0102(function-redefined): 57,4: FSMkfs.get_uuid_args: method already defined line 51 ************* Module /home/jenkins/workspace/blivet-2.x-PR/tests/formats_test/methods_test.py E1101(no-member):303,12: FSMethodsTestCase._test_create_backend: Instance of 'DeviceFormat' has no '_mkfs' member E1101(no-member):305,26: FSMethodsTestCase._test_create_backend: Instance of 'DeviceFormat' has no 'relabels' member E1101(no-member):306,25: FSMethodsTestCase._test_create_backend: Instance of 'DeviceFormat' has no 'can_assign_uuid' member Makefile:75: recipe for target 'pylint' failed The warning about deprecated-module seems to be a bug in pylint: https://www.logilab.org/ticket/2481 Instead of silencing the warning, I'm using the hex digit characters directly, because string.hexdigits contains both lower-case a-f and upper-case A-F hex characters. In our case however, we only want to match case-sensitive. Signed-off-by: aszlig <[email protected]>

aszlig · 2017-01-19T16:16:25Z

@vpodzime: Thanks :-) The pylint warnings should be resolved now.

So far we had disabled the tests while referring to the NixOS VM test instead. However, it's desirable to run as much tests as we can, so let's run the pocketlint tests in checkPhase instead of skipping it altogether. In my case this is very useful because it would have caught a few errors during development of the UUIDs pull request: storaged-project/blivet#537 But even if we're not directly developing for the upstream project, this also catches Nix-related errors, such as references against pyanaconda which might still exist or exist again after an update. I'm using a list to accumulate find arguments because I wanted to avoid endless repetitions of -o -path xyz -prune. Signed-off-by: aszlig <[email protected]>

vpodzime

Other than the comments below, could you please squash the changes a bit? I'm okay with the final pylint fixes, but for example the commits that fix/change the label restrictions should be squashed to the commits that set the restrictions in the first place.

vpodzime · 2017-01-26T09:13:16Z

blivet/tasks/availability.py

@@ -298,6 +298,7 @@ def available_resource(name):
 MKFS_XFS_APP = application("mkfs.xfs")
 MKNTFS_APP = application("mkntfs")
 MKREISERFS_APP = application("mkreiserfs")
+MLABEL_APP = application("mlabel")


What about the fatlabel utility that's part of the dosfstools package?

fatlabel doesn't have the ability to set UUIDs yet, see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=563746 and its source at https://github.com/dosfstools/dosfstools/blob/820c2f90726db0468e0a684a5dc500fbde66466f/src/fatlabel.c.

vpodzime · 2017-01-26T09:18:11Z

blivet/tasks/fsuuid.py

+            return False
+        chunklens = [len(chunk) for chunk in chunks
+                     if all(char in hexdigits for char in chunk)]
+        return chunklens == [8, 4, 4, 4, 12]


Are these the only restrictions specified by the RFC? Or should this be using some regexp to limit the character sets as well instead?

vpodzime · 2017-01-26T09:22:23Z

blivet/formats/fs.py

@@ -86,6 +86,7 @@ def __init__(self, **kwargs):
            :keyword mountpoint: the filesystem's planned mountpoint
            :keyword label: the filesystem label
            :keyword uuid: the filesystem UUID
+            :keyword new_uuid: new UUID to set for this filesystem


I don't understand this. We know if the file system exists (in the system) or not (just in the model). If the UUID is set before it even exists, it means that it is requested to be set on creation, doesn't it? And other parts of blivet shouldn't care about these things, they just need to know the UUID of the file system when everything is created (for /etc/fstab,...)

Mhm, you're right, it makes sense to actually use the uuid keyword here.

vpodzime · 2017-01-26T09:24:20Z

blivet/formats/fs.py

@@ -251,6 +253,14 @@ def label_format_ok(self, label):
    label = property(lambda s: s._get_label(), lambda s, l: s._set_label(l),
                     doc="this filesystem's label")

+    def can_assign_uuid(self):


What about can_set_uuid?

This would have sounded odd for can_reassign_uuid: can_reset_uuid sounds a bit like "use the autogenerated UUID".

What about can_set_uuid and can_modify_uuid? (or change or edit for that matter).

vpodzime · 2017-01-26T09:25:36Z

blivet/formats/fs.py

@@ -261,6 +264,14 @@ def can_assign_uuid(self):
        """
        return self._mkfs.can_set_uuid and self._mkfs.available

+    def can_reassign_uuid(self):


And can_reset_uuid here?

vpodzime · 2017-01-26T09:31:01Z

blivet/formats/swap.py

@@ -21,8 +21,10 @@
 #

 from parted import PARTITION_SWAP, fileSystemType
+from ..errors import FSWriteUUIDError


Just a note on the commit message -- swap is not a file system. It's just a device format, similarly to LVM PV, LUKS or these things.

I know, the commit message says "other file systems" and I should have just used "file systems".

vpodzime · 2017-01-26T09:32:22Z

blivet/formats/swap.py

+        else:
+            if not self.uuid_format_ok(self.new_uuid):
+                raise FSWriteUUIDError("bad UUID format for swap filesystem")
+            extra = [blockdev.ExtraArg("-U", self.new_uuid)]


You should be able to just pass the {"-U": self.uuid} dictionary as extra making things look more pythonic.

Okay, will do that.

vpodzime · 2017-01-26T09:37:56Z

blivet/formats/fs.py

+            try:
+                self.write_uuid()
+            except FSError as e:
+                log.warning("Failed to write UUID (%s) for filesystem %s: %s",


This actually needs to (re)raise the exception. Otherwise it would go unnoticed. Nobody reads the logs.

Yeah, it makes even more sense to not log this in the first place but actually directly throw an exception.

vpodzime · 2017-01-26T09:39:11Z

tests/formats_test/fsuuid.py

@@ -37,13 +40,21 @@ class SetUUIDWithMkFs(SetUUID):
       native mkfs tool can set the UUID.
    """

-    def test_set_uuid(self):
+    @skipIf(sys.version_info < (3, 4), "assertLogs is not supported")


This is not needed if the method actually raises an exception.

vpodzime · 2017-01-26T09:45:52Z

There's also one more thing -- could you please check the 3.0-devel branch which contains changes from the #434 -- we will need a special action type for (re)setting UUID there.

These arguments are only relevant if we want to set a UUID during creation of file systems. Right now this doesn't implement any real functionality and only implements the interface and the options for various file systems. The reason I'm using get_uuid_args instead of just uuid_options is that for XFS, we need to use ["-m", "uuid=something"], so we don't have a single flag with a plain argument. Signed-off-by: aszlig <[email protected]>

This is more or less modelled the same way as tasks.fslabeling, consisting of classes that currently only contain a method for verifying whether an UUID is correct for a given file system or not. I'm using _uuidfs within the FS class (and subclasses) even though it sounds a bit odd, mainly because it is consistent with _labelfs and also because it avoids clashes with future _uuid attributes. For the _check_rfc4122_uuid() class method, I'm using string processing rather than uuid.UUID(), because first of all uuid.UUID() is much more complicated and it also allows to omit the dashes while the file system utilities may not allow this. Signed-off-by: aszlig <[email protected]>

We already have a uuid keyword argument, which might get passed if the format already exists, but if it exists on a new format, we can safely assume that the user wants the format to have a specific UUID and we use that instead of letting the mkfs utility generate a random UUID. The implementation for _create() deviates a bit from the scheme we had with labels so far, where we have a labeling() method which returns True if we can set a label either during or after the creation of a file system. In our case we now have a can_assign_uuid(), which *only* returns True if we can assign an UUID during creation. We will have another method soon which checks whether we can assign an UUID *after* creation. Also, using a similar naming scheme to labeling(), we would have ended up with something like uuiding() or any other weird name. IMHO setting this to can_assign_uuid() also has the advantage that the name in itself is more descriptive about what this method does. Signed-off-by: aszlig <[email protected]>

Some file systems don't have utilities for setting the UUID during creation, so we need to fall back to doing it afterwards in _post_create(). This introduces mlabel (for FAT) and tune2fs (for ext2/3/4) as new applications required in tasks/availability. The "mlabel" tool is required to set serials for FAT file systems, because dosfstools seems to only allow setting the volume ID during creation and doesn't have a command to change it for existing file systems. "fatlabel" only can set labels, but not UUIDs but "mlabel" can. Unfortunately, I haven't found a way to set HFS+ UUIDs after creation using the hfsprogs package, so I'm omitting HFS+ here for now. Signed-off-by: aszlig <[email protected]>

These are the more simple tests, because they only rely on pure functions. We're only possing a range of invalid and valid UUIDs and assert that the checkers (uuid_format_ok) return correctly. Signed-off-by: aszlig <[email protected]>

While we already had pure tests for just verifying whether an UUID is correct, let's now add the impure stuff that actually uses the various file system tools to write the UUIDs for real. Note that the fsuuid.py file does not use the same scheme than fslabeling.py, even though it might look similar. For UUIDs we assume here, that we can _always_ assign a new UUID after the file system has been created. Of course, this is not yet true for HFS+, because I haven't yet found a way to set the UUID afterwards. So the distinction between SetUUIDWithMkFs and SetUUIDAfterMkFs is that with the former we assume that the mkfs utility has a way to set the UUID, even though we have a _post_create() method that sets the UUID if that's not the case. Signed-off-by: aszlig <[email protected]>

This is quite similar to what we have for file systems, but doesn't rely on the fsuuid/fswriteuuid tasks except for the _check_rfc4122_uuid() method from fsuuid. Other than that the implementation is the same. We can't assign the UUID for a swap file system at a later point, so we only implement this during creation. Signed-off-by: aszlig <[email protected]>

So far we only have tested valid UUIDs properly, while we had a test using invalid UUIDs the test was more or less a no-op, because during creation no error is thrown if we use an invalid UUID. Rather than throwing an error the create() method logs a warning, which we now verify using assertLogs test_set_invalid_uuid(). Signed-off-by: aszlig <[email protected]>

aszlig · 2017-01-28T04:28:15Z

@vpodzime: I've addressed most of the points you've mentioned here:

Instead of using a new_uuid keyword argument, uuid is now re-used from DeviceFormat.
Reworded the commit that mentions swap in par with "other file systems".
A plain dict is now used instead of a list of blockdev.ExtraArg.
During _post_create() an FSError is now thrown instead of log.warning if writing an UUID should fail.
Squashed a few commits: The ones you mentioned fixing up the UUID format for FAT/NTFS, the ones adding mlabel and tune2fs and also the commit fixing the pylint warnings.

The following point is not yet addressed:

Rename can_assign_uuid() to can_set_uuid(), because renaming can_reassign_uuid() in par with it would lead to can_reset_uuid() which sounds more like "use the autogenerated UUID".

Also, even though I have squashed a few of the commits I personally think that squashing commits during review is a bad idea, because it's difficult for the reviewer to keep track on what's already been reviewed and what's left to review.

As for addressing 3.0-devel, should I create another PR that ports these changes over once this is no longer "WIP" or what is your approach here?

@vpodzime

This has been requested by @vpodzime in storaged-project/blivet#537 and it obviously didn't make sense to introduce a new keyword argument. IIRC the reason for introducing it in the first place was that it might conflict with the device tree populator. But when creating a new file system it doesn't matter because we don't go through the populator anyway and we also don't run _create on a pre-existing filesystem unless we really want to format it, but then it's fine to set the UUID. Signed-off-by: aszlig <[email protected]>

aszlig · 2017-01-28T05:01:44Z

Regarding setting UUIDs for BTRFS (well, not only BTRFS but StorageDevices in general): What is the correct place to place tests in? I'm asking because tests/format_tests seems to be a bit odd because BTRFS volumes are created using BTRFSVolumeDevice instead of using just the format.

BTRFSVolumeDevice implicitly set a format as well, so it would theoretically fit there and it would also have all tests regarding setting UUIDs in one place.

On the other hand creating a new file in tests/devices_test would also make sense because it would tie together all the UUID-setting tests for all devices and the UUID-setting tests for all formats in another place (which we already have).

vpodzime · 2017-02-06T13:09:47Z

Rename can_assign_uuid() to can_set_uuid(), because renaming can_reassign_uuid() in par with it would lead to can_reset_uuid() which sounds more like "use the autogenerated UUID".

Makes sense. I'm okay with the assign versions.

vpodzime · 2017-02-06T13:13:29Z

As for addressing 3.0-devel, should I create another PR that ports these changes over once this is no longer "WIP" or what is your approach here?

If it's possible, I'd like to have this PR finished, reviewed and merged to 2.1-devel and then to 3.0-devel by the merge of 2.1-devel into 3.0-devel and then have a new PR for 3.0-devel adding the extra features.

vpodzime · 2017-02-06T13:14:09Z

On the other hand creating a new file in tests/devices_test would also make sense because it would tie together all the UUID-setting tests for all devices and the UUID-setting tests for all formats in another place (which we already have).

Sounds good to me.

vpodzime · 2017-05-10T09:32:56Z

#573 contains changes from here and is merged.

I'm heading for a hybrid approach (using UUIDs and partition layout holes) in nixpart for achieving storage tree determinism, so we need to have support for setting UUIDs. Blivet currently doesn't yet support this, so I've implemented it. Upstream pull request: storaged-project/blivet#537 Signed-off-by: aszlig <[email protected]>

So far we had disabled the tests while referring to the NixOS VM test instead. However, it's desirable to run as much tests as we can, so let's run the pocketlint tests in checkPhase instead of skipping it altogether. In my case this is very useful because it would have caught a few errors during development of the UUIDs pull request: storaged-project/blivet#537 But even if we're not directly developing for the upstream project, this also catches Nix-related errors, such as references against pyanaconda which might still exist or exist again after an update. I'm using a list to accumulate find arguments because I wanted to avoid endless repetitions of -o -path xyz -prune. Signed-off-by: aszlig <[email protected]>

aszlig changed the title ~~Allow setting file system UUIDs~~ WIP: Allow setting file system UUIDs Jan 15, 2017

aszlig force-pushed the uuids branch from 9603083 to 9999f79 Compare January 19, 2017 16:15

vpodzime requested review from vpodzime and dwlehman January 20, 2017 09:14

vpodzime requested changes Jan 26, 2017

View reviewed changes

aszlig added 8 commits January 28, 2017 03:19

tests: Add tests to check the UUID format checkers

94ecf24

These are the more simple tests, because they only rely on pure functions. We're only possing a range of invalid and valid UUIDs and assert that the checkers (uuid_format_ok) return correctly. Signed-off-by: aszlig <[email protected]>

aszlig force-pushed the uuids branch from 9999f79 to e357807 Compare January 28, 2017 04:11

vpodzime mentioned this pull request Apr 12, 2017

Fixes for thin snapshot creation #573

Merged

vpodzime closed this May 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Allow setting file system UUIDs #537

WIP: Allow setting file system UUIDs #537

aszlig commented Jan 14, 2017

aszlig commented Jan 15, 2017

aszlig commented Jan 17, 2017

vojtechtrefny commented Jan 17, 2017

vpodzime commented Jan 19, 2017

vpodzime commented Jan 19, 2017

vpodzime commented Jan 19, 2017

aszlig commented Jan 19, 2017

vpodzime left a comment •

edited

Loading

vpodzime Jan 26, 2017

aszlig Jan 28, 2017

vpodzime Jan 26, 2017

vpodzime Jan 26, 2017

aszlig Jan 28, 2017

vpodzime Jan 26, 2017

aszlig Jan 28, 2017

dwlehman Feb 6, 2017

vpodzime Jan 26, 2017

vpodzime Jan 26, 2017

aszlig Jan 28, 2017

vpodzime Jan 26, 2017

aszlig Jan 28, 2017

vpodzime Jan 26, 2017

aszlig Jan 28, 2017

vpodzime Jan 26, 2017

vpodzime commented Jan 26, 2017

aszlig commented Jan 28, 2017

aszlig commented Jan 28, 2017

vpodzime commented Feb 6, 2017

vpodzime commented Feb 6, 2017

vpodzime commented Feb 6, 2017

vpodzime commented May 10, 2017

WIP: Allow setting file system UUIDs #537

WIP: Allow setting file system UUIDs #537

Conversation

aszlig commented Jan 14, 2017

aszlig commented Jan 15, 2017

aszlig commented Jan 17, 2017

vojtechtrefny commented Jan 17, 2017

vpodzime commented Jan 19, 2017

vpodzime commented Jan 19, 2017

vpodzime commented Jan 19, 2017

aszlig commented Jan 19, 2017

vpodzime left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpodzime commented Jan 26, 2017

aszlig commented Jan 28, 2017

aszlig commented Jan 28, 2017

vpodzime commented Feb 6, 2017

vpodzime commented Feb 6, 2017

vpodzime commented Feb 6, 2017

vpodzime commented May 10, 2017

vpodzime left a comment •

edited

Loading