feat: rework BD naming to avoid requiring WWNs#266
Open
tserong wants to merge 9 commits intoharvester:masterfrom
Open
feat: rework BD naming to avoid requiring WWNs#266tserong wants to merge 9 commits intoharvester:masterfrom
tserong wants to merge 9 commits intoharvester:masterfrom
Conversation
tserong
requested review from
a team,
Vicente-Cheng and
Yu-Jack
as code owners
tserong
force-pushed
the
wip-dont-require-wwn
branch
7 times, most recently
from
aa16be7 to
b800238
Compare
tserong
changed the title
tserong
force-pushed
the
wip-dont-require-wwn
branch
from
b800238 to
c6a02ec
Compare
tserong
force-pushed
the
wip-dont-require-wwn
branch
from
c6a02ec to
7a6aa91
Compare
This was referenced Mar 2, 2026
tserong
force-pushed
the
wip-dont-require-wwn
branch
2 times, most recently
from
c40a53c to
67fce5c
Compare
Member
Author
|
I know CodeFactor thinks |
|
This pull request is now in conflict. Could you fix it @tserong? 🙏 |
…ices harvester#253 updated BD CRs to use stable /dev/mapper paths for multipath devices. These paths need to be resolved back to /dev/dm-0 when calling GetDiskByDevPath(), otherwise fetching details like the WWN and Serial number from /sys/block will fail, which means we lose the device WWN, SerialNumber and Capacity information when multipath devices are provisioned. Signed-off-by: Tim Serong <tserong@suse.com>
Harvester hasn't supported adding additional partitioned disks since v1.0.2. Signed-off-by: Tim Serong <tserong@suse.com>
With debug logging turned on, you will periodically see something like the following: time="2026-02-19T09:55:30Z" level=debug msg="Prepare to checking block device ac1b8d0086729ec4840eac4e2319cfaf" time="2026-02-19T09:55:30Z" level=debug msg="Update block device ac1b8d0086729ec4840eac4e2319cfaf tags (Status) from [] to []" Note that it's triggering an update even though the tags don't appear to have changed (they're both rendered in the logs as empty slices). The problem is that bd.Status.Tags is specified as `json:"tags,omitempty"` but disk.Tags is specified as `json:"tags"`. This means that if both are empty, bd.Status.Tags will be nil, while disk.Tags will be an empty slice. In this case, reflect.DeepEqual() will return false, which will result in an unnecessary update. We can avoid this by skipping the DeepCopy and DeepEqual entirely if both bd.Status.Tags and disk.Tags are already empty. Signed-off-by: Tim Serong <tserong@suse.com>
The original implementation ran `blkid -s UUID`, `blkid -s PTUUID` and `blkid -s TYPE` for each device scanned. We can collapse this back to a single call if we invoke it once as `blkid -o export` then parse the output and turn it into a map of strings. Signed-off-by: Tim Serong <tserong@suse.com>
We can't really cope properly with disks that don't have WWNs, or whose
WWNs change (the latter should never happen, but it did at least once in
the past when a kernel update resulted in IDs for a certain make and
model of NVMe changing).
We can solve this as follows:
- Identify disks by, in order:
1. UUID if present (will be there for provisioned LHv1 volumes and
LVM volumes)
2. WWN if present
3. Vendor+Model+SerialNumber+BusPath if none of the above exist.
- BD CR names become non-deterministic (they're UUIDs for any new BDs
after this change), so when scanning we don't check existing BDs by
name, we check if there's one with the same UUID, then WWN, then
Vendor+Model+Serial+BusPath.
- This automatically prevents adding devices with duplicate FS/LVM UUIDs
or WWNs.
- It would be nice if we could also interrogate provisioned LHv2 devices
when scanned but that's rather more problematic (see details in
harvester/harvester#10115)
Related-to: harvester/harvester#6261
Related-to: harvester/harvester#8295
Signed-off-by: Tim Serong <tserong@suse.com>
Signed-off-by: Tim Serong <tserong@suse.com>
There's a couple of changes we need to make here: 1. We're actually starting with two blockdevices now, not one, because the new NDM code picks up /dev/vdb in the vagrant host 2. The duplicated WWN device needs a new backing file, otherwise it'll pick up the duplicate disk UUID, not the duplicate WWN. Signed-off-by: Tim Serong <tserong@suse.com>
There'll be more than the two expected block devices thanks for NDM now picking up /dev/vdb. It's more robust to check how many we started with then expect to end up with two more than whatever that original number was. Signed-off-by: Tim Serong <tserong@suse.com>
Signed-off-by: Tim Serong <tserong@suse.com>
tserong
force-pushed
the
wip-dont-require-wwn
branch
from
cc5f7cd to
54a514e
Compare
Member
Author
|
Conflicts resolved |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem:
We can't really cope properly with disks that don't have WWNs, or whose WWNs change (the latter should never happen, but it did at least once in the past when a kernel update resulted in IDs for a certain make and model of NVMe changing).
Solution:
Related Issue:
harvester/harvester#6261
harvester/harvester#8295
Test plan:
TBD
Note:
The first four commits in this PR aren't actually directly related to the naming rework. They're fixes for other issues that I found along the way while working on this.