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.

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

Revise internal use and format of device names #1320

Closed
phillxnet opened this Issue May 21, 2016 · 10 comments

Comments

Projects
None yet
3 participants
@phillxnet
Member

phillxnet commented May 21, 2016

In order to increase the flexibility and utility of the db Disk.name field it is proposed that it be re-deployed to hold by-id device names as opposed to the current ‘sd’ type names. This in combination with the Disk.role field is intended to aid in supporting additional storage options such as LUKS full disk encryption.

Ie change:
/dev/sdX derived from a db entry of sdX
to:
/dev/disk/by-id/wwid-for-device derived from a db entry of wwid-for-device.

A World Wide Identifier (WWID) is a persistent and system independent identifier required of all SCSI standards compliant devices.
Reference:-
https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Storage_Administration_Guide/persistent_naming.html

@phillxnet

This comment has been minimized.

Show comment
Hide comment
@phillxnet

phillxnet May 21, 2016

Member

I am currently working on this issue.

Member

phillxnet commented May 21, 2016

I am currently working on this issue.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 22, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 22, 2016

extend device by-id name helper to strip path if needed #1320
So as not to deal with paths in Disk.name db field we can
strip them first in our helper by way of a default flag that
leaves existing function intact.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 22, 2016

extend api/disk regex to include additional chars #1320
As /dev/disk/by-id names contain colons, underscores and
dashes we need to include these in our extended disk_regex
to accommodate the move to by-id names in the Disk.name
db field.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 22, 2016

use by-id disk names in db and consequent btrfs commands #1320
Initial conversion of disk name entries in db from 'sda' type to
those created by udev in the /dev/disk/by-id directory. Also
involves changing hard coded paths in low level btrfs functions.
@phillxnet

This comment has been minimized.

Show comment
Hide comment
@phillxnet

phillxnet May 22, 2016

Member
  • convert btrfs import mechanism to by-id name use.
  • convert APM level reporting to by-id name use.
  • convert Power State / spin down config and reporting to by-id name use.
  • convert SMART subsystem to by-id name use.
  • revise fake dev name for missing devices mechanism as now legitimate device name (by-id) and fake dev name collision is far more likely than before.
  • convert resize_pool to by-id name use.
  • resolve get_dev_byid_name returning null names ie for virtio without serial, this trips the db's "violates not-null constraint"
  • don't offer to use drives with no serial in pool creation wizard, as we can't track them.
  • don't collect SMART data on devices with no serial as since we key this to a name and with no serial that name is transient we end up with misdirected and consequently inappropriate / misleading data.
  • improve stability in multi by-id names for the same device as in some circumstances the names are reported in varying order. They always point to the same underlying (though transient) device name but greater utility would be had by stabilising the one we use: ie the longest.
  • debug disk removal from pool not 'sticking' ie disk gets removed but then shortly after the db is updated with on disk state which was not changed in the meantime. ie remove disk from pool not working.
  • don't offer to use drives with no serial in pool resize (add disks to pool) wizard.
  • address inefficiency in get_base_device_byid avoid using double conversion if we already have a by-id type device name, ie use surface syntactic method first then fall back to double conversion.
  • address various todo's concerning function location to better segregate by functional area and aid in future testing.
Member

phillxnet commented May 22, 2016

  • convert btrfs import mechanism to by-id name use.
  • convert APM level reporting to by-id name use.
  • convert Power State / spin down config and reporting to by-id name use.
  • convert SMART subsystem to by-id name use.
  • revise fake dev name for missing devices mechanism as now legitimate device name (by-id) and fake dev name collision is far more likely than before.
  • convert resize_pool to by-id name use.
  • resolve get_dev_byid_name returning null names ie for virtio without serial, this trips the db's "violates not-null constraint"
  • don't offer to use drives with no serial in pool creation wizard, as we can't track them.
  • don't collect SMART data on devices with no serial as since we key this to a name and with no serial that name is transient we end up with misdirected and consequently inappropriate / misleading data.
  • improve stability in multi by-id names for the same device as in some circumstances the names are reported in varying order. They always point to the same underlying (though transient) device name but greater utility would be had by stabilising the one we use: ie the longest.
  • debug disk removal from pool not 'sticking' ie disk gets removed but then shortly after the db is updated with on disk state which was not changed in the meantime. ie remove disk from pool not working.
  • don't offer to use drives with no serial in pool resize (add disks to pool) wizard.
  • address inefficiency in get_base_device_byid avoid using double conversion if we already have a by-id type device name, ie use surface syntactic method first then fall back to double conversion.
  • address various todo's concerning function location to better segregate by functional area and aid in future testing.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 23, 2016

update disk power state and hdparm setting readers to by-id #1320
Since the db now contains a by-id name convention we modify these
readers accordingly. Variable name re-factoring included for clarity.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 23, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 23, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 23, 2016

update disk spin down config wrapper to by-id device name type #1320
Also update is_rotational comments with possible improvement and
make existing comments more consistent with current function.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 23, 2016

include by-id name in skipping hdparm settings message #1320
Previously we just logged a generic message with no device name
embedded. Improve log message by adding in dev name by-id

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 23, 2016

update SMART options wrapper to by-id device name type #1320
Initially achieved by adding an additional wrapper to translate the
original get_base_device function so that it can be used with the new
by-id type name format.
Also corrected capitalization issue with recently added local variables.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 23, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 23, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 23, 2016

@phillxnet

This comment has been minimized.

Show comment
Hide comment
@phillxnet

phillxnet May 24, 2016

Member

From fs/btrfs/scan_disks we have:
"# 12 chars (fake-serial-) + 36 chars (uuid4) = 48 chars"
Given our move to much long by-id type names we should no longer just use the length of this serial as a flag for a device with no original serial number.
Checked by handlebar helper checkSerialStatus storageadmin/static/storageadmin/js/views/disks.js

Member

phillxnet commented May 24, 2016

From fs/btrfs/scan_disks we have:
"# 12 chars (fake-serial-) + 36 chars (uuid4) = 48 chars"
Given our move to much long by-id type names we should no longer just use the length of this serial as a flag for a device with no original serial number.
Checked by handlebar helper checkSerialStatus storageadmin/static/storageadmin/js/views/disks.js

@phillxnet

This comment has been minimized.

Show comment
Hide comment
@phillxnet

phillxnet May 24, 2016

Member

get_dev_byid_name looks the DEVLINKS line eg:-
DEVLINKS=/dev/disk/by-id/virtio-test-serial22222
in for example (for a virtio of vdd):-
udevadm info --query=property --name vdd
This is created by udev from the model "virtio" and the serial "test-serial22222"
In the case of a virtio without a serial ascribed to it we have no DEVLINKS as there is no serial for it to be drawn from!
This causes get_dev_byid_name to return None (by design) which has created the "violates not-null constraint" on Disk.name field. Previously we used the vdd device name but due to this missing DEVLINKS entry in the output of at least virtio devices without serial we need a graceful and meaningful failover.
I am leaning towards failing back, in this circumstance, to the original name.
Will pick this up again soon.

Member

phillxnet commented May 24, 2016

get_dev_byid_name looks the DEVLINKS line eg:-
DEVLINKS=/dev/disk/by-id/virtio-test-serial22222
in for example (for a virtio of vdd):-
udevadm info --query=property --name vdd
This is created by udev from the model "virtio" and the serial "test-serial22222"
In the case of a virtio without a serial ascribed to it we have no DEVLINKS as there is no serial for it to be drawn from!
This causes get_dev_byid_name to return None (by design) which has created the "violates not-null constraint" on Disk.name field. Previously we used the vdd device name but due to this missing DEVLINKS entry in the output of at least virtio devices without serial we need a graceful and meaningful failover.
I am leaning towards failing back, in this circumstance, to the original name.
Will pick this up again soon.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 24, 2016

improve robustness of fake-serial- detection #1320
Previously we only checked for a length of 48, improve
this by looking specifically for scan_disks stamp on all
fake-serial-uuid4's that it generates.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 24, 2016

add debug logging for issue #1320
Used to monitor get_dev_byid_name function.
@phillxnet

This comment has been minimized.

Show comment
Hide comment
@phillxnet

phillxnet May 24, 2016

Member

Disk.name by-id conversion mostly done and all disk and pool functions appear to be working, bar dealing with corner case serial issues.
I now just need to debugging (see above) and clean a few things up.

Member

phillxnet commented May 24, 2016

Disk.name by-id conversion mostly done and all disk and pool functions appear to be working, bar dealing with corner case serial issues.
I now just need to debugging (see above) and clean a few things up.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 25, 2016

if no by-id type disk name then fail over to caller name #1320
Disks with no serial number are not given a DEVLINKS entry by
udev ie virtio with no serial. This causes an issue as we need a
kernel name of sorts for every disk. Failing over to the calling
devices as found by scan_disks and flagging via an additional
return value (a boolean) associated via a tuple return type.
We can't use these no-serial devices anyway but do need to
have 'a' name in order to store and flag them.
Minor formatting updates and comment updates included.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 25, 2016

@holmesb

This comment has been minimized.

Show comment
Hide comment
@holmesb

holmesb May 25, 2016

This will be a great improvement. For one thing it will make installing in a XenServer VM possible using the normal method. Xen uses /dev/xvdx rather /dev/sdx device names, so currently have to use a workaround.

holmesb commented May 25, 2016

This will be a great improvement. For one thing it will make installing in a XenServer VM possible using the normal method. Xen uses /dev/xvdx rather /dev/sdx device names, so currently have to use a workaround.

@phillxnet

This comment has been minimized.

Show comment
Hide comment
@phillxnet

phillxnet May 25, 2016

Member

@holmesb Thanks for chipping in more info on this one; but I'm afraid this won't have any effect on the actual install, sorry. But I'm hoping it will have other 'good' effects. At least that's my aim.

Member

phillxnet commented May 25, 2016

@holmesb Thanks for chipping in more info on this one; but I'm afraid this won't have any effect on the actual install, sorry. But I'm hoping it will have other 'good' effects. At least that's my aim.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 25, 2016

disable SMART function for drives with transient names #1320
As we index the smart data to drive names and now these names
are less transient we have a chance to ensure that the SMART
data collected for a given drive will stick with that drive by using
only non transient names to index that data. So when we are
unable to retrieve a non transient name, ie when there is no serial
number and so no by-id name we disable the SMART data function.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 27, 2016

update pool device list generator to by-id device name type #1320
Minor variable name refactoring to aid readability and comments added to
assist in further refactoring if need be.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 28, 2016

improve robustness of fake-serial detection in db backend #1320
Previously we only looked for a length of 48 to confirm a fake serial
but this could give false positives so instead match for serials beginning
with 'fake-serial-' independent of length.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 28, 2016

make temporary placeholder names for detached devices more obvious #1320


By adding "detached-" to the beginning of placeholder device names we make
them more obvious in logs etc. Especially given the move to by-id type names
which are now much longer. Also easier to identify / parse in UI code as length
no longer a differentiating factor and contents was previously all random.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 28, 2016

add 12 zeros as unreliable disk serial on Disks page #1320
An Orico 4 disk usb 3.0 enclosure was found to obfuscate
all it's drives serial numbers with 12 zeros. This makes them
non unique but the first was labeled as OK. Given this is not
reliable flag all drives with this serial as having an unreliable
serial number.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 28, 2016

add disk serial viability check during pool creation #1320
When creating a pool don't offer up disks that have fake-serial
numbers assigned by scan_disks() as they will have unreliable
device names in the db. Also block 12 zeros as serial.
Code reformatted to use spaces not tabs.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 28, 2016

minor rewording on disks_table to match detached device naming #1320
In the future the term offline in the context of btrfs may be more confusing
so for the time being at least move to attached / detached / reattached as
we then move closer to removable / backup / import type device roles.
Also detached device names are renamed to start with 'detached-' to
indicate their status in logs and current table views.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 29, 2016

trivial code reformat to replace tabs with spaces in models.js #1320
Some extraneous trailing spaces also removed.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 29, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 29, 2016

only offer disks to add to a pool if their serial passes self validat…
…ion #1320

This way a disk with a fake-serial- added by scan_disks() because
none was found is not included in the list of disks offered when
adding to an existing pool.
Also removed console.log line.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 29, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 29, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 30, 2016

rationalize and improve stability of by-id name finder #1320
Ensure we actually find a by-id and always return the same one
in circumstances where there are multiple by-id entries by
always returning the longest or first if multiple by-id entries are of
equal length. Also improve code readability and rationalize
redundant pattern repetition.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue May 30, 2016

restore prior direct db Disk.name matching to conditionally disable S…
…MART #1320

Updates name matches to reflect by-id type names, removed temporary work
around that involved (expensive) conversion back to the previous name types
prior to by-id name conversion. This is not needed as by-id type names for the
target devices are equally as suited to direct name matching.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 1, 2016

reduce number of system calls required to gain base device name #1320
As the db field Disk.name now uses by-id type names we can skip
the prior lsblk system call and output parsing to find our base device
as by-id holds this information within it's surface syntax; so parse that
instead.
Also improve separation of SMART command pre-processor and
get base device system by moving the path addition to the
get_dev_options pre-processor and leaving the lower level process
with simple sting io.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

trivial code reformat to replace tabs with spaces in models.js #1320
Some extraneous trailing spaces also removed.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

only offer disks to add to a pool if their serial passes self validat…
…ion #1320

This way a disk with a fake-serial- added by scan_disks() because
none was found is not included in the list of disks offered when
adding to an existing pool.
Also removed console.log line.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

rationalize and improve stability of by-id name finder #1320
Ensure we actually find a by-id and always return the same one
in circumstances where there are multiple by-id entries by
always returning the longest or first if multiple by-id entries are of
equal length. Also improve code readability and rationalize
redundant pattern repetition.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

restore prior direct db Disk.name matching to conditionally disable S…
…MART #1320

Updates name matches to reflect by-id type names, removed temporary work
around that involved (expensive) conversion back to the previous name types
prior to by-id name conversion. This is not needed as by-id type names for the
target devices are equally as suited to direct name matching.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

reduce number of system calls required to gain base device name #1320
As the db field Disk.name now uses by-id type names we can skip
the prior lsblk system call and output parsing to find our base device
as by-id holds this information within it's surface syntax; so parse that
instead.
Also improve separation of SMART command pre-processor and
get base device system by moving the path addition to the
get_dev_options pre-processor and leaving the lower level process
with simple sting io.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

move remount from fs/btrfs to system/osi #1320
Not directly related to btrfs so moving to more general location.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

move convert_to_kib from fs/btrfs to system/osi #1320
Not directly related to btrfs so moving to more general location.
Added simple docstrings and renamed to comply with PEP8.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

move root_disk() from fs/btrfs to system/osi #1320
Not directly related to btrfs so moving to more general location.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

move wipe_disk() from fs/btrfs to system/osi #1320
Not directly related to btrfs so moving to more general location.
Added docstrings and did minor variable name refactoring.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

move blink_disk() from fs/btrfs to system/osi #1320
Not directly related to btrfs so moving to more general location.
Added docstrings and did minor variable name refactoring.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

move scan_disk() from fs/btrfs to system/osi #1320
Not directly related to btrfs so moving to more general location.
Switched to using existing const for lsblk binary location.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

remove some redundant import statements #1320
Also normalize import multi-line syntax in this file.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

disable SMART calls on nvme devices #1320
As reported by @Snafu on the Rockstor forum (Thanks)
the smartmontools project has only recently added
nvme support: https://www.smartmontools.org/ticket/657
So fixed in upstream but CentOS7's version of
smartmontools doesn't yet include this fix.
N.B. May require additional -d option filters.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

remove some debug logging used in issue #1320
Also some minor PEP8 formatting of long code lines.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

initial commit of transient to by-id name map function #1320
Lightweight method to create a map of all attached devices
sda type names to by-id type names. Intended primarily for
use by the Disk Activity widget.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

update Disk Activity widget to by-id name use #1320
By utilizing a name map created on every Disk Activity
widget instantiation the widget can quickly convert the
/proc/diskstats sda type names to the new by-id names
used in the db via a dict lookup.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

ensure the longest by-id name is used in get_byid_name_map() #1320
This removes the previous assumption that the longest by-id name for
a given device is always listed first as this was found to be incorrect.
Brings this function in line with the behaviour of get_dev_byid_name()
so as to maintain consistency. N.B. the context here is that some
devices have multiple by-id names so we take care to pick the longest
as it is more descriptive and gives repeatable naming.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

add TODO's for proposed code move to system/hdparm #1320
To split appart the now very large system/osi these TODO's
highlight a subset of code associated with drive APM and spin down
that can be moved to for example system/hdparm in another pr.
Included here for review.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jun 24, 2016

restore trivial quotes change in handlebars helper #1320
This was lost in a rebase / merge so restoring to match
upstream master.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jul 1, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jul 1, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jul 1, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jul 2, 2016

add get_dev_temp_name() function #1320
This function returns the the temporary canonical name
of a given by-id type name, or if an OSError is encountered
will fail over to returning the passed device name unchanged.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jul 2, 2016

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Jul 2, 2016

@schakrava schakrava closed this in #1357 Jul 4, 2016

schakrava added a commit that referenced this issue Jul 4, 2016

Merge pull request #1357 from phillxnet/1320_Revise_internal_use_and_…
…format_of_device_names

revise internal use and format of device names. Fixes #1320
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment