diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index 6428bf9e35c..7814a9f860d 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -309,6 +309,11 @@ func (c *csiAttacher) MountDeviceWithStatusTracking(spec *volume.Spec, devicePat nodeName := string(c.plugin.host.GetNodeName()) publishContext, err := c.plugin.getPublishContext(c.k8s, csiSource.VolumeHandle, csiSource.Driver, nodeName) + if err != nil { + opExitStatus = volumetypes.OperationStateNoChange + return opExitStatus, err + } + nodeStageSecrets := map[string]string{} if csiSource.NodeStageSecretRef != nil { nodeStageSecrets, err = getCredentialsFromSecret(c.k8s, csiSource.NodeStageSecretRef) diff --git a/pkg/volume/csi/csi_attacher_test.go b/pkg/volume/csi/csi_attacher_test.go index 075601ad9ca..ae215e93b28 100644 --- a/pkg/volume/csi/csi_attacher_test.go +++ b/pkg/volume/csi/csi_attacher_test.go @@ -44,7 +44,9 @@ import ( featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/volume" + fakecsi "k8s.io/kubernetes/pkg/volume/csi/fake" volumetest "k8s.io/kubernetes/pkg/volume/testing" + volumetypes "k8s.io/kubernetes/pkg/volume/util/types" ) var ( @@ -1055,72 +1057,110 @@ func TestAttacherGetDeviceMountPath(t *testing.T) { func TestAttacherMountDevice(t *testing.T) { pvName := "test-pv" testCases := []struct { - testName string - volName string - devicePath string - deviceMountPath string - stageUnstageSet bool - shouldFail bool - spec *volume.Spec + testName string + volName string + devicePath string + deviceMountPath string + stageUnstageSet bool + shouldFail bool + createAttachment bool + exitStatus volumetypes.OperationStatus + spec *volume.Spec }{ { - testName: "normal PV", - volName: "test-vol1", - devicePath: "path1", - deviceMountPath: "path2", - stageUnstageSet: true, - spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + testName: "normal PV", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + stageUnstageSet: true, + createAttachment: true, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + exitStatus: volumetypes.OperationFinished, }, { - testName: "normal PV with mount options", - volName: "test-vol1", - devicePath: "path1", - deviceMountPath: "path2", - stageUnstageSet: true, - spec: volume.NewSpecFromPersistentVolume(makeTestPVWithMountOptions(pvName, 10, testDriver, "test-vol1", []string{"test-op"}), false), + testName: "normal PV with mount options", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + stageUnstageSet: true, + createAttachment: true, + exitStatus: volumetypes.OperationFinished, + spec: volume.NewSpecFromPersistentVolume(makeTestPVWithMountOptions(pvName, 10, testDriver, "test-vol1", []string{"test-op"}), false), }, { - testName: "no vol name", - volName: "", - devicePath: "path1", - deviceMountPath: "path2", - stageUnstageSet: true, - shouldFail: true, - spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, ""), false), + testName: "normal PV but with missing attachment should result in no-change", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + stageUnstageSet: true, + createAttachment: false, + shouldFail: true, + exitStatus: volumetypes.OperationStateNoChange, + spec: volume.NewSpecFromPersistentVolume(makeTestPVWithMountOptions(pvName, 10, testDriver, "test-vol1", []string{"test-op"}), false), }, { - testName: "no device path", - volName: "test-vol1", - devicePath: "", - deviceMountPath: "path2", - stageUnstageSet: true, - shouldFail: false, - spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + testName: "no vol name", + volName: "", + devicePath: "path1", + deviceMountPath: "path2", + stageUnstageSet: true, + shouldFail: true, + createAttachment: true, + exitStatus: volumetypes.OperationFinished, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, ""), false), }, { - testName: "no device mount path", - volName: "test-vol1", - devicePath: "path1", - deviceMountPath: "", - stageUnstageSet: true, - shouldFail: true, - spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + testName: "no device path", + volName: "test-vol1", + devicePath: "", + deviceMountPath: "path2", + stageUnstageSet: true, + shouldFail: false, + createAttachment: true, + exitStatus: volumetypes.OperationFinished, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), }, { - testName: "stage_unstage cap not set", - volName: "test-vol1", - devicePath: "path1", - deviceMountPath: "path2", - stageUnstageSet: false, - spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + testName: "no device mount path", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "", + stageUnstageSet: true, + shouldFail: true, + createAttachment: true, + exitStatus: volumetypes.OperationFinished, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), }, { - testName: "failure with volume source", - volName: "test-vol1", - devicePath: "path1", - deviceMountPath: "path2", - shouldFail: true, - spec: volume.NewSpecFromVolume(makeTestVol(pvName, testDriver)), + testName: "stage_unstage cap not set", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + stageUnstageSet: false, + createAttachment: true, + exitStatus: volumetypes.OperationFinished, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + }, + { + testName: "failure with volume source", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + shouldFail: true, + createAttachment: true, + exitStatus: volumetypes.OperationFinished, + spec: volume.NewSpecFromVolume(makeTestVol(pvName, testDriver)), + }, + { + testName: "pv with nodestage timeout should result in in-progress device", + volName: fakecsi.NodeStageTimeOut_VolumeID, + devicePath: "path1", + deviceMountPath: "path2", + stageUnstageSet: true, + createAttachment: true, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, fakecsi.NodeStageTimeOut_VolumeID), false), + exitStatus: volumetypes.OperationInProgress, + shouldFail: true, }, } @@ -1146,18 +1186,20 @@ func TestAttacherMountDevice(t *testing.T) { nodeName := string(csiAttacher.plugin.host.GetNodeName()) attachID := getAttachmentName(tc.volName, testDriver, nodeName) - // Set up volume attachment - attachment := makeTestAttachment(attachID, nodeName, pvName) - _, err := csiAttacher.k8s.StorageV1().VolumeAttachments().Create(attachment) - if err != nil { - t.Fatalf("failed to attach: %v", err) + if tc.createAttachment { + // Set up volume attachment + attachment := makeTestAttachment(attachID, nodeName, pvName) + _, err := csiAttacher.k8s.StorageV1().VolumeAttachments().Create(attachment) + if err != nil { + t.Fatalf("failed to attach: %v", err) + } + go func() { + fakeWatcher.Delete(attachment) + }() } - go func() { - fakeWatcher.Delete(attachment) - }() // Run - err = csiAttacher.MountDevice(tc.spec, tc.devicePath, tc.deviceMountPath) + exitStatus, err := csiAttacher.MountDeviceWithStatusTracking(tc.spec, tc.devicePath, tc.deviceMountPath) // Verify if err != nil { @@ -1170,6 +1212,10 @@ func TestAttacherMountDevice(t *testing.T) { t.Errorf("test should fail, but no error occurred") } + if exitStatus != tc.exitStatus { + t.Fatalf("expected exitStatus: %v got: %v", tc.exitStatus, exitStatus) + } + // Verify call goes through all the way numStaged := 1 if !tc.stageUnstageSet { diff --git a/pkg/volume/csi/csi_client_test.go b/pkg/volume/csi/csi_client_test.go index cef0e814fc6..3724b2cb574 100644 --- a/pkg/volume/csi/csi_client_test.go +++ b/pkg/volume/csi/csi_client_test.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" "k8s.io/kubernetes/pkg/volume" "k8s.io/kubernetes/pkg/volume/csi/fake" + volumetypes "k8s.io/kubernetes/pkg/volume/util/types" ) type fakeCsiDriverClient struct { @@ -156,6 +157,12 @@ func (c *fakeCsiDriverClient) NodePublishVolume( } _, err := c.nodeClient.NodePublishVolume(ctx, req) + if err != nil { + if isFinalError(err) { + return err + } + return volumetypes.NewOperationTimedOutError(err.Error()) + } return err } @@ -201,6 +208,12 @@ func (c *fakeCsiDriverClient) NodeStageVolume(ctx context.Context, } _, err := c.nodeClient.NodeStageVolume(ctx, req) + if err != nil { + if isFinalError(err) { + return err + } + return volumetypes.NewOperationTimedOutError(err.Error()) + } return err } diff --git a/pkg/volume/csi/csi_mounter.go b/pkg/volume/csi/csi_mounter.go index 6e0fb98aad8..22a5085bd03 100644 --- a/pkg/volume/csi/csi_mounter.go +++ b/pkg/volume/csi/csi_mounter.go @@ -104,8 +104,8 @@ func (c *csiMountMgr) SetUp(mounterArgs volume.MounterArgs) error { } func (c *csiMountMgr) SetUpWithStatusTracking(mounterArgs volume.MounterArgs) (volumetypes.OperationStatus, error) { - err := c.SetUp(mounterArgs) - return volumetypes.OperationFinished, err + opExitStatus, err := c.setupUtil(c.GetPath(), mounterArgs) + return opExitStatus, err } func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { diff --git a/pkg/volume/csi/csi_mounter_test.go b/pkg/volume/csi/csi_mounter_test.go index 985e208faa0..fcde71c0c13 100644 --- a/pkg/volume/csi/csi_mounter_test.go +++ b/pkg/volume/csi/csi_mounter_test.go @@ -36,7 +36,9 @@ import ( featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/volume" + fakecsi "k8s.io/kubernetes/pkg/volume/csi/fake" "k8s.io/kubernetes/pkg/volume/util" + volumetypes "k8s.io/kubernetes/pkg/volume/util/types" ) var ( @@ -396,6 +398,113 @@ func TestMounterSetUpSimple(t *testing.T) { } } +func TestMounterSetupWithStatusTracking(t *testing.T) { + fakeClient := fakeclient.NewSimpleClientset() + plug, tmpDir := newTestPlugin(t, fakeClient) + defer os.RemoveAll(tmpDir) + + testCases := []struct { + name string + podUID types.UID + spec func(string, []string) *volume.Spec + shouldFail bool + exitStatus volumetypes.OperationStatus + createAttachment bool + }{ + { + name: "setup with correct persistent volume source should result in finish exit status", + podUID: types.UID(fmt.Sprintf("%08X", rand.Uint64())), + spec: func(fsType string, options []string) *volume.Spec { + pvSrc := makeTestPV("pv1", 20, testDriver, "vol1") + pvSrc.Spec.CSI.FSType = fsType + pvSrc.Spec.MountOptions = options + return volume.NewSpecFromPersistentVolume(pvSrc, false) + }, + exitStatus: volumetypes.OperationFinished, + createAttachment: true, + }, + { + name: "setup with missing attachment should result in nochange", + podUID: types.UID(fmt.Sprintf("%08X", rand.Uint64())), + spec: func(fsType string, options []string) *volume.Spec { + return volume.NewSpecFromPersistentVolume(makeTestPV("pv3", 20, testDriver, "vol4"), false) + }, + exitStatus: volumetypes.OperationStateNoChange, + createAttachment: false, + shouldFail: true, + }, + { + name: "setup with timeout errors on NodePublish", + podUID: types.UID(fmt.Sprintf("%08X", rand.Uint64())), + spec: func(fsType string, options []string) *volume.Spec { + return volume.NewSpecFromPersistentVolume(makeTestPV("pv4", 20, testDriver, fakecsi.NodePublishTimeOut_VolumeID), false) + }, + createAttachment: true, + exitStatus: volumetypes.OperationInProgress, + shouldFail: true, + }, + { + name: "setup with missing secrets should result in nochange exit", + podUID: types.UID(fmt.Sprintf("%08X", rand.Uint64())), + spec: func(fsType string, options []string) *volume.Spec { + pv := makeTestPV("pv5", 20, testDriver, "vol6") + pv.Spec.PersistentVolumeSource.CSI.NodePublishSecretRef = &api.SecretReference{ + Name: "foo", + Namespace: "default", + } + return volume.NewSpecFromPersistentVolume(pv, false) + }, + exitStatus: volumetypes.OperationStateNoChange, + createAttachment: true, + shouldFail: true, + }, + } + + for _, tc := range testCases { + registerFakePlugin(testDriver, "endpoint", []string{"1.0.0"}, t) + t.Run(tc.name, func(t *testing.T) { + mounter, err := plug.NewMounter( + tc.spec("ext4", []string{}), + &api.Pod{ObjectMeta: meta.ObjectMeta{UID: tc.podUID, Namespace: testns}}, + volume.VolumeOptions{}, + ) + if mounter == nil { + t.Fatal("failed to create CSI mounter") + } + + csiMounter := mounter.(*csiMountMgr) + csiMounter.csiClient = setupClient(t, true) + + if csiMounter.volumeLifecycleMode != storagev1beta1.VolumeLifecyclePersistent { + t.Fatal("unexpected volume mode: ", csiMounter.volumeLifecycleMode) + } + + if tc.createAttachment { + attachID := getAttachmentName(csiMounter.volumeID, string(csiMounter.driverName), string(plug.host.GetNodeName())) + attachment := makeTestAttachment(attachID, "test-node", csiMounter.spec.Name()) + _, err = csiMounter.k8s.StorageV1().VolumeAttachments().Create(attachment) + if err != nil { + t.Fatalf("failed to setup VolumeAttachment: %v", err) + } + } + + opExistStatus, err := csiMounter.SetUpWithStatusTracking(volume.MounterArgs{}) + + if opExistStatus != tc.exitStatus { + t.Fatalf("expected exitStatus: %v but got %v", tc.exitStatus, opExistStatus) + } + + if tc.shouldFail && err == nil { + t.Fatalf("expected failure but Setup succeeded") + } + + if !tc.shouldFail && err != nil { + t.Fatalf("expected successs got mounter.Setup failed with: %v", err) + } + }) + } +} + func TestMounterSetUpWithInline(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIInlineVolume, true)() diff --git a/pkg/volume/csi/fake/fake_client.go b/pkg/volume/csi/fake/fake_client.go index 19072963da0..0ce54f50ad8 100644 --- a/pkg/volume/csi/fake/fake_client.go +++ b/pkg/volume/csi/fake/fake_client.go @@ -21,9 +21,17 @@ import ( "errors" "strings" - "google.golang.org/grpc" - csipb "github.com/container-storage-interface/spec/lib/go/csi" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +const ( + // NodePublishTimeout_VolumeID is volume id that will result in NodePublish operation to timeout + NodePublishTimeOut_VolumeID = "node-publish-timeout" + // NodeStageTimeOut_VolumeID is a volume id that will result in NodeStage operation to timeout + NodeStageTimeOut_VolumeID = "node-stage-timeout" ) // IdentityClient is a CSI identity client used for testing @@ -158,6 +166,12 @@ func (f *NodeClient) NodePublishVolume(ctx context.Context, req *csipb.NodePubli if !strings.Contains(fsTypes, fsType) { return nil, errors.New("invalid fstype") } + + if req.GetVolumeId() == NodePublishTimeOut_VolumeID { + timeoutErr := status.Errorf(codes.DeadlineExceeded, "timeout exceeded") + return nil, timeoutErr + } + f.nodePublishedVolumes[req.GetVolumeId()] = CSIVolume{ VolumeHandle: req.GetVolumeId(), Path: req.GetTargetPath(), @@ -214,6 +228,11 @@ func (f *NodeClient) NodeStageVolume(ctx context.Context, req *csipb.NodeStageVo return nil, errors.New("invalid fstype") } + if req.GetVolumeId() == NodeStageTimeOut_VolumeID { + timeoutErr := status.Errorf(codes.DeadlineExceeded, "timeout exceeded") + return nil, timeoutErr + } + f.nodeStagedVolumes[req.GetVolumeId()] = csiVol return &csipb.NodeStageVolumeResponse{}, nil }