Skip to content

Commit ed81120

Browse files
torirevillatorirevilla
authored
Export policy fix NAS-Economy
Ensure the correct export policy is used even when the volume config does not have a stored value.
1 parent 269335e commit ed81120

File tree

3 files changed

+143
-26
lines changed

3 files changed

+143
-26
lines changed

‎storage_drivers/ontap/api/ontap_rest.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4755,7 +4755,7 @@ func (c RestClient) QtreeGet(ctx context.Context, name, volumePrefix string) (*m
47554755
params.SetSvmUUID(convert.ToPtr(c.svmUUID))
47564756
params.SetName(convert.ToPtr(name)) // qtree name
47574757
params.SetVolumeName(convert.ToPtr(pattern)) // Flexvol name prefix
4758-
fields := []string{""}
4758+
fields := []string{"export_policy.name"}
47594759
params.SetFields(fields)
47604760

47614761
result, err := c.api.Storage.QtreeCollectionGet(params, c.authInfo)

‎storage_drivers/ontap/ontap_nas_qtree.go‎

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -718,11 +718,34 @@ func (d *NASQtreeStorageDriver) publishQtreeShare(
718718
}
719719

720720
// Ensure the qtree has the correct export policy and rules applied.
721+
backendPolicyName := getExportPolicyName(publishInfo.BackendUUID)
722+
qtreePolicyName := qtree
723+
// Trident versions <23.04 may not have the exportPolicy field set in the volume config,
724+
// if this is the case we need to use the export policy from the qtree info from ONTAP.
725+
if volConfig.ExportPolicy != backendPolicyName && volConfig.ExportPolicy != qtreePolicyName &&
726+
volConfig.ExportPolicy != getEmptyExportPolicyName(*d.Config.StoragePrefix) {
727+
backendQtree, err := d.API.QtreeGetByName(ctx, qtree, flexvol)
728+
if err != nil {
729+
return err
730+
}
731+
if backendQtree == nil {
732+
return errors.NotFoundError(fmt.Sprintf("qtree %s not found", qtree))
733+
}
734+
volConfig.ExportPolicy = backendQtree.ExportPolicy
735+
}
736+
721737
// If the qtree already have backend policy, leave it as it is. We will have an opportunity to migrate it
722738
// to qtree policy during unpublish.
723-
qtreePolicyName := qtree
724-
if volConfig.ExportPolicy == getExportPolicyName(publishInfo.BackendUUID) {
725-
qtreePolicyName = volConfig.ExportPolicy
739+
switch volConfig.ExportPolicy {
740+
case getEmptyExportPolicyName(*d.Config.StoragePrefix), qtree:
741+
qtreePolicyName = qtree
742+
case backendPolicyName:
743+
qtreePolicyName = backendPolicyName
744+
default:
745+
// This can happen if the customer switched from autoExportPolicy=false to true and the volume is still using
746+
// default or user supplied export policy.
747+
Logc(ctx).Debugf("Export policy %s is not managed by Trident.", volConfig.ExportPolicy)
748+
return nil
726749
}
727750

728751
volConfig.ExportPolicy = qtreePolicyName
@@ -743,8 +766,6 @@ func (d *NASQtreeStorageDriver) publishQtreeShare(
743766
}
744767

745768
// Ensure the parent flex-vol has the correct export policy and rules applied
746-
backendPolicyName := getExportPolicyName(publishInfo.BackendUUID)
747-
748769
if err := ensureNodeAccessForPolicy(ctx, targetNode, d.API, &d.Config, backendPolicyName); err != nil {
749770
return err
750771
}
@@ -796,17 +817,20 @@ func (d *NASQtreeStorageDriver) Unpublish(
796817
Logc(ctx).WithError(err).Error("Error getting volume pattern.")
797818
return err
798819
}
799-
exists, flexvol, err := d.API.QtreeExists(ctx, qtreeName, volumePattern)
820+
qtree, err := d.API.QtreeGetByName(ctx, qtreeName, volumePattern)
800821
if err != nil {
801822
Logc(ctx).WithError(err).Error("Error checking for existing qtree.")
802823
return err
803824
}
804-
if !exists {
825+
if qtree == nil {
805826
Logc(ctx).WithField("qtree", qtreeName).Debug("Qtree not found.")
806827
return errors.NotFoundError("qtree not found")
807828
}
808829

809830
exportPolicy := volConfig.ExportPolicy
831+
if exportPolicy == "" {
832+
exportPolicy = qtree.ExportPolicy
833+
}
810834
if exportPolicy == qtreeName {
811835
// Remove export policy rules matching the node IP address from qtree level policy
812836
if err = removeExportPolicyRules(ctx, exportPolicy, publishInfo, d.API); err != nil {
@@ -822,7 +846,7 @@ func (d *NASQtreeStorageDriver) Unpublish(
822846
}
823847
if len(allExportRules) == 0 {
824848
// Set qtree to the empty policy
825-
if err = d.setQtreeToEmptyPolicy(ctx, qtreeName, flexvol); err != nil {
849+
if err = d.setQtreeToEmptyPolicy(ctx, qtreeName, qtree.Volume); err != nil {
826850
return err
827851
}
828852

@@ -837,7 +861,7 @@ func (d *NASQtreeStorageDriver) Unpublish(
837861
} else if exportPolicy == getExportPolicyName(publishInfo.BackendUUID) {
838862
// Qtree using backend-based export policy.
839863
if len(publishInfo.Nodes) == 0 {
840-
if err = d.setQtreeToEmptyPolicy(ctx, qtreeName, flexvol); err != nil {
864+
if err = d.setQtreeToEmptyPolicy(ctx, qtreeName, qtree.Volume); err != nil {
841865
return err
842866
}
843867
volConfig.ExportPolicy = getEmptyExportPolicyName(*d.Config.StoragePrefix)

‎storage_drivers/ontap/ontap_nas_qtree_test.go‎

Lines changed: 109 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1164,6 +1164,75 @@ func TestPublish_WithErrorInApiOperation(t *testing.T) {
11641164
assert.Error(t, result2, "Expected error when qtree does not exist, got nil")
11651165
}
11661166

1167+
func TestPublishQtreeShare_WithDefaultPolicy_Success(t *testing.T) {
1168+
qtreeName := "testQtree"
1169+
flexVolName := "testFlexVol"
1170+
nodeIP := "1.1.1.1"
1171+
defaultPolicy := "default"
1172+
1173+
nodes := make([]*models.Node, 0)
1174+
nodes = append(nodes, &models.Node{Name: "node1", IPs: []string{nodeIP}})
1175+
1176+
publishInfo := models.VolumePublishInfo{
1177+
BackendUUID: BackendUUID,
1178+
Unmanaged: false,
1179+
Nodes: nodes,
1180+
HostName: "node1",
1181+
}
1182+
1183+
// Create mock driver and api
1184+
mockAPI, driver := newMockOntapNasQtreeDriver(t)
1185+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, flexVolName).Return(
1186+
&api.Qtree{ExportPolicy: defaultPolicy, Volume: flexVolName}, nil)
1187+
1188+
// This handles the case where the customer originally created a backend with autoExportPolicy set to false and
1189+
// mounted a volume and then later changed autoExportPolicy to true
1190+
volConfig := &storage.VolumeConfig{ExportPolicy: defaultPolicy}
1191+
driver.Config.AutoExportPolicy = true
1192+
1193+
result := driver.publishQtreeShare(ctx, qtreeName, flexVolName, volConfig, &publishInfo)
1194+
1195+
assert.NoError(t, result, "Expected no error in publishQtreeShare, got error")
1196+
}
1197+
1198+
func TestPublishQtreeShare_LegacyVolumeWithEmptyPolicyInConfig_Success(t *testing.T) {
1199+
qtreeName := "testQtree"
1200+
flexVolName := "testFlexVol"
1201+
nodeIP := "1.1.1.1"
1202+
backendPolicy := getExportPolicyName(BackendUUID)
1203+
1204+
nodes := make([]*models.Node, 0)
1205+
nodes = append(nodes, &models.Node{Name: "node1", IPs: []string{nodeIP}})
1206+
1207+
publishInfo := models.VolumePublishInfo{
1208+
BackendUUID: BackendUUID,
1209+
Unmanaged: false,
1210+
Nodes: nodes,
1211+
HostName: "node1",
1212+
}
1213+
1214+
// Create mock driver and api
1215+
mockAPI, driver := newMockOntapNasQtreeDriver(t)
1216+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, flexVolName).Return(
1217+
&api.Qtree{ExportPolicy: backendPolicy, Volume: flexVolName}, nil)
1218+
mockAPI.EXPECT().ExportPolicyExists(ctx, gomock.Any()).AnyTimes().Return(true, nil)
1219+
// Return node ip rules when asked for them
1220+
mockAPI.EXPECT().ExportRuleList(gomock.Any(), backendPolicy).Return(map[string]int{"1.1.1.1": 1}, nil)
1221+
mockAPI.EXPECT().ExportRuleList(gomock.Any(), backendPolicy).Return(map[string]int{"1.1.1.1": 1}, nil)
1222+
mockAPI.EXPECT().QtreeModifyExportPolicy(ctx, qtreeName, flexVolName, backendPolicy).AnyTimes().Return(nil)
1223+
mockAPI.EXPECT().VolumeModifyExportPolicy(ctx, flexVolName, backendPolicy).AnyTimes().Return(nil)
1224+
1225+
// This handles the case where the customer originally created and mounted a volume in trident version <=23.01
1226+
// and then later upgraded to 24.10. This would have the volConfig.ExportPolicy = "".
1227+
driver.Config.AutoExportPolicy = true
1228+
driver.Config.AutoExportCIDRs = []string{"0.0.0.0/0"}
1229+
volConfig := &storage.VolumeConfig{ExportPolicy: ""}
1230+
1231+
result := driver.publishQtreeShare(ctx, qtreeName, flexVolName, volConfig, &publishInfo)
1232+
1233+
assert.NoError(t, result, "Expected no error in publishQtreeShare, got error")
1234+
}
1235+
11671236
func TestPublishQtreeShare_WithEmptyPolicy_Success(t *testing.T) {
11681237
qtreeName := "testQtree"
11691238
flexVolName := "testFlexVol"
@@ -1180,7 +1249,7 @@ func TestPublishQtreeShare_WithEmptyPolicy_Success(t *testing.T) {
11801249
HostName: "node1",
11811250
}
11821251

1183-
volConfig := &storage.VolumeConfig{ExportPolicy: "trident_empty"}
1252+
volConfig := &storage.VolumeConfig{ExportPolicy: getEmptyExportPolicyName("trident")}
11841253

11851254
// Create mock driver and api
11861255
mockAPI, driver := newMockOntapNasQtreeDriver(t)
@@ -1296,7 +1365,7 @@ func TestPublishQtreeShare_WithErrorInApiOperation(t *testing.T) {
12961365
HostName: "node1",
12971366
}
12981367

1299-
volConfig := &storage.VolumeConfig{ExportPolicy: "trident_empty"}
1368+
volConfig := &storage.VolumeConfig{ExportPolicy: getEmptyExportPolicyName("trident")}
13001369

13011370
// CASE 1: Error in checking if export policy exists
13021371
mockAPI, driver := newMockOntapNasQtreeDriver(t)
@@ -5871,7 +5940,8 @@ func TestOntapNasEcoUnpublish(t *testing.T) {
58715940
name: "qtreeWithOneMount",
58725941
args: args{publishEnforcement: false, autoExportPolicy: true},
58735942
mocks: func(mockAPI *mockapi.MockOntapAPI, qtreeName, flexVolName, backendPolicy string) {
5874-
mockAPI.EXPECT().QtreeExists(ctx, qtreeName, gomock.Any()).Return(true, flexVolName, nil)
5943+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, gomock.Any()).
5944+
Return(&api.Qtree{ExportPolicy: qtreeName, Volume: flexVolName}, nil)
58755945
mockAPI.EXPECT().ExportRuleList(ctx, qtreeName).
58765946
Return(map[string]int{"1.1.1.1": 1, "2.2.2.2": 2}, nil)
58775947
mockAPI.EXPECT().ExportRuleDestroy(ctx, qtreeName, gomock.Any()).Times(2)
@@ -5885,7 +5955,8 @@ func TestOntapNasEcoUnpublish(t *testing.T) {
58855955
name: "emptyPolicyDoesNotExist",
58865956
args: args{publishEnforcement: false, autoExportPolicy: true},
58875957
mocks: func(mockAPI *mockapi.MockOntapAPI, qtreeName, flexVolName, backendPolicy string) {
5888-
mockAPI.EXPECT().QtreeExists(ctx, qtreeName, gomock.Any()).Return(true, flexVolName, nil)
5958+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, gomock.Any()).
5959+
Return(&api.Qtree{ExportPolicy: qtreeName, Volume: flexVolName}, nil)
58895960
mockAPI.EXPECT().ExportRuleList(ctx, qtreeName).
58905961
Return(map[string]int{"1.1.1.1": 1, "2.2.2.2": 2}, nil)
58915962
mockAPI.EXPECT().ExportRuleDestroy(ctx, qtreeName, gomock.Any()).Times(2)
@@ -5906,7 +5977,8 @@ func TestOntapNasEcoUnpublish(t *testing.T) {
59065977
name: "qtreeWithTwoMounts",
59075978
args: args{publishEnforcement: false, autoExportPolicy: true},
59085979
mocks: func(mockAPI *mockapi.MockOntapAPI, qtreeName, flexVolName, backendPolicy string) {
5909-
mockAPI.EXPECT().QtreeExists(ctx, qtreeName, gomock.Any()).Return(true, flexVolName, nil)
5980+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, gomock.Any()).
5981+
Return(&api.Qtree{ExportPolicy: qtreeName, Volume: flexVolName}, nil)
59105982
mockAPI.EXPECT().ExportRuleList(ctx, qtreeName).
59115983
Return(map[string]int{"1.1.1.1": 1, "2.2.2.2": 2, "4.4.4.4": 4, "5.5.5.5": 5}, nil)
59125984
mockAPI.EXPECT().ExportRuleDestroy(ctx, qtreeName, gomock.Any()).Times(2)
@@ -5918,24 +5990,26 @@ func TestOntapNasEcoUnpublish(t *testing.T) {
59185990
name: "qtreeDoesNotExist",
59195991
args: args{publishEnforcement: false, autoExportPolicy: true},
59205992
mocks: func(mockAPI *mockapi.MockOntapAPI, qtreeName, flexVolName, backendPolicy string) {
5921-
mockAPI.EXPECT().QtreeExists(ctx, qtreeName, gomock.Any()).Return(false, "", nil)
5993+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, gomock.Any()).
5994+
Return(nil, fmt.Errorf("qtree does not exist"))
59225995
},
59235996
wantErr: assert.Error,
59245997
},
59255998
{
59265999
name: "qtreeExistError",
59276000
args: args{publishEnforcement: false, autoExportPolicy: true},
59286001
mocks: func(mockAPI *mockapi.MockOntapAPI, qtreeName, flexVolName, backendPolicy string) {
5929-
mockAPI.EXPECT().QtreeExists(ctx, qtreeName, gomock.Any()).Return(false, "",
5930-
fmt.Errorf("some api error"))
6002+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, gomock.Any()).
6003+
Return(nil, fmt.Errorf("some api error"))
59316004
},
59326005
wantErr: assert.Error,
59336006
},
59346007
{
59356008
name: "exportRuleListError",
59366009
args: args{publishEnforcement: false, autoExportPolicy: true},
59376010
mocks: func(mockAPI *mockapi.MockOntapAPI, qtreeName, flexVolName, backendPolicy string) {
5938-
mockAPI.EXPECT().QtreeExists(ctx, qtreeName, gomock.Any()).Return(true, flexVolName, nil)
6011+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, gomock.Any()).
6012+
Return(&api.Qtree{ExportPolicy: qtreeName, Volume: flexVolName}, nil)
59396013
mockAPI.EXPECT().ExportRuleList(ctx, qtreeName).Return(nil, fmt.Errorf("some api error"))
59406014
},
59416015
wantErr: assert.Error,
@@ -5979,6 +6053,7 @@ func TestOntapNasEcoLegacyUnpublish(t *testing.T) {
59796053

59806054
type args struct {
59816055
autoExportPolicy bool
6056+
exportPolicy string
59826057
nodeNum int
59836058
}
59846059

@@ -5994,9 +6069,25 @@ func TestOntapNasEcoLegacyUnpublish(t *testing.T) {
59946069
// After unpublish, the qtree is expected to be set to the empty export policy with no rules because it
59956070
// is no longer in use by any pods on any node.
59966071
name: "legacyQtreeWithOneMount",
5997-
args: args{autoExportPolicy: true, nodeNum: 0},
6072+
args: args{autoExportPolicy: true, nodeNum: 0, exportPolicy: "trident-1234"},
6073+
mocks: func(mockAPI *mockapi.MockOntapAPI, qtreeName, flexVolName, backendPolicy string) {
6074+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, gomock.Any()).
6075+
Return(&api.Qtree{ExportPolicy: backendPolicy, Volume: flexVolName}, nil)
6076+
mockAPI.EXPECT().QtreeModifyExportPolicy(ctx, qtreeName, flexVolName, "tridentempty")
6077+
},
6078+
wantErr: assert.NoError,
6079+
},
6080+
{
6081+
// This qtree has the backend based export policy "trident-1234" but its volConfig.ExportPolicy="" because
6082+
// this qtree vol was created using trident version <= 23.01. During Unpublish, the correct export policy
6083+
// should be querired from backend and used.
6084+
// After unpublish, the qtree is expected to be set to the empty export policy with no rules because it
6085+
// is no longer in use by any pods on any node.
6086+
name: "legacyQtreeWithEmptyExportPolicyInVolConfig",
6087+
args: args{autoExportPolicy: true, nodeNum: 0, exportPolicy: ""},
59986088
mocks: func(mockAPI *mockapi.MockOntapAPI, qtreeName, flexVolName, backendPolicy string) {
5999-
mockAPI.EXPECT().QtreeExists(ctx, qtreeName, gomock.Any()).Return(true, flexVolName, nil)
6089+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, gomock.Any()).
6090+
Return(&api.Qtree{ExportPolicy: backendPolicy, Volume: flexVolName}, nil)
60006091
mockAPI.EXPECT().QtreeModifyExportPolicy(ctx, qtreeName, flexVolName, "tridentempty")
60016092
},
60026093
wantErr: assert.NoError,
@@ -6006,9 +6097,10 @@ func TestOntapNasEcoLegacyUnpublish(t *testing.T) {
60066097
// After unpublish, the qtree is expected to be set to the empty export policy with no rules because it
60076098
// is no longer in use by any pods on any node.
60086099
name: "emptyPolicyDoesNotExist",
6009-
args: args{autoExportPolicy: true, nodeNum: 0},
6100+
args: args{autoExportPolicy: true, nodeNum: 0, exportPolicy: "trident-1234"},
60106101
mocks: func(mockAPI *mockapi.MockOntapAPI, qtreeName, flexVolName, backendPolicy string) {
6011-
mockAPI.EXPECT().QtreeExists(ctx, qtreeName, gomock.Any()).Return(true, flexVolName, nil)
6102+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, gomock.Any()).
6103+
Return(&api.Qtree{ExportPolicy: backendPolicy, Volume: flexVolName}, nil)
60126104
mockAPI.EXPECT().QtreeModifyExportPolicy(ctx, qtreeName, flexVolName, "tridentempty").
60136105
Return(errors.NotFoundError("export policy not found"))
60146106
mockAPI.EXPECT().ExportPolicyCreate(ctx, "tridentempty")
@@ -6020,9 +6112,10 @@ func TestOntapNasEcoLegacyUnpublish(t *testing.T) {
60206112
// This qtree has the backend based export policy "trident-1234" and has more than one publication.
60216113
// After unpublish, the qtree is expected to continue to use the backend based export policy.
60226114
name: "legacyQtreeWithMultipleMounts",
6023-
args: args{autoExportPolicy: true, nodeNum: 2},
6115+
args: args{autoExportPolicy: true, nodeNum: 2, exportPolicy: "trident-1234"},
60246116
mocks: func(mockAPI *mockapi.MockOntapAPI, qtreeName, flexVolName, backendPolicy string) {
6025-
mockAPI.EXPECT().QtreeExists(ctx, qtreeName, gomock.Any()).Return(true, flexVolName, nil)
6117+
mockAPI.EXPECT().QtreeGetByName(ctx, qtreeName, gomock.Any()).
6118+
Return(&api.Qtree{ExportPolicy: backendPolicy, Volume: flexVolName}, nil)
60266119
},
60276120
wantErr: assert.NoError,
60286121
},
@@ -6032,7 +6125,6 @@ func TestOntapNasEcoLegacyUnpublish(t *testing.T) {
60326125
t.Run(tr.name, func(t *testing.T) {
60336126
volConfig := &storage.VolumeConfig{
60346127
InternalName: "trident_pvc_123",
6035-
ExportPolicy: "trident-1234",
60366128
}
60376129

60386130
publishInfo := &models.VolumePublishInfo{
@@ -6044,6 +6136,7 @@ func TestOntapNasEcoLegacyUnpublish(t *testing.T) {
60446136

60456137
mockAPI, driver := newMockOntapNasQtreeDriver(t)
60466138
driver.Config.AutoExportPolicy = tr.args.autoExportPolicy
6139+
volConfig.ExportPolicy = tr.args.exportPolicy
60476140

60486141
mockAPI.EXPECT().SVMName().AnyTimes().Return("SVM1")
60496142
tr.mocks(mockAPI, volConfig.InternalName, "flexVolName", getExportPolicyName(publishInfo.BackendUUID))

0 commit comments

Comments
 (0)