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

Fix/ub 1671 idempotent issues in delete #266

Merged
merged 13 commits into from
Nov 11, 2018

Conversation

olgashtivelman
Copy link
Collaborator

@olgashtivelman olgashtivelman commented Nov 8, 2018

in this PR the idempotent issues in the delete request are fixed.
the issues are as follows: (in delete volume function)

  • in get Back-end in the storage_api_handler delete function - if volume does not exist then return nil.
  • in get volume: if volume does not exist then return success
  • in vol delete from XIV : if an 404 error is returned from SC then continue with the flow. (to delete from ubiquity DB)
  • in delete volume from DB (for the case where we have a non-db volume) if a volume not found error is returned then return success.
    in all the above issues an idempotent there is a warning message in the logger.

comment : an assumption is made that if a volume is removed from the DB then it is definitely removed from the storage since the removal from the DB is the latter operation.

Note: addition PR to handle delete idempotent in the flex side -> #266


This change is Reviewable

@coveralls
Copy link

coveralls commented Nov 8, 2018

Coverage Status

Coverage increased (+0.2%) to 57.988% when pulling acb92fa on fix/ub-1671_idempotent_delete into ae6a0a5 on dev.

Copy link
Contributor

@shay-berman shay-berman left a comment

Choose a reason for hiding this comment

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

:lgtm:

NOTE: there is one comment to fix, please check it. And if it passed staging then feel free to merge to dev.

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @olgashtivelman)


local/scbe/datamodel_wrapper.go, line 103 at r1 (raw file):

		// sanity
		if d.dbVolume == nil {
			return d.logger.ErrorRet(&resources.VolumeNotFoundError{VolName: name}, "failed")

I think its better to add here a logging for idempotent, like we do in many other cases of idempotent.
Of course not to raise error just message to the log that this is the case.

Copy link
Collaborator Author

@olgashtivelman olgashtivelman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @shay-berman)


local/scbe/datamodel_wrapper.go, line 103 at r1 (raw file):

Previously, shay-berman wrote…

I think its better to add here a logging for idempotent, like we do in many other cases of idempotent.
Of course not to raise error just message to the log that this is the case.

Done.

@olgashtivelman olgashtivelman merged commit 3c95a5d into dev Nov 11, 2018
@olgashtivelman olgashtivelman deleted the fix/ub-1671_idempotent_delete branch November 11, 2018 10:53
deeghuge pushed a commit that referenced this pull request Nov 12, 2018
in this PR the idempotent issues in the delete request are fixed.
the issues are as follows: (in delete volume function)

* in get Back-end in the storage_api_handler delete function - if volume does not exist then return nil.
* in get volume: if volume does not exist then return success
* in vol delete from XIV : if an 404 error is returned from SC then continue with the flow. (to delete from ubiquity DB)
* in delete volume from DB (for the case where we have a non-db volume) if a volume not found error is returned then return success.

in all the above issues an idempotent there is a warning message in the logger.
comment : an assumption is made that if a volume is removed from the DB then it is definitely removed from the storage since the removal from the DB is the latter operation.

Note: addition PR to handle delete idempotent in the flex side -> #266
@shay-berman shay-berman added v2.0.0 idempotency Improve Flex API idempotency aspects labels Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idempotency Improve Flex API idempotency aspects v2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants