ceph-volume zap: improve zapping to remove all partitions and all LVs, encrypted or not by alfredodeza · Pull Request #25330 · ceph/ceph

alfredodeza · 2018-11-29T17:15:44Z

In order to be able to destroy devices/partitions/lvs related to an OSD ID a major rework had to be done for zapping, that properly detects them and fully removes them. Introduces the ability to fully remove partitions.

Fixes some related issues like:

skip removal of non existing vgs (present when two devices are zapped that belong to the same vg)
fixes detection of devices in the Device class (adds tests)
adds a Device.vgs attribute to avoid having to fetch the vgs every time they are needed
sets partitions as a top level key when scanning disks, fixes a bug with partition scanning that was trying to enforce a '1' value in partition which was incorrect.
Adds a holders key to the partition device scanning, to help in encrypted status detection from a partition.
creates a udevadm helper to provide extra information about devices

Fixes: http://tracker.ceph.com/issues/37449

alfredodeza · 2018-11-29T17:48:33Z

src/ceph-volume/ceph_volume/util/disk.py

+    return _udevadm_parser(out)
+
+
+def _udevadm_parser(output):


@jan--f this work includes this version of the parser, I am not clear if we decided to keep pursuing PR #25201 or if that is on hold for now. In any case, I am happy to consume your version if that gets pushed through.

This helper is needed to determine the partition number so that we can remove partitions accordingly

#25201 is not on hold afaik, I just need to deal with the FreeBDS portion.

alfredodeza · 2018-11-29T22:16:57Z

jenkins test ceph-volume lvm

jan--f

Other then the contentious error handling this looks very useful. 👍

jan--f · 2018-11-30T08:08:24Z

src/ceph-volume/ceph_volume/util/disk.py

+    for line in output:
+        try:
+            key, value = line.split('=')
+        except ValueError:


I don't have anything against this except for the silent error swallowing. I know there is a log statement now but the code still continues with bad input.

Are you suggesting a hard fail? Or what would you propose here

We could add a logging statement to the terminal. I would prefer not have a hard fail. An non-parseable line should not prevent gathering metadata for a partition

Are you suggesting a hard fail? Or what would you propose here

Yes as discussed in #25201.

We could add a logging statement to the terminal. I would prefer not have a hard fail. An non-parseable line should not prevent gathering metadata for a partition

I think it very much should fail since we don't know at all what is in the output. One line failing (and being skipped) does not mean that the other parsed lines might no contain something that causes a bug somewhere else. When we detect that one line is not as expected, we should assume the whole invocation of udevadm, and thus the whole returned output is bad.

I am going to respectfully disagree here. I understand your concern, but this is type of parsing and handling of output is how we process pretty much everything else in ceph-volume.

That doesn't mean it is a good reason to introduce something that can be improved, but this type of parsing is good enough for what we need, and IMO we shouldn't fail hard. In any case, the consumer of the produced output can chose to fail hard (as we do for udevadm())

Look if this scenario ever happens this code logs that there is something wrong with the output and then continue anyway with this unknown output (there might be a '=' in one of the output lines). I think this is not useful behaviour and potentially dangerous.

The current usage is just to retrieve the partition number. I see your point of potential breakage with a = in the value side of things.

That doesn't represent a dangerous behavior for determining partition numbers, but I agree that it might be better to have a strict mode where the caller may set a hard failure in case there are errors in parsing.

I have created http://tracker.ceph.com/issues/37488 to follow up on that. The current use case does not warrant a strict mode.

#25201 is ready to merge. We might as well just go with that.

alfredodeza · 2018-11-30T15:37:28Z

jenkins test ceph-volume tox