fix drive name of volume at startup if needed (#873)

When a node-server starts, it syncs the drive states like drive name,
capacity, make etc. This is because, the drive order wouldv'e changed
during node restarts.

In such scenarios, We need to also make sure to fix the drive name in
corresponding volumes. Without this fix, the `kubectl directpv list
volumes` command will show wrong drive names (if the node has restarted)

pkg/device/sync_test.go

@@ -0,0 +1,117 @@
+// This file is part of MinIO DirectPV
+// Copyright (c) 2023 MinIO, Inc.
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package device
+
+import (
+	"testing"
+
+	directpvtypes "github.com/minio/directpv/pkg/apis/directpv.min.io/types"
+	"github.com/minio/directpv/pkg/types"
+)
+
+func newDrive(name string, totalCapacity int64, make, volume string) *types.Drive {
+	drive := types.NewDrive(
+		directpvtypes.DriveID(name+"-id"),
+		types.DriveStatus{
+			TotalCapacity: totalCapacity,
+			Make:          make,
+		},
+		directpvtypes.NodeID("nodeId"),
+		directpvtypes.DriveName(name),
+		directpvtypes.AccessTierDefault,
+	)
+	drive.AddVolumeFinalizer(volume)
+	return drive
+}
+
+func newTestDevice(name string, totalCapacity int64, dmname string) device {
+	return device{
+		TotalCapacity: totalCapacity,
+		Device: Device{
+			Name:   name,
+			DMName: dmname,
+		},
+	}
+}
+
+func TestSyncDrive(t *testing.T) {
+	testCases := []struct {
+		drive                 *types.Drive
+		device                device
+		updated               bool
+		expectedDriveName     directpvtypes.DriveName
+		expectedDriveCapacity int64
+		expectedMake          string
+	}{
+		{
+			drive:                 newDrive("sda", 100, "dmname", "volume-1"),
+			device:                newTestDevice("sda", 100, "dmname"),
+			updated:               false,
+			expectedDriveName:     "sda",
+			expectedDriveCapacity: 100,
+			expectedMake:          "dmname",
+		},
+		{
+			drive:                 newDrive("sda", 100, "dmname", "volume-1"),
+			device:                newTestDevice("sda", 200, "dmname"),
+			updated:               true,
+			expectedDriveName:     "sda",
+			expectedDriveCapacity: 200,
+			expectedMake:          "dmname",
+		},
+		{
+			drive:                 newDrive("sda", 100, "dmname", "volume-1"),
+			device:                newTestDevice("sda", 100, "dmname-new"),
+			updated:               true,
+			expectedDriveName:     "sda",
+			expectedDriveCapacity: 100,
+			expectedMake:          "dmname-new",
+		},
+		{
+			drive:                 newDrive("sda", 100, "dmname", "volume-1"),
+			device:                newTestDevice("sdb", 100, "dmname"),
+			updated:               true,
+			expectedDriveName:     "sdb",
+			expectedDriveCapacity: 100,
+			expectedMake:          "dmname",
+		},
+		{
+			drive:                 newDrive("sda", 100, "dmname", "volume-1"),
+			device:                newTestDevice("sda", 100, "dmname"),
+			updated:               false,
+			expectedDriveName:     "sda",
+			expectedDriveCapacity: 100,
+			expectedMake:          "dmname",
+		},
+	}
+
+	for _, testCase := range testCases {
+		updated := syncDrive(testCase.drive, testCase.device)
+		if updated != testCase.updated {
+			t.Errorf("expected updated value: %v; but got %v", testCase.updated, updated)
+		}
+		if testCase.drive.GetDriveName() != testCase.expectedDriveName {
+			t.Errorf("expected drive name: %v; but got %v", testCase.expectedDriveName, testCase.drive.GetDriveName())
+		}
+		if testCase.drive.Status.TotalCapacity != testCase.expectedDriveCapacity {
+			t.Errorf("expected drive capacity: %v; but got %v", testCase.expectedDriveCapacity, testCase.drive.Status.TotalCapacity)
+		}
+		if testCase.drive.Status.Make != testCase.expectedMake {
+			t.Errorf("expected drive make: %v; but got %v", testCase.expectedMake, testCase.drive.Status.Make)
+		}
+	}
+}

pkg/volume/event.go

@@ -30,6 +30,7 @@ import (
 	"github.com/minio/directpv/pkg/sys"
 	"github.com/minio/directpv/pkg/types"
 	"github.com/minio/directpv/pkg/xfs"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/tools/cache"
@@ -77,15 +78,37 @@ func (handler *volumeEventHandler) ObjectType() runtime.Object {
 	return &types.Volume{}
 }
 
-func (handler *volumeEventHandler) Handle(ctx context.Context, _ controller.EventType, object runtime.Object) error {
+func (handler *volumeEventHandler) Handle(ctx context.Context, eventType controller.EventType, object runtime.Object) error {
 	volume := object.(*types.Volume)
 	if !volume.GetDeletionTimestamp().IsZero() {
 		return handler.delete(ctx, volume)
 	}
 
+	if eventType == controller.AddEvent {
+		return sync(ctx, volume)
+	}
+
 	return nil
 }
 
+func sync(ctx context.Context, volume *types.Volume) error {
+	drive, err := client.DriveClient().Get(ctx, string(volume.GetDriveID()), metav1.GetOptions{})
+	if err != nil {
+		if !apierrors.IsNotFound(err) {
+			return err
+		}
+		return nil
+	}
+	driveName := drive.GetDriveName()
+	if volume.GetDriveName() != driveName {
+		volume.SetDriveName(driveName)
+		_, err = client.VolumeClient().Update(ctx, volume, metav1.UpdateOptions{
+			TypeMeta: types.NewVolumeTypeMeta(),
+		})
+	}
+	return err
+}
+
 func (handler *volumeEventHandler) delete(ctx context.Context, volume *types.Volume) error {
 	if !volume.IsReleased() {
 		return fmt.Errorf("volume %v must be released before cleaning up", volume.Name)

pkg/volume/event_test.go

@@ -188,3 +188,66 @@ func TestAbnormalDeleteEventHandle(t *testing.T) {
 		t.Errorf("[%s] DeleteVolumeHandle expected to fail but succeeded", newObj.Name)
 	}
 }
+
+func TestSync(t *testing.T) {
+	newDrive := func(name, driveName, volume string) *types.Drive {
+		drive := types.NewDrive(
+			directpvtypes.DriveID(name),
+			types.DriveStatus{
+				TotalCapacity: 100,
+				Make:          "make",
+			},
+			directpvtypes.NodeID("nodeId"),
+			directpvtypes.DriveName(driveName),
+			directpvtypes.AccessTierDefault,
+		)
+		drive.AddVolumeFinalizer(volume)
+		return drive
+	}
+
+	newVolume := func(name, driveID, driveName string) *types.Volume {
+		volume := types.NewVolume(
+			name,
+			"fsuuid",
+			"nodeId",
+			directpvtypes.DriveID(driveID),
+			directpvtypes.DriveName(driveName),
+			100,
+		)
+		volume.Status.DataPath = "datapath"
+		return volume
+	}
+
+	drive := newDrive("drive-1", "sda", "volume-1")
+	volume := newVolume("volume-1", "drive-1", "sdb")
+	objects := []runtime.Object{
+		drive,
+		volume,
+	}
+	clientset := types.NewExtFakeClientset(clientsetfake.NewSimpleClientset(objects...))
+	client.SetDriveInterface(clientset.DirectpvLatest().DirectPVDrives())
+	client.SetVolumeInterface(clientset.DirectpvLatest().DirectPVVolumes())
+
+	volume, err := client.VolumeClient().Get(context.TODO(), volume.Name, metav1.GetOptions{
+		TypeMeta: types.NewVolumeTypeMeta(),
+	})
+	if err != nil {
+		t.Fatalf("Volume (%s) not found; %v", volume.Name, err)
+	}
+
+	err = sync(context.TODO(), volume)
+	if err != nil {
+		t.Fatalf("unable to sync; %v", err)
+	}
+
+	volume, err = client.VolumeClient().Get(context.TODO(), volume.Name, metav1.GetOptions{
+		TypeMeta: types.NewVolumeTypeMeta(),
+	})
+	if err != nil {
+		t.Fatalf("Volume (%s) not found after sync; %v", volume.Name, err)
+	}
+
+	if volume.GetDriveName() != drive.GetDriveName() {
+		t.Fatalf("expected drive name: %v; but got %v", drive.GetDriveName(), volume.GetDriveName())
+	}
+}