Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rewrite device identification and initialization #1809

Merged
merged 15 commits into from
Feb 11, 2020
Merged

Rewrite device identification and initialization #1809

merged 15 commits into from
Feb 11, 2020

Conversation

mulkieran
Copy link
Member

@mulkieran mulkieran commented Feb 11, 2020 •
edited
Loading

Resolves: #1737.

Related: #1659.

Contributions are:

  • The initialize + resolve_devices functions are re-assembled and decomposed into two different functions, process_devices, which finds all necessary information about devices, and intialize_devices, which does the initialization. We expect this to allow some improvements to the implementation of idempotency in creating pools and adding devices, currently the code must do a complicated analysis of the results of calling BlockDevMgr::initialize(), with the new interface, implementing the idempotency should be much more simple and durable.
  • There are better error messages in many contexts.
  • There is no case where two file descriptors are simultaneously in use on the same device.
  • The code is less eager to do expensive operations, including opening files.
  • resolve_devices, along with its dependency on a devicemapper method is removed entirely and the necessary information is obtained from udev.
  • Some tests are tidied up and refined a bit. One new test is added.
  • The code is more modular. blockdevmgr.rs used to be the fourth largest module, at 851 lines. Since the split it is just over 500 lines, and devices.rs, which contains the device identification and initialization activities is also just over 500.

Supercedes #1764.

@mulkieran mulkieran self-assigned this Feb 11, 2020
@mulkieran mulkieran mentioned this pull request Feb 11, 2020
Closed
@mulkieran mulkieran force-pushed the develop-2.0.1-block-device-ownership branch from ee2060a to 4e68ddc Compare February 11, 2020 21:37
@mulkieran
Copy link
Member Author

rebased.

@mulkieran
Move udev module back down to backstore where it belongs
9eda469 
It had to be moved up a level because the udev functionality was being
used in strat_engine/engine.rs to decide the ownership in the
block_evaluate method. Now that functionality has been unified into
identify_block_device in the identify module in backstore, so the udev
module can be moved down again and be the identify module's sibling.

Signed-off-by: mulhern <[email protected]>
@mulkieran
No longer export DevOwnership from backstore
2769927 
It is no longer used outside backstore.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Tighten up get_udev_property and don't overdo the error string value
91336b6 
There isn't really any good identifying information for a udev database
entry, so it's better not to try to supply any.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Use a closure to sum up the problem when an error is returned
0955e87 
Make log entries in identify less wordy.

This also means that the error returned by blockdevmgr::initialize is
slightly more informative.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Return more context on failure to get ownership
3426095 
In blockdevmgr::initialize there was previously no context for failure
to determine ownership for a given devnode, now the context identifies the
devnode. This is helpful here, since the user may have specified that
devnode.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Split device module into two modules
9b62e13 
In the next few commits the device module will go away entirely,
while the devices module will take some method from the blockdevmgr
module.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Move some blockdevmgr methods into devices module
3fe34fc 
Move initialize and wipe_blockdevs. initialize is getting bigger and
needs room to breathe. It depends on wipe_blockdevs. All methods in this
module are about blockdevs before they are owned by Stratis.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Split out initialize-specific tests into devices module
de741b4 
Remove any unnecessary use of BlockDevMgr functionality.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Rewrite initialize function
5bb1394 
Remove device module entirely. It only contains DevOwnership, which
is now entirely unused.

The contributions of this rewrite are:
* Decompose the problem better. There are now two functions, one which
gets information about each device and another which initializes the devices.
Previously all this was in one big method, initialize. There are two
advantages:
  * The separated functions are a lot clearer.
  * The interface will allow the management of idempotency to be hoisted up
  the call chain to the engine. Currently, the idempotency treatment on
  create_pool or add must work by analyzing what happened, this will allow it
  to be much more direct and stable. In this commit, the check is moved up
  only one level, to blockdevmgr and abstracted into the function
  check_device_ids.
* It removes the DevOwnership enumerated type entirely. This type had been
designed to be used both when deciding identity to intialize devices and
when deciding whether a device should be included in a pool. This dual
purpose made it tough to use in either context, and it has already been
dropped from the identify module in a previous PR. Tests that made use of
DevOwnership were replaced with tests that made use of UdevOwnership.
* Make resolve_devices private, preparatory to removing it in a subsequent
commit.
* Improve error messages to provide context for various failures.
* Be less eager about performing relatively costly operations that get
results that may not be needed. For example, if udev indicates that the
device is owned by another, there is no need to get its size.
* Adds a FIXME marking the spot where we ought to verify that the device
is truly unowned using libblkid.
* Do not multiply file descriptors gratuitously. Previously, while one file
descriptor was being held on a device, another would be made in order to
read Stratis metadata. Now only one file descriptor is open at a time on
any given device.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Use udev to get the device number
5be3106 
Since using udev, it makes sense to use it to get the device number.
This allows getting rid of resolve_devices entirely, and avoid reading
filesystem metadata.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Move another test to devices module
ef5c6e8 
It only tests devices functionality, so that is the right place for it.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Tidy up the tests in devices.rs
9edf179 
Add some extra checks on the behavior of process_devices.
To better delineate the problem if the test should go wrong.
Use braces to separate tests parts.
Verify that device identfiers have been written by initialization in some
additional contexts.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Add a test for non-existent paths
31012f6 
On general principles.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Verify that identifying metadata has been written to devices in a test
d2f3009 
This test has intermittent failures; it fails to find the devices it should
using find_all. find_all() works, but it uses udev, and the udev database
does not seem to be properly updated, although it should be since we
use udev settle. Verify that the proper identifiers are on the devices
immediately after they are added to the backstore.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Use the fallback method for finding Stratis block devices
17f3fd0 
Do not expect udev to catch up in time.

Signed-off-by: mulhern <[email protected]>
@mulkieran mulkieran force-pushed the develop-2.0.1-block-device-ownership branch from 4e68ddc to 17f3fd0 Compare February 11, 2020 22:12
@mulkieran
Copy link
Member Author

rebased.

@mulkieran mulkieran merged commit 432754b into stratis-storage:develop-2.0.1 Feb 11, 2020
@mulkieran mulkieran deleted the develop-2.0.1-block-device-ownership branch February 11, 2020 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbaublitz jbaublitz jbaublitz approved these changes

@bgurney-rh bgurney-rh bgurney-rh approved these changes

@drckeefe drckeefe drckeefe approved these changes

@lleshchi lleshchi lleshchi approved these changes

Assignees

@mulkieran mulkieran

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@mulkieran @jbaublitz @bgurney-rh @drckeefe @lleshchi