[Driver] Enhance the volume detachment process from driver #1193
Conversation
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No, ErrorObjectUnavailable just indicates the LUN is not associated to the specified LUNGROUP.
For other failed circumstances, return error as before.
| if err := c.request("PUT", url, data, nil); err != nil { | ||
| if c.checkErrorCode(err, ErrorLunGroupNotInMappingView) { |
There was a problem hiding this comment.
I'd like to start another pr to add all these logs.
| if err := c.request("PUT", url, data, nil); err != nil { | ||
| if c.checkErrorCode(err, ErrorHostGroupNotInMappingView) { |
There was a problem hiding this comment.
Same as above for logging
| 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) { |
| 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) { |
There was a problem hiding this comment.
Will it be only if the HostGroup doesn't exist? Can there be some other system issues?
There was a problem hiding this comment.
only at the situation of hostgroup already not exist, we ignore this error and return success.
| return nil | ||
| } | ||
|
|
||
| return err |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, we return err as before to keep consistent for other errors.
| return nil | ||
| } | ||
|
|
||
| return err |
There was a problem hiding this comment.
Similar to what commented at 1085 line
| return "", "", "", "", err | ||
| } | ||
|
|
||
| lunGrpId, err := d.client.FindLunGroup(PrefixLunGroup + hostId) | ||
| if err != nil { | ||
| if err != nil && !IsNotFoundError(err) { |
There was a problem hiding this comment.
Trying to understand, why more strict check for isNotFoundError? If there is any other error than IsNotFoundError, we can proceed?
There was a problem hiding this comment.
this logic is, only if this error is NOT FOUND, we can proceed, otherwise we return err as before.
|
@sushanthakumar Please resolve the conflicts |
What this PR does / why we need it:
This PR addresses some enhancement on the volume detachment process from driver especially during some errors are encountered while interacting with back ends
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #This PR fixes detach part of the issue : #1197
Special notes for your reviewer:
Test steps:
Create multiple attachments through multiple pod creation simultaneously
Simulate back end error while performing volume detach such as from oceanstore
Verify the volume and attachments states
Release note: