From 30dc068cd18601015d59a1253f06c86ed92e7dc9 Mon Sep 17 00:00:00 2001 From: Praveen raj Mani Date: Mon, 30 Oct 2023 18:01:36 +0530 Subject: [PATCH] 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 | 117 +++++++++++++++++++++++++++++++++++++++ pkg/volume/event.go | 25 ++++++++- pkg/volume/event_test.go | 63 +++++++++++++++++++++ 3 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 pkg/device/sync_test.go diff --git a/pkg/device/sync_test.go b/pkg/device/sync_test.go new file mode 100644 index 000000000..d2acb25ea --- /dev/null +++ b/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 . + +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) + } + } +} diff --git a/pkg/volume/event.go b/pkg/volume/event.go index f24b1d49b..7881d7490 100644 --- a/pkg/volume/event.go +++ b/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) diff --git a/pkg/volume/event_test.go b/pkg/volume/event_test.go index 1f0273bda..ce77c7e3e 100644 --- a/pkg/volume/event_test.go +++ b/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()) + } +}