Skip to content

Commit 1c57625

Browse files
jwebster7jwebster7
authored
Stale LUKS mappers revisited
1 parent 7f629ca commit 1c57625

File tree

9 files changed

+450
-172
lines changed

9 files changed

+450
-172
lines changed

‎frontend/csi/node_helpers/kubernetes/plugin.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,13 +192,13 @@ func (h *helper) AddPublishedPath(ctx context.Context, volumeID, pathToAdd strin
192192

193193
volTrackingInfo, err := h.ReadTrackingInfo(ctx, volumeID)
194194
if err != nil {
195-
return fmt.Errorf("failed to read the tracking file; %v", err)
195+
return fmt.Errorf("failed to read the tracking file; %w", err)
196196
}
197197

198198
volTrackingInfo.PublishedPaths[pathToAdd] = struct{}{}
199199

200200
if err := h.WriteTrackingInfo(ctx, volumeID, volTrackingInfo); err != nil {
201-
return fmt.Errorf("failed to update the tracking file; %v", err)
201+
return fmt.Errorf("failed to update the tracking file; %w", err)
202202
}
203203

204204
h.publishedPaths[volumeID] = volTrackingInfo.PublishedPaths

‎frontend/csi/node_server.go‎

Lines changed: 27 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -638,12 +638,12 @@ func (p *Plugin) nodeExpandVolume(
638638
devicePath := publishInfo.DevicePath
639639
if convert.ToBool(publishInfo.LUKSEncryption) {
640640
if !luks.IsLegacyDevicePath(devicePath) {
641-
devicePath, err = p.devices.GetLUKSDevicePathForVolume(ctx, volumeId)
641+
devicePath, err = p.devices.GetLUKSDeviceForMultipathDevice(devicePath)
642642
if err != nil {
643643
Logc(ctx).WithFields(LogFields{
644644
"volumeId": volumeId,
645645
"publishedPath": publishInfo.DevicePath,
646-
}).WithError(err).Error("Failed to get LUKS device path for volume.")
646+
}).WithError(err).Error("Failed to get LUKS device path from device path.")
647647
return status.Error(codes.Internal, err.Error())
648648
}
649649
}
@@ -1465,18 +1465,15 @@ func (p *Plugin) nodeUnstageFCPVolume(
14651465
publishInfo.DevicePath = dmPath
14661466
}
14671467
} else {
1468-
// If not using luks legacy device path we need to find the LUKS mapper device.
1469-
luksMapperPath, err = p.devices.GetLUKSDevicePathForVolume(ctx, req.GetVolumeId())
1468+
// If not using LUKS legacy device path, we need to find the LUKS mapper device.
1469+
luksMapperPath, err = p.devices.GetLUKSDeviceForMultipathDevice(publishInfo.DevicePath)
14701470
if err != nil {
1471-
// If the LUKS device is not found, the functional difference is negligible to unstage.
1472-
// But it may be useful to log at different levels for observability.
1473-
log := Logc(ctx).WithFields(fields).WithError(err)
1474-
if errors.IsNotFoundError(err) {
1475-
log.Warn("Failed to get LUKS device path for volume.")
1476-
} else {
1477-
log.Debug("Could not determine LUKS device path for volume.")
1471+
if !errors.IsNotFoundError(err) {
1472+
Logc(ctx).WithFields(fields).WithError(err).Warn(
1473+
"Could not determine LUKS device path from multipath device. " +
1474+
"Continuing with device removal.")
14781475
}
1479-
log.Debug("Continuing with device removal.")
1476+
Logc(ctx).WithFields(fields).Info("No LUKS device path found from multipath device.")
14801477
}
14811478
}
14821479
err = p.devices.EnsureLUKSDeviceClosedWithMaxWaitLimit(ctx, luksMapperPath)
@@ -1519,10 +1516,11 @@ func (p *Plugin) nodeUnstageFCPVolume(
15191516
"multipathDevice": deviceInfo.MultipathDevice,
15201517
}
15211518

1522-
luksMapperPath, err = p.devices.GetLUKSDevicePathForVolume(ctx, req.GetVolumeId())
1519+
luksMapperPath, err = p.devices.GetLUKSDeviceForMultipathDevice(deviceInfo.MultipathDevice)
15231520
if err != nil {
15241521
if !errors.IsNotFoundError(err) {
1525-
Logc(ctx).WithFields(fields).WithError(err).Error("Failed to get LUKS device path from multipath device.")
1522+
Logc(ctx).WithFields(fields).
1523+
WithError(err).Error("Failed to get LUKS device path from multipath device.")
15261524
return err
15271525
}
15281526
Logc(ctx).WithFields(fields).Info("No LUKS device path found from multipath device.")
@@ -1971,7 +1969,7 @@ func (p *Plugin) nodeUnstageISCSIVolume(
19711969
if convert.ToBool(publishInfo.LUKSEncryption) {
19721970
var err error
19731971
var luksMapperPath string
1974-
fields := LogFields{"device": publishInfo.DevicePath, "volume": req.GetVolumeId()}
1972+
fields := LogFields{"device": publishInfo.DevicePath}
19751973
// Set device path to dm device to correctly verify legacy volumes.
19761974
if luks.IsLegacyDevicePath(publishInfo.DevicePath) {
19771975
luksMapperPath = publishInfo.DevicePath
@@ -1985,22 +1983,17 @@ func (p *Plugin) nodeUnstageISCSIVolume(
19851983
publishInfo.DevicePath = dmPath
19861984
}
19871985
} else {
1988-
// Use the volume ID to get the LUKS mapper path.
1989-
// This should always work if the mapper is still present.
1990-
luksMapperPath, err = p.devices.GetLUKSDevicePathForVolume(ctx, req.GetVolumeId())
1986+
// If not using LUKS legacy device path, we need to find the LUKS mapper device.
1987+
luksMapperPath, err = p.devices.GetLUKSDeviceForMultipathDevice(publishInfo.DevicePath)
19911988
if err != nil {
1992-
// If the LUKS device is not found, the functional difference is negligible to unstage.
1993-
// But it may be useful to log at different levels for observability.
1994-
log := Logc(ctx).WithFields(fields).WithError(err)
1995-
if errors.IsNotFoundError(err) {
1996-
log.Warn("Failed to get LUKS device path for volume.")
1997-
} else {
1998-
log.Debug("Could not determine LUKS device path for volume.")
1989+
if !errors.IsNotFoundError(err) {
1990+
Logc(ctx).WithFields(fields).WithError(err).Warn(
1991+
"Could not determine LUKS device path from multipath device. " +
1992+
"Continuing with device removal.")
19991993
}
2000-
log.Debug("Continuing with device removal.")
1994+
Logc(ctx).WithFields(fields).Info("No LUKS device path found from multipath device.")
20011995
}
20021996
}
2003-
20041997
err = p.devices.EnsureLUKSDeviceClosedWithMaxWaitLimit(ctx, luksMapperPath)
20051998
if err != nil {
20061999
Logc(ctx).WithError(err).Debug("Unable to remove LUKS device. Continuing with tracking file removal.")
@@ -2051,7 +2044,7 @@ func (p *Plugin) nodeUnstageISCSIVolume(
20512044
"multipathDevice": deviceInfo.MultipathDevice,
20522045
}
20532046

2054-
luksMapperPath, err = p.devices.GetLUKSDevicePathForVolume(ctx, req.GetVolumeId())
2047+
luksMapperPath, err = p.devices.GetLUKSDeviceForMultipathDevice(deviceInfo.MultipathDevice)
20552048
if err != nil {
20562049
if !errors.IsNotFoundError(err) {
20572050
Logc(ctx).WithFields(fields).WithError(err).Error("Failed to get LUKS device path from multipath device.")
@@ -3063,19 +3056,14 @@ func (p *Plugin) nodeUnstageNVMeVolume(
30633056
"publishedPath": publishInfo.DevicePath,
30643057
}
30653058

3066-
// Use the volume ID to get the LUKS mapper path.
3067-
// This should always work if the mapper is still present.
3068-
luksMapperPath, err = p.devices.GetLUKSDevicePathForVolume(ctx, req.GetVolumeId())
3059+
luksMapperPath, err = p.devices.GetLUKSDevicePathForDevicePath(ctx, devicePath)
30693060
if err != nil {
3070-
// If the LUKS device is not found, the functional difference is negligible to unstage.
3071-
// But it may be useful to log at different levels for observability.
3072-
log := Logc(ctx).WithFields(fields).WithError(err)
3073-
if errors.IsNotFoundError(err) {
3074-
log.Warn("Failed to get LUKS device path for volume.")
3075-
} else {
3076-
log.Debug("Could not determine LUKS device path for volume.")
3061+
if !errors.IsNotFoundError(err) {
3062+
Logc(ctx).WithFields(fields).WithError(err).Error("Failed to get LUKS device path from device path.")
3063+
return &csi.NodeUnstageVolumeResponse{}, err
30773064
}
3078-
log.Debug("Continuing with device removal.")
3065+
Logc(ctx).WithFields(fields).WithError(err).Debug("Failed to get LUKS device path from device path. " +
3066+
"Device may already be removed.")
30793067
}
30803068

30813069
if luksMapperPath != "" {

‎frontend/csi/node_server_test.go‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2114,8 +2114,7 @@ func TestNodeUnstageISCSIVolume(t *testing.T) {
21142114
},
21152115
getDeviceClient: func() devices.Devices {
21162116
mockDeviceClient := mock_devices.NewMockDevices(gomock.NewController(t))
2117-
2118-
mockDeviceClient.EXPECT().GetLUKSDevicePathForVolume(gomock.Any(), gomock.Any()).Return(mockDevicePath, nil)
2117+
mockDeviceClient.EXPECT().GetLUKSDeviceForMultipathDevice(gomock.Any()).Return(mockDevicePath, nil)
21192118
mockDeviceClient.EXPECT().EnsureLUKSDeviceClosedWithMaxWaitLimit(gomock.Any(), mockDevicePath).Return(nil)
21202119
mockDeviceClient.EXPECT().EnsureLUKSDeviceClosed(gomock.Any(), mockDevicePath).Return(nil)
21212120
mockDeviceClient.EXPECT().RemoveMultipathDeviceMappingWithRetries(gomock.Any(), gomock.Any(),
@@ -2146,7 +2145,6 @@ func TestNodeUnstageISCSIVolume(t *testing.T) {
21462145
getISCSIClient: func() iscsi.ISCSI {
21472146
mockISCSIClient := mock_iscsi.NewMockISCSI(gomock.NewController(t))
21482147
mockISCSIClient.EXPECT().RemoveLUNFromSessions(gomock.Any(), gomock.Any(), gomock.Any())
2149-
21502148
mockISCSIClient.EXPECT().TargetHasMountedDevice(gomock.Any(), gomock.Any()).Return(false, nil)
21512149
mockISCSIClient.EXPECT().SafeToLogOut(gomock.Any(), gomock.Any(), gomock.Any()).Return(true)
21522150
mockISCSIClient.EXPECT().RemovePortalsFromSession(gomock.Any(), gomock.Any(), gomock.Any())
@@ -2160,7 +2158,7 @@ func TestNodeUnstageISCSIVolume(t *testing.T) {
21602158
},
21612159
getDeviceClient: func() devices.Devices {
21622160
mockDeviceClient := mock_devices.NewMockDevices(gomock.NewController(t))
2163-
mockDeviceClient.EXPECT().GetLUKSDevicePathForVolume(gomock.Any(), gomock.Any()).Return(mockDevicePath, nil)
2161+
mockDeviceClient.EXPECT().GetLUKSDeviceForMultipathDevice(gomock.Any()).Return(mockDevicePath, nil)
21642162
mockDeviceClient.EXPECT().EnsureLUKSDeviceClosedWithMaxWaitLimit(gomock.Any(), mockDevicePath).
21652163
Return(nil)
21662164
mockDeviceClient.EXPECT().EnsureLUKSDeviceClosed(gomock.Any(), mockDevicePath).Return(nil)
@@ -2204,7 +2202,7 @@ func TestNodeUnstageISCSIVolume(t *testing.T) {
22042202
},
22052203
getDeviceClient: func() devices.Devices {
22062204
mockDeviceClient := mock_devices.NewMockDevices(gomock.NewController(t))
2207-
mockDeviceClient.EXPECT().GetLUKSDevicePathForVolume(gomock.Any(), gomock.Any()).Return(mockDevicePath, nil)
2205+
mockDeviceClient.EXPECT().GetLUKSDeviceForMultipathDevice(gomock.Any()).Return(mockDevicePath, nil)
22082206
mockDeviceClient.EXPECT().EnsureLUKSDeviceClosedWithMaxWaitLimit(gomock.Any(), mockDevicePath).
22092207
Return(fmt.Errorf("mock error"))
22102208
return mockDeviceClient
@@ -2222,7 +2220,7 @@ func TestNodeUnstageISCSIVolume(t *testing.T) {
22222220
},
22232221
getDeviceClient: func() devices.Devices {
22242222
mockDeviceClient := mock_devices.NewMockDevices(gomock.NewController(t))
2225-
mockDeviceClient.EXPECT().GetLUKSDevicePathForVolume(gomock.Any(), gomock.Any()).Return(mockDevicePath, nil)
2223+
mockDeviceClient.EXPECT().GetLUKSDeviceForMultipathDevice(gomock.Any()).Return(mockDevicePath, nil)
22262224
mockDeviceClient.EXPECT().EnsureLUKSDeviceClosedWithMaxWaitLimit(gomock.Any(), mockDevicePath).
22272225
Return(nil)
22282226
mockDeviceClient.EXPECT().RemoveMultipathDeviceMappingWithRetries(gomock.Any(), gomock.Any(),
@@ -2295,8 +2293,7 @@ func TestNodeUnstageISCSIVolume(t *testing.T) {
22952293
return mockDeviceClient
22962294
},
22972295
},
2298-
// mockDeviceClient.EXPECT().GetLUKSDevicePathForVolume(gomock.Any(), gomock.Any()).Return(mockDevicePath, nil)
2299-
"SAN: iSCSI unstage: GetLUKSDevicePathForVolume error": {
2296+
"SAN: iSCSI unstage: GetLUKSDeviceForMultipathDevice error": {
23002297
assertError: assert.Error,
23012298
request: NewNodeUnstageVolumeRequestBuilder().Build(),
23022299
publishInfo: NewVolumePublishInfoBuilder(TypeiSCSIVolumePublishInfo).WithLUKSEncryption("true").Build(),
@@ -2315,8 +2312,8 @@ func TestNodeUnstageISCSIVolume(t *testing.T) {
23152312
},
23162313
getDeviceClient: func() devices.Devices {
23172314
mockDeviceClient := mock_devices.NewMockDevices(gomock.NewController(t))
2318-
mockDeviceClient.EXPECT().GetLUKSDevicePathForVolume(gomock.Any(),
2319-
gomock.Any()).Return(mockDevicePath, errors.New("mock error"))
2315+
mockDeviceClient.EXPECT().GetLUKSDeviceForMultipathDevice(gomock.Any()).Return("", fmt.Errorf(
2316+
"mock error"))
23202317
return mockDeviceClient
23212318
},
23222319
},
@@ -2335,7 +2332,7 @@ func TestNodeUnstageISCSIVolume(t *testing.T) {
23352332
},
23362333
getDeviceClient: func() devices.Devices {
23372334
mockDeviceClient := mock_devices.NewMockDevices(gomock.NewController(t))
2338-
mockDeviceClient.EXPECT().GetLUKSDevicePathForVolume(gomock.Any(), gomock.Any()).Return(mockDevicePath, nil)
2335+
mockDeviceClient.EXPECT().GetLUKSDeviceForMultipathDevice(gomock.Any()).Return(mockDevicePath, nil)
23392336
mockDeviceClient.EXPECT().EnsureLUKSDeviceClosedWithMaxWaitLimit(gomock.Any(), mockDevicePath).
23402337
Return(nil)
23412338
return mockDeviceClient
@@ -2368,7 +2365,7 @@ func TestNodeUnstageISCSIVolume(t *testing.T) {
23682365
},
23692366
getDeviceClient: func() devices.Devices {
23702367
mockDeviceClient := mock_devices.NewMockDevices(gomock.NewController(t))
2371-
mockDeviceClient.EXPECT().GetLUKSDevicePathForVolume(gomock.Any(), gomock.Any()).Return(mockDevicePath, nil)
2368+
mockDeviceClient.EXPECT().GetLUKSDeviceForMultipathDevice(gomock.Any()).Return(mockDevicePath, nil)
23722369
mockDeviceClient.EXPECT().EnsureLUKSDeviceClosedWithMaxWaitLimit(gomock.Any(), mockDevicePath).
23732370
Return(nil)
23742371
mockDeviceClient.EXPECT().EnsureLUKSDeviceClosed(gomock.Any(), mockDevicePath).Return(nil)

‎frontend/csi/volume_publish_manager_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,8 @@ func TestReadTrackingInfo(t *testing.T) {
111111
mockJSONUtils.EXPECT().ReadJSONFile(gomock.Any(), emptyTrackInfo, fName, "volume tracking info").
112112
SetArg(1, *trackInfo).Return(nil)
113113
trackInfo, err := v.ReadTrackingInfo(context.Background(), volId)
114-
assert.NoError(t, err, "no error expected when write succeed")
115-
assert.NotNil(t, trackInfo, "expected a valid tracking info")
116114
assert.Equal(t, fsType, trackInfo.FilesystemType, "tracking file did not have expected value in it")
115+
assert.NoError(t, err, "tracking file should have been written")
117116

118117
emptyTrackInfo = &models.VolumeTrackingInfo{}
119118
mockJSONUtils.EXPECT().ReadJSONFile(gomock.Any(), emptyTrackInfo, fName,

‎mocks/mock_utils/mock_devices/mock_devices_client.go‎

Lines changed: 6 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)