Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Driver] Enhance the volume detachment process from driver #1193

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

[Driver] Enhance the volume detachment process from driver #1193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 53 additions & 10 deletions contrib/drivers/huawei/oceanstor/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -980,9 +980,14 @@ func (c *OceanStorClient) GetHostLunId(hostId, lunId string) (int, error) {
func (c *OceanStorClient) RemoveLunFromLunGroup(lunGrpId, lunId string) error {
url := fmt.Sprintf("/lungroup/associate?ID=%s&ASSOCIATEOBJTYPE=11&ASSOCIATEOBJID=%s", lunGrpId, lunId)
if err := c.request("DELETE", url, nil, nil); err != nil {
if c.checkErrorCode(err, ErrorObjectUnavailable) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deletion of LUN from Group may fail for multiple reasons.
LUN doesn't exist
LUN is busy/masked/mapped
or maybe some system specific issues.
Does ErrorObjectUnavailable covers all?
Also, for debugging it may be a good idea to log the error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, ErrorObjectUnavailable just indicates the LUN is not associated to the specified LUNGROUP.
For other failed circumstances, return error as before.

return nil
}

log.Errorf("Remove lun %s from lun group %s failed, %v", lunId, lunGrpId, err)
return err
}

log.Infof("Remove lun %s from lun group %s success", lunId, lunGrpId)
return nil
}
Expand All @@ -992,16 +997,23 @@ func (c *OceanStorClient) RemoveLunGroupFromMappingView(viewId, lunGrpId string)
log.Infof("Lun group %s has already been removed from mapping view %s", lunGrpId, viewId)
return nil
}

url := "/mappingview/REMOVE_ASSOCIATE"
data := map[string]interface{}{
"ASSOCIATEOBJTYPE": ObjectTypeLunGroup,
"ASSOCIATEOBJID": lunGrpId,
"TYPE": ObjectTypeMappingView,
"ID": viewId}

if err := c.request("PUT", url, data, nil); err != nil {
if c.checkErrorCode(err, ErrorLunGroupNotInMappingView) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log the error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to start another pr to add all these logs.

return nil
}

log.Errorf("Remove lun group %s from mapping view %s failed", lunGrpId, viewId)
return err
}

log.Infof("Remove lun group %s from mapping view %s success", lunGrpId, viewId)
return nil
}
Expand All @@ -1011,58 +1023,93 @@ func (c *OceanStorClient) RemoveHostGroupFromMappingView(viewId, hostGrpId strin
log.Infof("Host group %s has already been removed from mapping view %s", hostGrpId, viewId)
return nil
}

url := "/mappingview/REMOVE_ASSOCIATE"
data := map[string]interface{}{
"ASSOCIATEOBJTYPE": ObjectTypeHostGroup,
"ASSOCIATEOBJID": hostGrpId,
"TYPE": ObjectTypeMappingView,
"ID": viewId}

if err := c.request("PUT", url, data, nil); err != nil {
if c.checkErrorCode(err, ErrorHostGroupNotInMappingView) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above for logging

return nil
}

log.Errorf("Remove host group %s from mapping view %s failed", hostGrpId, viewId)
return err
}

log.Infof("Remove host group %s from mapping view %s success", hostGrpId, viewId)
return nil
}

func (c *OceanStorClient) RemoveHostFromHostGroup(hostGrpId, hostId string) error {

url := fmt.Sprintf("/host/associate?TYPE=14&ID=%s&ASSOCIATEOBJTYPE=21&ASSOCIATEOBJID=%s",
hostGrpId, hostId)
if err := c.request("DELETE", url, nil, nil); err != nil {
if c.checkErrorCode(err, ErrorHostNotInHostGroup) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

return nil
}

log.Errorf("Remove host %s from host group %s failed", hostId, hostGrpId)
return err
}

log.Infof("Remove host %s from host group %s success", hostId, hostGrpId)
return nil
}

func (c *OceanStorClient) RemoveIscsiFromHost(initiator string) error {

url := "/iscsi_initiator/remove_iscsi_from_host"
data := map[string]interface{}{"TYPE": ObjectTypeIscsiInitiator, "ID": initiator}
if err := c.request("PUT", url, data, nil); err != nil {
if c.checkErrorCode(err, ErrorInitiatorNotInHost) {
return nil
}

log.Errorf("Remove initiator %s failed", initiator)
return err
}

log.Infof("Remove initiator %s success", initiator)
return nil
}

func (c *OceanStorClient) DeleteHostGroup(id string) error {
return c.request("DELETE", "/hostgroup/"+id, nil, nil)
err := c.request("DELETE", "/hostgroup/"+id, nil, nil)
if err != nil && c.checkErrorCode(err, ErrorHostGroupNotExist) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be only if the HostGroup doesn't exist? Can there be some other system issues?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only at the situation of hostgroup already not exist, we ignore this error and return success.

return nil
}

return err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have caught ErrorHostGroupNotExist, then we will return nil but if there is some other error we return err. Will that be consistent for the caller?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we return err as before to keep consistent for other errors.

}

func (c *OceanStorClient) DeleteLunGroup(id string) error {
return c.request("DELETE", "/LUNGroup/"+id, nil, nil)
err := c.request("DELETE", "/LUNGroup/"+id, nil, nil)
if err != nil && c.checkErrorCode(err, ErrorObjectUnavailable) {
return nil
}

return err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to what commented at 1085 line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

}

func (c *OceanStorClient) DeleteHost(id string) error {
return c.request("DELETE", "/host/"+id, nil, nil)
err := c.request("DELETE", "/host/"+id, nil, nil)
if err != nil && c.checkErrorCode(err, ErrorHostNotExist) {
return nil
}

return err
}

func (c *OceanStorClient) DeleteMappingView(id string) error {
return c.request("DELETE", "/mappingview/"+id, nil, nil)
err := c.request("DELETE", "/mappingview/"+id, nil, nil)
if err != nil && c.checkErrorCode(err, ErrorMappingViewNotExist) {
return nil
}

return err
}

func (c *OceanStorClient) GetArrayInfo() (*System, error) {
Expand Down Expand Up @@ -1262,10 +1309,6 @@ func (c *OceanStorClient) IsHostAssociatedToHostgroup(hostId string) (bool, erro
return false, nil
}

func (c *OceanStorClient) RemoveHost(hostId string) error {
return c.request("DELETE", fmt.Sprintf("/host/%s", hostId), nil, nil)
}

func (c *OceanStorClient) AddFCPortTohost(hostId string, wwn string) error {
url := fmt.Sprintf("/fc_initiator/%s", wwn)
data := map[string]interface{}{
Expand Down
21 changes: 16 additions & 5 deletions contrib/drivers/huawei/oceanstor/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,22 @@ const (

// Error Code
const (
ErrorConnectToServer = -403
ErrorUnauthorizedToServer = -401
ErrorObjectUnavailable = 1077948996
ErrorHostGroupNotExist = 1077937500
ErrorObjectNameAlreadyExist = 1077948993
ErrorConnectToServer = -403
ErrorUnauthorizedToServer = -401
ErrorObjectUnavailable = 1077948996
ErrorHostGroupNotExist = 1077937500
ErrorObjectNameAlreadyExist = 1077948993
ErrorHostAlreadyInHostGroup = 1077937501
ErrorObjectIDNotUnique = 1077948997
ErrorHostGroupAlreadyInMappingView = 1073804556
ErrorLunGroupAlreadyInMappingView = 1073804560
ErrorLunNotExist = 1077936859
ErrorLunGroupNotInMappingView = 1073804554
ErrorHostGroupNotInMappingView = 1073804552
ErrorHostNotInHostGroup = 1073745412
ErrorHostNotExist = 1077937498
ErrorMappingViewNotExist = 1077951819
ErrorInitiatorNotInHost = 1077950342
)

// misc
Expand Down
17 changes: 12 additions & 5 deletions contrib/drivers/huawei/oceanstor/oceanstor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ func (d *Driver) connectFCUseNoSwitch(opt *pb.CreateVolumeAttachmentOpts, initia
}

if wwnsInHost == nil && iqnsInHost == nil && flag == false {
if err = d.client.RemoveHost(hostId); err != nil {
if err = d.client.DeleteHost(hostId); err != nil {
return nil, nil, err
}
}
Expand Down Expand Up @@ -739,19 +739,26 @@ func (d *Driver) deleteZoneAndRemoveFCInitiators(wwns []string, hostId, hostGrpI
func (d *Driver) getMappedInfo(hostName string) (string, string, string, string, error) {
hostId, err := d.client.GetHostIdByName(hostName)
if err != nil {
if IsNotFoundError(err) {
log.Warningf("host(%s) has been removed already, ignore it.", hostName)
return "", "", "", "", nil
}

return "", "", "", "", err
}

lunGrpId, err := d.client.FindLunGroup(PrefixLunGroup + hostId)
if err != nil {
if err != nil && !IsNotFoundError(err) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to understand, why more strict check for isNotFoundError? If there is any other error than IsNotFoundError, we can proceed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic is, only if this error is NOT FOUND, we can proceed, otherwise we return err as before.

return "", "", "", "", err
}

hostGrpId, err := d.client.FindHostGroup(PrefixHostGroup + hostId)
if err != nil {
if err != nil && !IsNotFoundError(err) {
return "", "", "", "", err
}

viewId, err := d.client.FindMappingView(PrefixMappingView + hostId)
if err != nil {
if err != nil && !IsNotFoundError(err) {
return "", "", "", "", err
}

Expand Down Expand Up @@ -799,7 +806,7 @@ func (d *Driver) clearHostRelatedResource(lunGrpId, viewId, hostId, hostGrpId st
return err
}
if !flag {
if err := d.client.RemoveHost(hostId); err != nil {
if err := d.client.DeleteHost(hostId); err != nil {
return err
}
}
Expand Down