Skip to content

Commit

Permalink
fix unmanaged import for SAN drivers
Browse files Browse the repository at this point in the history
maps already-mapped luns to per-node igroup
  • Loading branch information
reederc42 authored Apr 26, 2023
1 parent 5ce2d98 commit 1f5192c
Show file tree
Hide file tree
Showing 13 changed files with 38 additions and 226 deletions.
8 changes: 4 additions & 4 deletions mocks/mock_storage_drivers/mock_ontap/mock_api.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions storage_drivers/ontap/api/abstraction.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ type OntapAPI interface {
LunGetByName(ctx context.Context, name string) (*Lun, error)
LunRename(ctx context.Context, lunPath, newLunPath string) error
LunMapInfo(ctx context.Context, initiatorGroupName, lunPath string) (int, error)
EnsureLunMapped(ctx context.Context, initiatorGroupName, lunPath string, importNotManaged bool) (int, error)
EnsureLunMapped(ctx context.Context, initiatorGroupName, lunPath string) (int, error)
LunUnmap(ctx context.Context, initiatorGroupName, lunPath string) error
LunSize(ctx context.Context, lunPath string) (int, error)
LunSetSize(ctx context.Context, lunPath, newSize string) (uint64, error)
Expand All @@ -99,7 +99,7 @@ type OntapAPI interface {

IscsiInitiatorGetDefaultAuth(ctx context.Context) (IscsiInitiatorAuth, error)
IscsiInitiatorSetDefaultAuth(
ctx context.Context, authType, userName, passphrase, outbountUserName,
ctx context.Context, authType, userName, passphrase, outboundUserName,
outboundPassphrase string,
) error
IscsiInterfaceGet(ctx context.Context, svm string) ([]string, error)
Expand Down
12 changes: 5 additions & 7 deletions storage_drivers/ontap/api/abstraction_rest.go
Original file line number Diff line number Diff line change
Expand Up @@ -1990,7 +1990,7 @@ func (d OntapAPIREST) LunMapInfo(ctx context.Context, initiatorGroupName, lunPat
}

func (d OntapAPIREST) isLunMapped(
ctx context.Context, lunPath, initiatorGroupName string, importNotManaged bool,
ctx context.Context, lunPath, initiatorGroupName string,
) (bool, int, error) {
alreadyMapped := false
lunID := -1
Expand All @@ -2013,9 +2013,9 @@ func (d OntapAPIREST) isLunMapped(
for _, record := range lunMapResponse.Payload.LunMapResponseInlineRecords {
if record.Igroup != nil && record.Igroup.Name != nil {
if *record.Igroup.Name != initiatorGroupName {
Logc(ctx).Debugf("LUN %s is mapped to igroup %s.", lunPath, record.Igroup.Name)
Logc(ctx).Debugf("LUN %s is mapped to igroup %s.", lunPath, *record.Igroup.Name)
}
if *record.Igroup.Name == initiatorGroupName || importNotManaged {
if *record.Igroup.Name == initiatorGroupName {
if record.LogicalUnitNumber != nil {
lunID = int(*record.LogicalUnitNumber)
alreadyMapped = true
Expand Down Expand Up @@ -2053,10 +2053,8 @@ func (d OntapAPIREST) isLunMapped(
return alreadyMapped, lunID, nil
}

func (d OntapAPIREST) EnsureLunMapped(
ctx context.Context, initiatorGroupName, lunPath string, importNotManaged bool,
) (int, error) {
alreadyMapped, lunID, err := d.isLunMapped(ctx, lunPath, initiatorGroupName, importNotManaged)
func (d OntapAPIREST) EnsureLunMapped(ctx context.Context, initiatorGroupName, lunPath string) (int, error) {
alreadyMapped, lunID, err := d.isLunMapped(ctx, lunPath, initiatorGroupName)
if err != nil {
return -1, err
}
Expand Down
14 changes: 7 additions & 7 deletions storage_drivers/ontap/api/abstraction_rest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,28 +82,28 @@ func TestEnsureLunMapped(t *testing.T) {

// lunMapInfo returning error
rsi.EXPECT().LunMapInfo(ctx, "", lunPath).Return(lunMapCollection, errors.New("error"))
resultLun, err := oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath, true)
resultLun, err := oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath)
assert.Errorf(t, err, "problem reading maps for LUN /dev/sda: error")
assert.Equal(t, -1, resultLun)

// lunMapInfo returning nil lun collection
rsi.EXPECT().LunMapInfo(ctx, "", lunPath).Return(nil, nil)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath, true)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath)
assert.Errorf(t, err, "problem reading maps for LUN /dev/sda")
assert.Equal(t, -1, resultLun)

// positive test case where lun == nil, lunGetByName gets called to find the LUN details
lun := &models.Lun{LunInlineLunMaps: []*models.LunInlineLunMapsInlineArrayItem{{LogicalUnitNumber: number}}}
rsi.EXPECT().LunGetByName(ctx, lunPath).Return(lun, nil)
rsi.EXPECT().LunMapInfo(ctx, "", lunPath).Return(lunMapCollection, nil)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath, true)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath)
assert.Nil(t, err)
assert.Equal(t, int(*number), resultLun)

// record.LogicalUnitNumber == nil and lunGetByName returns error
rsi.EXPECT().LunGetByName(ctx, lunPath).Return(nil, errors.New("error getting LUN by name"))
rsi.EXPECT().LunMapInfo(ctx, "", lunPath).Return(lunMapCollection, nil)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath, true)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath)
assert.Errorf(t, err, "error getting LUN by name")
assert.Equal(t, -1, resultLun)

Expand All @@ -114,23 +114,23 @@ func TestEnsureLunMapped(t *testing.T) {
rsi.EXPECT().LunGetByName(ctx, lunPath).Return(nil, nil)
rsi.EXPECT().LunMapInfo(ctx, "", lunPath).Return(lunMapCollection, nil)
rsi.EXPECT().LunMap(ctx, initiatorGroup, lunPath, -1).Return(lunMapCreated, nil)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath, true)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath)
assert.NoError(t, err)
// As LogicalUnitNumber == nil currently, -1 is returned
assert.Equal(t, -1, resultLun)

// positive test case where record.LogicalUnitNumber != nil
lunMapCollection.Payload.LunMapResponseInlineRecords[0].LogicalUnitNumber = number
rsi.EXPECT().LunMapInfo(ctx, "", lunPath).Return(lunMapCollection, nil)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath, true)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath)
assert.Nil(t, err)
assert.Equal(t, int(*number), resultLun)

// If lun not already mapped OR incorrectly mapped
lunMapCollection.Payload.LunMapResponseInlineRecords[0].Igroup.Name = utils.Ptr("tmp")
rsi.EXPECT().LunMapInfo(ctx, "", lunPath).Return(lunMapCollection, nil)
rsi.EXPECT().LunMap(ctx, initiatorGroup, lunPath, -1).Return(lunMapCreated, nil)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath, false)
resultLun, err = oapi.EnsureLunMapped(ctx, initiatorGroup, lunPath)
assert.Nil(t, err)
assert.Equal(t, int(*number), resultLun)
}
Expand Down
6 changes: 2 additions & 4 deletions storage_drivers/ontap/api/abstraction_zapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -570,10 +570,8 @@ func (d OntapAPIZAPI) LunMapInfo(ctx context.Context, initiatorGroupName, lunPat
return lunID, nil
}

func (d OntapAPIZAPI) EnsureLunMapped(
ctx context.Context, initiatorGroupName, lunPath string, importNotManaged bool,
) (int, error) {
return d.api.LunMapIfNotMapped(ctx, initiatorGroupName, lunPath, importNotManaged)
func (d OntapAPIZAPI) EnsureLunMapped(ctx context.Context, initiatorGroupName, lunPath string) (int, error) {
return d.api.LunMapIfNotMapped(ctx, initiatorGroupName, lunPath)
}

func (d OntapAPIZAPI) LunSize(ctx context.Context, flexvolName string) (int, error) {
Expand Down
6 changes: 2 additions & 4 deletions storage_drivers/ontap/api/ontap_zapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -458,9 +458,7 @@ func (c Client) LunMapAutoID(initiatorGroupName, lunPath string) (*azgo.LunMapRe
return response, err
}

func (c Client) LunMapIfNotMapped(
ctx context.Context, initiatorGroupName, lunPath string, importNotManaged bool,
) (int, error) {
func (c Client) LunMapIfNotMapped(ctx context.Context, initiatorGroupName, lunPath string) (int, error) {
// Read LUN maps to see if the LUN is already mapped to the igroup
lunMapListResponse, err := c.LunMapListInfo(lunPath)
if err != nil {
Expand All @@ -483,7 +481,7 @@ func (c Client) LunMapIfNotMapped(
if igroup.InitiatorGroupName() != initiatorGroupName {
Logc(ctx).Debugf("LUN %s is mapped to igroup %s.", lunPath, igroup.InitiatorGroupName())
}
if igroup.InitiatorGroupName() == initiatorGroupName || importNotManaged {
if igroup.InitiatorGroupName() == initiatorGroupName {

lunID = igroup.LunId()
alreadyMapped = true
Expand Down
2 changes: 1 addition & 1 deletion storage_drivers/ontap/api/ontap_zapi_interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ type ZapiClientInterface interface {
// LunMapAutoID maps a LUN in an initiator group, allowing ONTAP to choose an available LUN ID
// equivalent to filer::> lun map -vserver iscsi_vs -path /vol/v/lun1 -igroup docker
LunMapAutoID(initiatorGroupName, lunPath string) (*azgo.LunMapResponse, error)
LunMapIfNotMapped(ctx context.Context, initiatorGroupName, lunPath string, importNotManaged bool) (int, error)
LunMapIfNotMapped(ctx context.Context, initiatorGroupName, lunPath string) (int, error)
// LunMapListInfo returns lun mapping information for the specified lun
// equivalent to filer::> lun mapped show -vserver iscsi_vs -path /vol/v/lun0
LunMapListInfo(lunPath string) (*azgo.LunMapListInfoResponse, error)
Expand Down
10 changes: 2 additions & 8 deletions storage_drivers/ontap/ontap_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -431,15 +431,9 @@ func reconcileSANNodeAccess(
"backendUUID": backendUUID,
}).Debug("Attempting to delete unused igroups")
for _, igroup := range igroups {
luns, err := clientAPI.IgroupListLUNsMapped(ctx, igroup)
if err != nil {
if err := DestroyUnmappedIgroup(ctx, clientAPI, igroup); err != nil {
return err
}
if len(luns) == 0 {
if err := clientAPI.IgroupDestroy(ctx, igroup); err != nil {
return err
}
}
}

return nil
Expand Down Expand Up @@ -649,7 +643,7 @@ func PublishLUN(
}

// Map LUN (it may already be mapped)
lunID, err := clientAPI.EnsureLunMapped(ctx, igroupName, lunPath, publishInfo.Unmanaged)
lunID, err := clientAPI.EnsureLunMapped(ctx, igroupName, lunPath)
if err != nil {
return err
}
Expand Down
6 changes: 3 additions & 3 deletions storage_drivers/ontap/ontap_common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3846,7 +3846,7 @@ func TestPublishLun(t *testing.T) {
// Test1 - Positive flow
mockAPI.EXPECT().LunGetFSType(ctx, lunPath).Return("fstype", nil)
mockAPI.EXPECT().EnsureIgroupAdded(ctx, igroupName, publishInfo.HostIQN[0])
mockAPI.EXPECT().EnsureLunMapped(ctx, igroupName, lunPath, publishInfo.Unmanaged).Return(1111, nil)
mockAPI.EXPECT().EnsureLunMapped(ctx, igroupName, lunPath).Return(1111, nil)
mockAPI.EXPECT().LunMapGetReportingNodes(ctx, igroupName, lunPath).Return([]string{"Node1"}, nil)
mockAPI.EXPECT().GetSLMDataLifs(ctx, ips, []string{"Node1"}).Return([]string{}, nil)

Expand All @@ -3868,7 +3868,7 @@ func TestPublishLun(t *testing.T) {
publishInfo.HostIQN = []string{"host_iqn"}
mockAPI.EXPECT().LunGetFSType(ctx, lunPath).Return("", fmt.Errorf("LunGetFSType returned error"))
mockAPI.EXPECT().EnsureIgroupAdded(ctx, igroupName, publishInfo.HostIQN[0])
mockAPI.EXPECT().EnsureLunMapped(ctx, igroupName, lunPath, publishInfo.Unmanaged).Return(1111, nil)
mockAPI.EXPECT().EnsureLunMapped(ctx, igroupName, lunPath).Return(1111, nil)
mockAPI.EXPECT().LunMapGetReportingNodes(ctx, igroupName, lunPath).Return([]string{"Node1"}, nil)
mockAPI.EXPECT().GetSLMDataLifs(ctx, ips, []string{"Node1"}).Return([]string{}, nil)

Expand Down Expand Up @@ -3904,7 +3904,7 @@ func TestPublishLun(t *testing.T) {

// Test 6 - EnsureLunMapped returns error
mockAPI.EXPECT().LunGetFSType(ctx, lunPath).Return("fstype", nil)
mockAPI.EXPECT().EnsureLunMapped(ctx, igroupName, lunPath, publishInfo.Unmanaged).Return(1111,
mockAPI.EXPECT().EnsureLunMapped(ctx, igroupName, lunPath).Return(1111,
fmt.Errorf("EnsureLunMapped returned error"))
mockAPI.EXPECT().EnsureIgroupAdded(ctx, igroupName, gomock.Any()).Return(nil)

Expand Down
36 changes: 1 addition & 35 deletions storage_drivers/ontap/ontap_san_economy.go
Original file line number Diff line number Diff line change
Expand Up @@ -1648,41 +1648,7 @@ func (d *SANEconomyStorageDriver) CreateFollowup(ctx context.Context, volConfig
}
Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> CreateFollowup")
defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< CreateFollowup")

if d.Config.DriverContext == tridentconfig.ContextDocker {
Logc(ctx).Debug("No follow-up create actions for Docker.")
return nil
}

// Don't map at create time if publish enforcement is enabled.
if volConfig.AccessInfo.PublishEnforcement {
Logc(ctx).Debug("No follow-up create actions for published enforced CSI volume.")
return nil
}

return d.mapOntapSANLUN(ctx, volConfig)
}

func (d *SANEconomyStorageDriver) mapOntapSANLUN(ctx context.Context, volConfig *storage.VolumeConfig) error {
// Determine which flexvol contains the LUN
exists, flexvol, err := d.LUNExists(ctx, volConfig.InternalName, d.FlexvolNamePrefix())
if err != nil {
return fmt.Errorf("could not determine if LUN %s exists: %v", volConfig.InternalName, err)
}
if !exists {
return fmt.Errorf("could not find LUN %s", volConfig.InternalName)
}
// Map LUN
lunPath := GetLUNPathEconomy(flexvol, volConfig.InternalName)
lunID, err := d.API.EnsureLunMapped(ctx, d.Config.IgroupName, lunPath, volConfig.ImportNotManaged)
if err != nil {
return err
}

err = PopulateOntapLunMapping(ctx, d.API, d.ips, volConfig, lunID, lunPath, d.Config.IgroupName)
if err != nil {
return fmt.Errorf("error mapping LUN for %s driver: %v", d.Name(), err)
}
Logc(ctx).Debug("No follow-up create actions for ontap-san-economy volume.")

return nil
}
Expand Down
Loading

0 comments on commit 1f5192c

Please sign in to comment.