fix: rework handleExistingDev

Signed-off-by: Tim Serong <tserong@suse.com>

Author: tserong

pkg/controller/blockdevice/scanner.go

@@ -126,88 +126,127 @@ func (s *Scanner) collectAllDevices() []*deviceWithAutoProvision {
 	return allDevices
 }
 
-// TODO: This function needs a bit of re-working:
-//   - There are two paths here where we re-active an inactive device,
-//     one is if the device path has changed, the other is if it's a
-//     multipath device.  But, it's possible for a device to go away,
-//     then come back with the original device path. This case isn't
-//     handled. We should try to consolidate all three cases if possible.
-//   - The isDevAlreadyProvisioned() check will always return false,
-//     because it's checking newBd, but should be checking oldDb (this
-//     doesn't actually cause any practical problems because the
-//     subsequent NeedsAutoProvision() check uses oldBd correctly, but
-//     it means the log messages aren't quite right).
-//   - I'm not actually sure the whole if/else structure here works
-//     properly.  What if a device was incative, and is now active,
-//     but _also_ needs to be autoprovisioned?  If I'm reading this
-//     correctly, that case won't be handled until the next scanner
-//     run (which might not be until the node is rebooted in the worst
-//     case).
-//   - No, I'm wrong, it's fine, that'll be picked up in OnBlockDeviceChanged
+// handleExistingDev will update an existing BD CR based on the current state
+// of an actual block device on the system.  It will return true if newBd
+// (the detected block device) really does correspond to oldBd (the BD CR),
+// and was potentially updated in some way, or false if newBd doesn't
+// actually correspond to oldBd.
 //
-// TODO: This needs to pick up all new status, incl. new UUID and WWN!!!
-// No, that's not quite true:
+// Note:
 //   - If the device was inactive before it's OK for its status to change,
 //     notably the device path.  It should also be fine if the buspath
 //     changes (maybe it got moved somwehere else)
 //   - Changes of vendor+model+serial would always be unexpected
 //   - Changes of UUID might be unexpected (unless a UUID got added due
 //     to a device being provisioned, but that shouldn't come by this
-//     code path.
+//     code path).
 //   - Changes of WWN might be unexepcted, except in that weird case where
 //     a kernel update changed the WWNs of existing disks.
-func (s *Scanner) handleExistingDev(oldBd *diskv1.BlockDevice, newBd *diskv1.BlockDevice, autoProvisioned bool) {
-	if isDevPathChanged(oldBd, newBd) {
-		// Dev Path will change when device is temporarily gone and back.
-		// Node reboot might also cause the device path change (depends on device interrupt).
-		// According to the above, we should update the device path once it changes.
-		if oldBd.Status.State == diskv1.BlockDeviceInactive {
-			logrus.Infof("The inactive block device %s with wwn %s is coming back", newBd.Name, newBd.Status.DeviceStatus.Details.WWN)
-			oldBd.Status.State = diskv1.BlockDeviceActive
-		}
-		logrus.Infof("Try to update the device path of %s, which is changed from %s to %s.", oldBd.Name, oldBd.Status.DeviceStatus.DevPath, newBd.Status.DeviceStatus.DevPath)
-		oldBd.Status.DeviceStatus.DevPath = newBd.Status.DeviceStatus.DevPath
-		if _, err := s.Blockdevices.Update(oldBd); err != nil {
-			logrus.Errorf("Update device %s status error, wake up scanner again: %v", oldBd.Name, err)
-			s.Cond.Signal()
+func (s *Scanner) handleExistingDev(oldBd *diskv1.BlockDevice, newBd *diskv1.BlockDevice, autoProvisioned bool) bool {
+	oldBdCp := oldBd.DeepCopy()
+
+	if oldBd.Status.State == diskv1.BlockDeviceActive {
+		// The BD is currently active. In this case the dev path shouldn't change, but
+		// it's possible that some other details may have changed (the UUID if someone
+		// manually created a filesystem on an unprovisioned device, or the WWN in
+		// rare cases where a kernel update messes with device naming)
+		if isDevPathChanged(oldBd, newBd) {
+			// The dev path for an active device has changed.  This means that newBd
+			// doesn't actually correspond to oldBd.  This should never happen in
+			// normal operation, but can happen if e.g. a multipath BD exists, but
+			// multipathd isn't started anymore.  In this case newBd will point to
+			// one of the raw devices.  The same is true in reverse.  Either way,
+			// we want to skip this and return false so that later the BD can be
+			// marked inactive or removed.
+			logrus.WithFields(logrus.Fields{
+				"name":      oldBd.Name,
+				"device":    oldBd.Status.DeviceStatus.DevPath,
+				"newDevice": newBd.Status.DeviceStatus.DevPath,
+			}).Warn("new device path detected for active device - skipping update")
+			return false
 		}
-	} else if isDevAlreadyProvisioned(newBd) {
-		logrus.Debugf("Skip the provisioned device: %s", newBd.Name)
-	} else if s.NeedsAutoProvision(oldBd, autoProvisioned) {
-		logrus.Debugf("Enqueue block device %s for auto-provisioning", newBd.Name)
-		s.Blockdevices.Enqueue(s.Namespace, newBd.Name)
-	} else if isDMDevice(oldBd.Status.DeviceStatus.DevPath) {
-		// Other dm devices should be filtered our at the beginning.
-		// So, we focus multipath device checking here.
-		// If system doesn't enable multipathd, the block device will become inactive after reboot.
-		// So, we need to add a checking after starting multipathd.
-		// Then the block device will become active from inactive.
-		oldBdCp := oldBd.DeepCopy()
-
-		// We've checked it outside, we can skip error
-		path, _ := filepath.EvalSymlinks(oldBd.Status.DeviceStatus.DevPath)
-		if _, err := utils.IsMultipathDevice(path); err == nil {
-			logrus.Infof("The multipath device %s is available, change it to active.", oldBd.Name)
+		// DevPath isn't changed, but other things might, e.g. UUID if someone manually formatted a disk
+		oldBdCp.Status.DeviceStatus.Capacity = newBd.Status.DeviceStatus.Capacity
+		oldBdCp.Status.DeviceStatus.Details = newBd.Status.DeviceStatus.Details
+	} else {
+		// The BD is inactive.  This can happen if a provisioned device is
+		// temporarily gone and has come back.  It can also happen for provisioned
+		// multipath devices if the system is rebooted without multipathd enabled,
+		// and then later multipathd is started again.
+		if strings.HasPrefix(oldBd.Status.DeviceStatus.DevPath, "/dev/mapper") {
+			if isDevPathChanged(oldBd, newBd) {
+				logrus.WithFields(logrus.Fields{
+					"name":      oldBd.Name,
+					"device":    oldBd.Status.DeviceStatus.DevPath,
+					"newDevice": newBd.Status.DeviceStatus.DevPath,
+				}).Warn("new device path detected for inactive multipath device - skipping update")
+				return false
+			}
+			path, _ := filepath.EvalSymlinks(oldBd.Status.DeviceStatus.DevPath)
+			if _, err := utils.IsMultipathDevice(path); err == nil {
+				logrus.WithFields(logrus.Fields{
+					"name": oldBd.Name,
+				}).Info("reactivating multipath device")
+				oldBdCp.Status.State = diskv1.BlockDeviceActive
+				// DeviceStatus really shouldn't have changed at this point.
+				// TODO: should we log if it did?
+				oldBdCp.Status.DeviceStatus.Capacity = newBd.Status.DeviceStatus.Capacity
+				oldBdCp.Status.DeviceStatus.Details = newBd.Status.DeviceStatus.Details
+			}
+		} else {
+			logrus.WithFields(logrus.Fields{
+				"name": oldBd.Name,
+				"wwn":  newBd.Status.DeviceStatus.Details.WWN,
+			}).Infof("reactivating block device")
+			if isDevPathChanged(oldBd, newBd) {
+				logrus.WithFields(logrus.Fields{
+					"name":   oldBd.Name,
+					"device": newBd.Status.DeviceStatus.DevPath,
+				}).Info("got new device path")
+				oldBdCp.Status.DeviceStatus.DevPath = newBd.Status.DeviceStatus.DevPath
+			}
 			oldBdCp.Status.State = diskv1.BlockDeviceActive
+			// This pulls in all status updates -- devpath, wwn, uuid, vendor, model, serial, ...
+			// TODO: log what's changed?
+			oldBdCp.Status.DeviceStatus.Capacity = newBd.Status.DeviceStatus.Capacity
+			oldBdCp.Status.DeviceStatus.Details = newBd.Status.DeviceStatus.Details
 		}
+	}
 
-		if reflect.DeepEqual(oldBd, oldBdCp) {
-			logrus.Debugf("Skip updating multipath device %s", oldBd.Name)
-			return
-		}
-
+	if !reflect.DeepEqual(oldBd, oldBdCp) {
+		logrus.WithFields(logrus.Fields{
+			"name": oldBd.Name,
+		}).Info("updating device")
+		// TODO: log diff of what's updated?
 		if _, err := s.Blockdevices.Update(oldBdCp); err != nil {
-			logrus.Errorf("Update device %s status error, wake up scanner again: %v", oldBdCp.Name, err)
+			logrus.WithFields(logrus.Fields{
+				"name": oldBd.Name,
+				"err":  err,
+			}).Error("error updating device, waking scanner")
 			s.Cond.Signal()
 		}
+	} else if isDevAlreadyProvisioned(oldBd) {
+		logrus.WithFields(logrus.Fields{
+			"name": oldBd.Name,
+		}).Debug("skipping provisioned device")
+	} else if s.NeedsAutoProvision(oldBd, autoProvisioned) {
+		logrus.WithFields(logrus.Fields{
+			"name": oldBd.Name,
+		}).Debug("enquing device for auto-provisioning")
+		s.Blockdevices.Enqueue(s.Namespace, oldBd.Name)
 	} else {
-		logrus.Debugf("Skip updating device %s", newBd.Name)
+		logrus.WithFields(logrus.Fields{
+			"name": oldBd.Name,
+		}).Debug("device is unchanged (no need to update)")
 	}
+	return true
 }
 
 func (s *Scanner) deactivateOrDeleteBlockDevices(oldBds map[string]*diskv1.BlockDevice) error {
 	for _, oldBd := range oldBds {
 		// It should be fine for devices that aren't actually provisioned to go away
+		// THIS IS BRILLIANT -- IF IT'S FOUND MP DEVICES AND THEY'RE NOT PROVISIONED
+		// THEN WHEN THE MP DEVICE ACTIVATES THEY GO AWAY!
 		if oldBd.Status.ProvisionPhase == diskv1.ProvisionPhaseUnprovisioned {
 			logrus.Debugf("Delete device %s", oldBd.Name)
 			if err := s.Blockdevices.Delete(oldBd.Namespace, oldBd.Name, &metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
@@ -355,15 +394,17 @@ func (s *Scanner) scanBlockDevicesOnNode(ctx context.Context) error {
 		}
 
 		if existingBd != nil {
-			// Pick up the name of the existing block device we found
+			// Pick up the name of the existing block device we found (not strictly necessary,
+			// but just in case we try to use newBd.name in handleExistingDev...)
 			newBd.Name = existingBd.Name
-			s.handleExistingDev(existingBd, newBd, autoProvisioned)
-			// only first time to update the cache
-			if !CacheDiskTags.Initialized() && existingBd.Spec.Tags != nil && len(existingBd.Spec.Tags) > 0 {
-				CacheDiskTags.UpdateDiskTags(existingBd.Name, existingBd.Spec.Tags)
+			if s.handleExistingDev(existingBd, newBd, autoProvisioned) {
+				// only first time to update the cache
+				if !CacheDiskTags.Initialized() && existingBd.Spec.Tags != nil && len(existingBd.Spec.Tags) > 0 {
+					CacheDiskTags.UpdateDiskTags(existingBd.Name, existingBd.Spec.Tags)
+				}
+				// remove blockdevice from list of existing device names so we can delete missing devices afterward
+				delete(existingBDsByName, newBd.Name)
 			}
-			// remove blockdevice from list of existing device names so we can delete missing devices afterward
-			delete(existingBDsByName, newBd.Name)
 		} else {
 			// New block device, needs a name...
 			newBd.Name = uuid.NewString()
@@ -522,15 +563,3 @@ func isDevPathChanged(oldBd *diskv1.BlockDevice, newBd *diskv1.BlockDevice) bool
 func isDevAlreadyProvisioned(newBd *diskv1.BlockDevice) bool {
 	return newBd.Status.ProvisionPhase == diskv1.ProvisionPhaseProvisioned
 }
-
-func isDMDevice(name string) bool {
-	// Resolve symlink to check if it points to dm-x
-	// Resolve /dev/mapper/0QEMU_QEMU_HARDDISK_disk1 -> /dev/dm-0
-	// Resolve /dev/dm-0 -> /dev/dm-0
-	path, err := filepath.EvalSymlinks(name)
-	if err == nil && strings.Contains(path, "dm-") {
-		return true
-	}
-
-	return false
-}