harvester: [BUG] Automatic disk provisioning result in unusable ghost disks on NVMe drives

@lanfon72 @weihanglo I think we can consider adding it as a note to the harvester doc, but the qemu-kvm is not for production use, will only target it for dev/testing purposes.

After another investigation, the root cause of this issue is “forgetting to persist the change of blockdevice CR after partitioning”.

Consider this part, the forceFormatDisk returns a newDevice but the caller forgets to persist it. To solve this, we only need patch as the following,

  if newDevice, err := c.forceFormatDisk(deviceCpy); err != nil {
  	return updateFormattingErrorCondition(deviceCpy, err)
  } else if newDevice.Name != deviceCpy.Name {
  	// forceFormatDisk may return a new device with new GPT label
  	// If in that case, just return instead of proceeding following update.
  	return newDevice, nil
+ } else {
+ 	deviceCpy = newDevice
  }

I’ve tested the issue under KVM with single-node harvester attached with twenty 20GB SCSI disks. Before this fix the issue is reproducible, but cannot be reproduced after that.

Note that there is still some condition update logic need to change accordingly. Also, since we are going to support direct provision of raw block device. The timeout problem of waiting for partitioning will also be eliminated.

@lanfon72 we may ask for some verification support after the v1.0.1-rc1 release, should be fine to keep this one on the testing stage.

I think it’s more than persisting the change. It’s still a race condition problem.

We’re passing a pointer to forceFormatDisk

First of all, because forceFormatDisk takes the pointer to the deviceCpy struct, deviceCpy is still being updated without assigning the returned value back to deviceCpy.

Why did ghost disks appear?

The appearance of ghost disks is caused by NDM keep repartitioning the disk, even if it’s been done before. Then, because every partitioning action would create a child partition with different PARTUUID, they are considered as “new” partitions, even if they ultimately represent the same thing: “the first partition of the disk, e.g. /dev/sda1”. These new partitions will be picked up by NDM and added as new BlockDevice custom resources, then provisioned onto Harvester/Longhorn.

Theoretically, after a new BlockDevice for the partition is created, the old BlockDevice representing the same thing should be deleted and unprovisioned from Harvester/Longhorn, but here it’s not, thus these old BlockDevice are showing as the “ghost disks”. I haven’t figured out why they are not deleted yet but I suspect it’s related to the repartitioning.

Reproducing

It seems like after forceFormatDisk succeeded, the information for indicating the disk has been successfully partitioned is not updated to the disk’s BlockDevice custom resource. Thus NDM would repartition the device again in the next scanning cycle.

After adjusting the disk scanner period and forceFormatDisk polling period, I can reproduce the disk repartitioning issue with one only disk. Here’s my explanation of how it occurred:

1. OnBlockDeviceChange detects the disk should be provisioned, then call forceFormatDisk to partition the disk.
2. forceFormatDisk executes the partitioning command to the disk, then waits for the child partition to appear, using wait.PollImmediate.
3. ScanBlockDevicesOnNode got triggered by a timer, while forceFormatDisk is still waiting for the child partition to appear. It scans the Node for the disk’s status, and updates the BlockDevice custom resource of the disk.
4. forceFormatDisk found the child partition, updated the deviceCpy struct using the pointer with necessary information to indicate the disk has been force-formatted. One critical piece of information is the LastFormattedAt field.
5. After forceFormatDisk finished execution, OnBlockDeviceChange eventually tries to update the disk’s BlockDevice custom resource with the deviceCpy struct modified by forceFormatDisk. However, because it’s been modified by ScanBlockDeviceOnNode in step 3, the update will fail due to the resource conflict issue:

   Operation cannot be fulfilled on blockdevices.harvesterhci.io \"7ad5288e45525a203d7c0b7ba1a348f8\": the object has been modified; please apply your changes to the latest version and try again"
   

   Because this update failed, LastFormattedAt will not be updated and will always be empty, so NDM would keep reformating the disk because of this emptiness.

Why we didn’t notice this update failure before?

We expect returning error here would cause Wrangler to log an error and requeue the event for retrying. However, no error messages were logged in our case here, so we never noticed the update is failing. We added a line of code to log the err and then realized it failed with resource conflict error.

Reference

• My mod to the code for reproducing this issue: https://github.com/johnliu55tw/node-disk-manager/commit/28580022869fd811ff6d5054063941a7c3cbf73f
• NDM Log generated by my mod: ndm-poll-longer.log

cc @weihanglo @yasker

There is a probable fix harvester/node-disk-manager#23 that I believe the re-entry of critical sections in blockdevices reconcilation is the root cause.

A quick simplified analysis, the node-disk-manager and its 30s ticker routine looks needing coordination and race control, especially those hardware related operations.

Another point is about the formatting and mounting of a disk with partitions (e.g. /dev/nvme4n1, /dev/nvme4n1p1), remove all partitions or keep those partitions or let user control. After formatting /dev/nvme4n1, it will also try to format /dev/nvme4n1p1.