From a54ebadd829a4611cea77180c188f18af95e71b2 Mon Sep 17 00:00:00 2001 From: Adam Krpan Date: Tue, 20 Aug 2024 00:25:58 +0000 Subject: [PATCH 1/2] PWX-38585: Removes extra pod name label Signed-off-by: Adam Krpan --- api/server/docker.go | 1 + api/server/sdk/volume_ops.go | 14 +-- csi/controller.go | 9 +- csi/controller_test.go | 237 ++++++++++++++++++++++++++++++++++- 4 files changed, 242 insertions(+), 19 deletions(-) diff --git a/api/server/docker.go b/api/server/docker.go index f5f0493be..2073cffa6 100644 --- a/api/server/docker.go +++ b/api/server/docker.go @@ -377,6 +377,7 @@ func (d *driver) create(w http.ResponseWriter, r *http.Request) { _, err = volumes.Clone(ctx, &api.SdkVolumeCloneRequest{ Name: name, ParentId: source.Parent, + AdditionalLabels: locator.GetVolumeLabels(), }) } else { // create diff --git a/api/server/sdk/volume_ops.go b/api/server/sdk/volume_ops.go index 49d44f820..35ad75223 100644 --- a/api/server/sdk/volume_ops.go +++ b/api/server/sdk/volume_ops.go @@ -36,9 +36,6 @@ import ( "google.golang.org/grpc/status" ) -// FADAPodLabelKey is a label added to volume locators in the case of FADA volume clone/snap restore -const FADAPodLabelKey = "pure-pod-name" // Used to plumb in the pod name for volume cloning - // When create is called for an existing volume, this function is called to make sure // the SDK only returns that the volume is ready when the status is UP func (s *VolumeServer) waitForVolumeReady(ctx context.Context, id string) (*api.Volume, error) { @@ -173,18 +170,9 @@ func (s *VolumeServer) create( } // Create a snapshot from the parent - // Only include the FADA pod label - var labels map[string]string = nil - if locator.GetVolumeLabels() != nil { - if pod, ok := locator.GetVolumeLabels()[FADAPodLabelKey]; ok { - labels = map[string]string{ - FADAPodLabelKey: pod, - } - } - } id, err = s.driver(ctx).Snapshot(ctx, parent.GetId(), false, &api.VolumeLocator{ Name: volName, - VolumeLabels: labels, + VolumeLabels: locator.GetVolumeLabels(), }, false) if err != nil { if err == kvdb.ErrNotFound { diff --git a/csi/controller.go b/csi/controller.go index a01079a92..5970090d5 100644 --- a/csi/controller.go +++ b/csi/controller.go @@ -614,14 +614,17 @@ func (s *OsdCsiServer) CreateVolume( } newVolumeId = createResp.VolumeId } else { - clonedMetadata := getClonedPVCMetadata(locator) + labels := locator.GetVolumeLabels() if spec.GetFADAPodName() != "" { - clonedMetadata[sdk.FADAPodLabelKey] = spec.GetFADAPodName() + labels[api.SpecPurePodName] = spec.GetFADAPodName() } + delete(labels, intreePvcNameKey) + delete(labels, intreePvcNamespaceKey) + delete(labels, api.SpecParent) cloneResp, err := volumes.Clone(ctx, &api.SdkVolumeCloneRequest{ Name: req.GetName(), ParentId: source.Parent, - AdditionalLabels: clonedMetadata, + AdditionalLabels: labels, }) if err != nil { return nil, err diff --git a/csi/controller_test.go b/csi/controller_test.go index 89031f2ba..e7c9d7713 100644 --- a/csi/controller_test.go +++ b/csi/controller_test.go @@ -33,7 +33,6 @@ import ( "github.com/golang/protobuf/ptypes/timestamp" "github.com/libopenstorage/openstorage/api" "github.com/libopenstorage/openstorage/api/mock" - "github.com/libopenstorage/openstorage/api/server/sdk" "github.com/libopenstorage/openstorage/api/spec" authsecrets "github.com/libopenstorage/openstorage/pkg/auth/secrets" mockLoadBalancer "github.com/libopenstorage/openstorage/pkg/loadbalancer/mock" @@ -1334,7 +1333,7 @@ func TestControllerCreateVolumeBadSnapshot(t *testing.T) { // Return an error from snapshot s.MockDriver(). EXPECT(). - Snapshot(gomock.Any(), parent, false, &api.VolumeLocator{Name: name}, false). + Snapshot(gomock.Any(), parent, false, &api.VolumeLocator{Name: name, VolumeLabels: nil}, false). Return("", fmt.Errorf("snapshoterr")). Times(1), ) @@ -2056,7 +2055,7 @@ func TestControllerCreateVolumeFromSnapshotFADAPod(t *testing.T) { // create s.MockDriver(). EXPECT(). - Snapshot(gomock.Any(), gomock.Any(), gomock.Any(), &api.VolumeLocator{Name: name, VolumeLabels: map[string]string{sdk.FADAPodLabelKey: pod}}, gomock.Any()). + Snapshot(gomock.Any(), gomock.Any(), gomock.Any(), &api.VolumeLocator{Name: name, VolumeLabels: map[string]string{api.SpecPurePodName: pod}}, gomock.Any()). Return(snapID, nil). Times(1), s.MockDriver(). @@ -2246,6 +2245,238 @@ func TestControllerCreateVolumeSnapshotThroughParameters(t *testing.T) { assert.Equal(t, mockParentID, volumeInfo.GetVolumeContext()[api.SpecParent]) } +func TestControllerCreateVolumeFromSource(t *testing.T) { + // Create server and client connection + s := newTestServer(t) + defer s.Stop() + c := csi.NewControllerClient(s.Conn()) + s.mockClusterEnumerateNode(t, "node-1") + // Setup request + mockParentID := "parendId" + name := "myvol" + size := int64(1234) + req := &csi.CreateVolumeRequest{ + Name: name, + VolumeCapabilities: []*csi.VolumeCapability{ + {}, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: size, + }, + VolumeContentSource: &csi.VolumeContentSource{ + Type: &csi.VolumeContentSource_Volume{ + Volume: &csi.VolumeContentSource_VolumeSource{ + VolumeId: mockParentID, + }, + }, + }, + Parameters: map[string]string{ + "testkey": "testval", + }, + Secrets: map[string]string{authsecrets.SecretTokenKey: systemUserToken}, + } + + // Setup mock functions + id := "myid" + snapID := id + "-snap" + gomock.InOrder( + + // First check on parent + s.MockDriver(). + EXPECT(). + Enumerate(&api.VolumeLocator{ + VolumeIds: []string{mockParentID}, + }, nil). + Return([]*api.Volume{{Id: mockParentID}}, nil). + Times(1), + + // VolFromName (name) + s.MockDriver(). + EXPECT(). + Inspect(gomock.Any(), []string{name}). + Return(nil, fmt.Errorf("not found")). + Times(1), + + s.MockDriver(). + EXPECT(). + Enumerate(gomock.Any(), nil). + Return(nil, fmt.Errorf("not found")). + Times(1), + + //VolFromName parent + s.MockDriver(). + EXPECT(). + Inspect(gomock.Any(), gomock.Any()). + Return( + []*api.Volume{{ + Id: mockParentID, + }}, nil). + Times(1), + + // create + s.MockDriver(). + EXPECT(). + Snapshot(gomock.Any(), gomock.Any(), gomock.Any(), &api.VolumeLocator{Name: name, VolumeLabels: map[string]string{"testkey": "testval"}}, gomock.Any()). + Return(snapID, nil). + Times(1), + s.MockDriver(). + EXPECT(). + Enumerate(&api.VolumeLocator{ + VolumeIds: []string{snapID}, + }, nil). + Return([]*api.Volume{ + { + Id: id, + Source: &api.Source{Parent: mockParentID}, + }, + }, nil). + Times(2), + + s.MockDriver(). + EXPECT(). + Set(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil). + Times(1), + + s.MockDriver(). + EXPECT(). + Enumerate(gomock.Any(), nil). + Return([]*api.Volume{ + { + Id: id, + Source: &api.Source{Parent: mockParentID}, + }, + }, nil). + Times(1), + ) + + r, err := c.CreateVolume(context.Background(), req) + assert.Nil(t, err) + assert.NotNil(t, r) + volumeInfo := r.GetVolume() + + assert.Equal(t, id, volumeInfo.GetVolumeId()) + assert.NotEqual(t, "true", volumeInfo.GetVolumeContext()[api.SpecSharedv4]) + assert.Equal(t, mockParentID, volumeInfo.GetVolumeContext()[api.SpecParent]) +} + +func TestControllerCreateVolumeFromSourceFADAPod(t *testing.T) { + // Create server and client connection + s := newTestServer(t) + defer s.Stop() + c := csi.NewControllerClient(s.Conn()) + s.mockClusterEnumerateNode(t, "node-1") + // Setup request + mockParentID := "parendId" + name := "myvol" + pod := "mypod" + size := int64(1234) + req := &csi.CreateVolumeRequest{ + Name: name, + VolumeCapabilities: []*csi.VolumeCapability{ + {}, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: size, + }, + VolumeContentSource: &csi.VolumeContentSource{ + Type: &csi.VolumeContentSource_Volume{ + Volume: &csi.VolumeContentSource_VolumeSource{ + VolumeId: mockParentID, + }, + }, + }, + Parameters: map[string]string{ + "testkey": "testval", + api.SpecPurePodName: pod, + }, + Secrets: map[string]string{authsecrets.SecretTokenKey: systemUserToken}, + } + + // Setup mock functions + id := "myid" + snapID := id + "-snap" + gomock.InOrder( + + // First check on parent + s.MockDriver(). + EXPECT(). + Enumerate(&api.VolumeLocator{ + VolumeIds: []string{mockParentID}, + }, nil). + Return([]*api.Volume{{Id: mockParentID}}, nil). + Times(1), + + // VolFromName (name) + s.MockDriver(). + EXPECT(). + Inspect(gomock.Any(), []string{name}). + Return(nil, fmt.Errorf("not found")). + Times(1), + + s.MockDriver(). + EXPECT(). + Enumerate(gomock.Any(), nil). + Return(nil, fmt.Errorf("not found")). + Times(1), + + //VolFromName parent + s.MockDriver(). + EXPECT(). + Inspect(gomock.Any(), gomock.Any()). + Return( + []*api.Volume{{ + Id: mockParentID, + }}, nil). + Times(1), + + // create + s.MockDriver(). + EXPECT(). + Snapshot(gomock.Any(), gomock.Any(), gomock.Any(), &api.VolumeLocator{Name: name, VolumeLabels: map[string]string{api.SpecPurePodName: pod, "testkey": "testval"}}, gomock.Any()). + Return(snapID, nil). + Times(1), + s.MockDriver(). + EXPECT(). + Enumerate(&api.VolumeLocator{ + VolumeIds: []string{snapID}, + }, nil). + Return([]*api.Volume{ + { + Id: id, + Source: &api.Source{Parent: mockParentID}, + }, + }, nil). + Times(2), + + s.MockDriver(). + EXPECT(). + Set(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil). + Times(1), + + s.MockDriver(). + EXPECT(). + Enumerate(gomock.Any(), nil). + Return([]*api.Volume{ + { + Id: id, + Source: &api.Source{Parent: mockParentID}, + }, + }, nil). + Times(1), + ) + + r, err := c.CreateVolume(context.Background(), req) + assert.Nil(t, err) + assert.NotNil(t, r) + volumeInfo := r.GetVolume() + + assert.Equal(t, id, volumeInfo.GetVolumeId()) + assert.NotEqual(t, "true", volumeInfo.GetVolumeContext()[api.SpecSharedv4]) + assert.Equal(t, mockParentID, volumeInfo.GetVolumeContext()[api.SpecParent]) +} + func TestControllerCreateVolumeBlock(t *testing.T) { // Create server and client connection s := newTestServer(t) From f678c150bfd3cb424a8f4cde180b3b14ea9f6936 Mon Sep 17 00:00:00 2001 From: Adam Krpan Date: Tue, 20 Aug 2024 22:59:38 +0000 Subject: [PATCH 2/2] PWX-38585: PR feedback Signed-off-by: Adam Krpan --- csi/controller.go | 3 --- csi/controller_test.go | 5 +++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/csi/controller.go b/csi/controller.go index 5970090d5..a4fb999fb 100644 --- a/csi/controller.go +++ b/csi/controller.go @@ -618,9 +618,6 @@ func (s *OsdCsiServer) CreateVolume( if spec.GetFADAPodName() != "" { labels[api.SpecPurePodName] = spec.GetFADAPodName() } - delete(labels, intreePvcNameKey) - delete(labels, intreePvcNamespaceKey) - delete(labels, api.SpecParent) cloneResp, err := volumes.Clone(ctx, &api.SdkVolumeCloneRequest{ Name: req.GetName(), ParentId: source.Parent, diff --git a/csi/controller_test.go b/csi/controller_test.go index e7c9d7713..5e3e89c00 100644 --- a/csi/controller_test.go +++ b/csi/controller_test.go @@ -1333,7 +1333,7 @@ func TestControllerCreateVolumeBadSnapshot(t *testing.T) { // Return an error from snapshot s.MockDriver(). EXPECT(). - Snapshot(gomock.Any(), parent, false, &api.VolumeLocator{Name: name, VolumeLabels: nil}, false). + Snapshot(gomock.Any(), parent, false, &api.VolumeLocator{Name: name, VolumeLabels: map[string]string{api.SpecParent: parent}}, false). Return("", fmt.Errorf("snapshoterr")). Times(1), ) @@ -2163,7 +2163,8 @@ func TestControllerCreateVolumeSnapshotThroughParameters(t *testing.T) { s.MockDriver(). EXPECT(). Snapshot(gomock.Any(), mockParentID, false, &api.VolumeLocator{ - Name: name, + Name: name, + VolumeLabels: map[string]string{api.SpecParent: mockParentID}, }, false). Return(id, nil).