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.

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

Conversation

sushanthakumar
Copy link
Contributor

@sushanthakumar sushanthakumar commented Jan 21, 2020

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:

Any other PR(s) this PR is dependant on: #1194 (Contains some base function for concurrency handling)

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:

@sushanthakumar sushanthakumar changed the base branch from master to development January 21, 2020 11:11
@@ -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.

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.

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

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

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.

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

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.

@kumarashit
Copy link
Collaborator

@sushanthakumar Please resolve the conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants