lvm: exclude missing PVs but mark VGs are partial if they have one#165
lvm: exclude missing PVs but mark VGs are partial if they have one#165ogayot merged 1 commit intocanonical:mainfrom
Conversation
Chris-Peterson444
left a comment
Chris-Peterson444
left a comment
There was a problem hiding this comment.
A couple of comments but this looks good to me. I agree this sounds like a better fix for probert rather than curtin. Thanks!
| VGS_REPORT = [ | ||
| { | ||
| "vg_name": "vg1", | ||
| "pv_name": "/dev/vda5", |
There was a problem hiding this comment.
nit: is there a reason to not put a newline between these two entries (here and elsewhere)?
There was a problem hiding this comment.
No, besides saving a few lines. But I'll have them on their own lines.
probert/lvm.py
Outdated
| # If the PV is missing the value is set typically set to "missing" | ||
| # but LVM2 checks for empty strings. | ||
| if report['pv_missing']: | ||
| missing_pvs += 1 | ||
|
|
||
| if not report['pv_missing']: | ||
| devices.add(report.get('pv_name')) |
There was a problem hiding this comment.
question: what is the nuance with "...but LVM2 checks for empty strings" ?
Reading this I expect that the pv_missing key is always there and we only care if it's not empty, right?
So could this check be the following instead?
| # If the PV is missing the value is set typically set to "missing" | |
| # but LVM2 checks for empty strings. | |
| if report['pv_missing']: | |
| missing_pvs += 1 | |
| if not report['pv_missing']: | |
| devices.add(report.get('pv_name')) | |
| # If the PV is missing the value is set typically set to "missing" | |
| # but LVM2 checks for empty strings. | |
| if report['pv_missing']: | |
| missing_pvs += 1 | |
| else: | |
| devices.add(report.get('pv_name')) |
There was a problem hiding this comment.
Yes exactly. I must have reordered instructions and didn't realize I was basically in a if something else ... construct.
Fixed, thanks!
probert/lvm.py
Outdated
| 'devices': sorted(list(devices)), | ||
| 'size': size}) | ||
| 'size': size, | ||
| 'partial': missing_pvs >= 1}) |
There was a problem hiding this comment.
| 'partial': missing_pvs >= 1}) | |
| 'partial': missing_pvs > 0}) |
I find this slightly easier to read
Previously when dealing with a VG that had missing PV(s), probert storage would
return something like:
"lvm": {
"logical_volumes": { [...] },
"physical_volumes": {"ubuntu-vg": ["/dev/sda", "[unknown]"]},
"volume_groups": {
"ubuntu-vg": {
"name": "ubuntu-vg", "devices": ["/dev/sda", "[unknown]"],
"size": "234567890B"
}
}
We now remove [unknown] entries (which correspond to missing PVs) but mark a VG
that has missing PVs as "partial: true":
"lvm": {
"logical_volumes": { [...] },
"physical_volumes": {"ubuntu-vg": ["/dev/sda"]},
"volume_groups": {
"ubuntu-vg": {
"name": "ubuntu-vg", "devices": ["/dev/sda"],
"size": "234567890B", "partial": true
}
}
This should prevent curtin from failing to validate the storage config. If need
be, we can add special handling for VGs that have "partial: true".
LP: #2142196
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
3 participants
Previously when dealing with a VG that had missing PV(s), probert storage would return something like:
And that would fail curtin storage-config validation because
[unknown]does not correspond to a valid block dev.And while curtin could explicitly treat
[unknown], it feels better to handle it at probert's level.We now remove
[unknown]entries (which correspond to missing PVs) but mark a VG that has missing PVs as "partial: true":This should prevent curtin from failing to validate the storage config. If need be, we can add special handling for VGs that have
partial: truein Subiquity and/or curtin.LP:#2142196