abstract by-id device path in a function

[KaiRo-at]

We are using a fork of Rockstor for a system where /dev/nbd0 needs to be treated like a "normal" disk. That device has no by-id path though, so one piece of supporting that requires abstracting away the by-id paths into a function that can return either the by-id (for normal disks) or the "normal" path (for nbd).
For one thing, we'd like to stay as close to upstream if possible, so it would be good to have that patch in normal Rockstor. Also, this patch makes it easy to support non-standard disks in Rockstor in the future, which may be a wanted feature by itself. :)
Original patch by @schuellerf

[schakrava]

Can one of the admins verify this patch?

[schakrava]

Can one of the admins verify this patch?

[phillxnet]

@schakrava and @KaiRo-at (hello) I may have issues with this approach as on quick inspection I suspect it circumvents a key dependency (but only by way of comment suggestion). Please see a prior discussion on what I suspect is a similar approach to this 'problem' proposed previously by @greghensley in the following now closed issue:

#1400

Also note the technical doc on Rockstor device management:
https://forum.rockstor.com/t/device-management-in-rockstor/1768

There is very deep dependency on by-id names, the fact that they don't exist is certain circumstances is indicative of deeper issues. References to RedHat documentation advising on the approach I have taken re disk management and by-id names is available in the second of the above references.

@KaiRo-at Thanks for the contribution and looking at this rather messy arrangement I left behind and please give your opinion on the above links and how they may relate to your use of the network block device. I am not personally familiar with these or how they differ from regular block devices.

Also I am current very near the end of a rather large change set that makes quite a few changes to incorporate LUKS so I would appreciate if this pr could be held off until after those changes have been reviewed and hopefully applied, although 'as is' they looks harmless enough. But currently I have to concentrate on my current issue.

I am almost certain that the observed 'no by-id' name is due to a root cause of insufficiently udev rules to establish a devices serial and any change towards circumventing by-id name use will have very harmful effects. Or is this a peculiarity of network block devices. As can be seen we have seen the same 'no by-id' name whenever a device has poor udev support or a hyper visor fails to return a serial upon which the udev rules depend. An example of where we treated the cause rather than the simptom is evident in the upshoot udev rules published in:
https://forum.rockstor.com/t/bcache-developers-notes/2762
which was a response to the closed issue above (along with quite a few changes in how we managed bcache devices in our lsblk and udev probing as well). But ultimately once they have sufficient udev rules they could be identified via a by-id name.

However I see that here you have only abstracted the simple path string concatenation which is a welcome change. My main concern is any circumvention of the by-id name use such as your abstraction comments suggest. Be aware that the by-id name use as canonical device reference is prevalent and expanding within Rockstor and many elements of normal device management depend upon it.

Thanks again for contributing and do please feel free to engage on the by-id issue having read the above links. I appreciate that you are not advocating a circumvention but the comments suggest that it is a simple surface issue but it is not in all observed block devices to date due to the non boot safe nature of non by-id name use (as we use to use prior to the by-id switch) ie pr #1320 "revise internal use and format of device names. Fixes #1320 #1357" Is for example the nbd name absolutely boot safe, or can it change from one boot to the next as other block devices are prone to doing?

Thanks and welcome to the Rockstor contributors list.

[KaiRo-at]

However I see that here you have only abstracted the simple path string concatenation which is a welcome change.

Yes, that is the general idea, I feel it makes the code look better overall in addition to allowing a special-case handling in a central place, if required.

Is for example the nbd name absolutely boot safe, or can it change from one boot to the next as other block devices are prone to doing?

The way we are using it, it actually is indeed absolutely boot-safe as there will always be only one such device. That said, we are looking into a solution on the udev level and having our nbd server actually produce a "serial number" / unique ID, which ends up being safer overall if it works.

That said, do you want this abstraction still in the code after you land your LUKS work? If so, I'm happy to push it forward then.

[phillxnet]

@KaiRo-at Sorry for the slow response.
"Yes, that is the general idea, I feel it makes the code look better overall in addition to allowing a special-case handling in a central place, if required."

Agreed, it would make a great location to catch those times when the min sys requirements as per the pending issue in our rockstor-doc repo:
rockstor/rockstor-doc#157
were not met and no by-id link was generated on otherwise workable devices.

"The way we are using it, it actually is indeed absolutely boot-safe as there will always be only one such device."
I wonder if that same device name, if it's target became disconnected and then at a critical time period later reconnected if it would still have the same device name.

"That said, we are looking into a solution on the udev level and having our nbd server actually produce a "serial number" / unique ID, which ends up being safer overall if it works."

Sounds great, and hopefully some accompanying udev rule magic would make for a smoother fit all in.
There are actually quite a few more hard coded references to to by-id so once this is in place it will be nice to have it to address all those references as well (where appropriate of course as some do need to be hard coded). The by-id stuff is pretty baked in now as it's actually really useful, also note it's requirement in supporting partitions, but hopefully you aren't going that route.

"That said, do you want this abstraction still in the code after you land your LUKS work? If so, I'm happy to push it forward then."

I'm game definitely (I should have done this myself at the time), as long as it doesn't break anything. I am a little keen on having the comments contain a little more warning re it generally not being a good idea (the no by-id bit) as I'm concerned about receiving may pr's where people simply want to patch around supporting devices without a serial (and consequently no by-id name) and what is a sound principal anyway of observing boot safe names only: which is of course the road to ruin :) and their being disappointment etc. Also note that Rockstor's disk_scan and consequent db entries rely on a serial to track a device and if not available then a random one is assigned each time as a disaster avoidance and the user is presented with a warning which is for good reason. Anyway I think I can safely assume you are aware of this so I'll leave it at that.

This decision is ultimately down to @schakrava as project lead and merge master but at least my efforts on the LUKS work is now, for the time being, no longer holding up such contributions as yours as it has now been merged. I'm afraid this will required you to re-base though so sorry about that but hopefully it's worth it. I have used a few more hard coded reference there as well so sorry about that as well.

Cheers for you patience. The LUKS thing just took me much longer that I thought it was going to.

And do please consider submitting your network block device work also as I'm sure there will be quite a few how would be interested. Although I'm still sceptical of it's robustness, especially if it's name stable nature depends on there being only one. Must learn more about such things re nbd: perhaps from your future pull requests :).

Thanks.

[schakrava]

@KaiRo-at and @phillxnet thank you both for a candid and thoughtful discussion. As is evident by previous work on the disk management area, I too prefer the simpler udev rule solution in general. Not as a matter of opinion, but it just is a clean solution.

However, I do appreciate your contribution and would be more than willing to get to a place to merge it. Now that @phillxnet LUKS change is merged, if you could update this pr and make other changes suggested in the earlier discussion, I'd be happy to take another look at this pr.

I apologize for prolonging this, but I think we all agree that this is a critical code path.