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

Jump to bottom

Swift locking support #18431

Closed
wants to merge 5 commits into from
Closed

Swift locking support #18431

wants to merge 5 commits into from

Conversation

yanndegat
Copy link
Contributor

  • remote/backend/swift: Add support for locking

@yanndegat
Copy link
Contributor Author

yanndegat commented Jul 11, 2018 •
edited
Loading

hi @jtopjian, @fatmcgav

i added locking support
inspired by other backend impls

@yanndegat yanndegat force-pushed the swift_locking_support branch 2 times, most recently from 1f0d35e to 25100a2 Compare July 11, 2018 14:17
catsby
catsby reviewed Jul 13, 2018
View reviewed changes
Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

Depends on #18378

fatmcgav
fatmcgav approved these changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@fatmcgav fatmcgav left a comment

Choose a reason for hiding this comment

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

One minor comment fix...

backend/remote-state/swift/backend_state.go Outdated
// is a normal create operation, and take the lock at that point.
//
// If we need to force-unlock, but for some reason the state no longer
// exists, the user will have to use aws tools to manually fix the
Copy link
Contributor

Choose a reason for hiding this comment

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

s/aws/openstack

jtopjian
jtopjian reviewed Jul 13, 2018
View reviewed changes
backend/remote-state/swift/client.go Outdated
// recent known by swift.
// swift is eventually consistent. Yet end users may expect to get a consistent
// response when querying for state or lock objects
type GetNewestOpts struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

@yanndegat

I've just merged support for Newest to Gophercloud: gophercloud/gophercloud#1118

You should be able to do a vendor update and then not have to create a custom type.

@jbardin
Copy link
Member

jbardin commented Jul 16, 2018

Hi @yanndegat,

This looks pretty good at first glance, but I would like to make sure it passes the acceptance tests first. This needs to call backend.TestBackendStateLocks and backend.TestBackendStateForceUnlock in the tests.

Since we don't have a way yet to run acceptance tests directly on backend PRs, we also require that the acceptance test output be pasted as a comment here to verify that they pass.

@yanndegat
Copy link
Contributor Author

hi @jbardin

thanks a lot.
no problem to add those tests.
Actually, how do they relate to what i've added: here
25100a2#diff-7e56003b6a8dd77c875fff599ade31a3R60

& right after with the testforceunlock test func ?

i added them by getting inspiration from other tests. if i add the ones you suggest, can i remove the ones i added because it seems to me that they overlaps ?

@jbardin
Copy link
Member

jbardin commented Jul 16, 2018

It looks like you copied a lot from the S3 backend, and IIRC that test was just to catch as specific failure for that implementation. I would call TestBackendStateForceUnlock instead to match the coverage of the other backends.

The TestBackendStateLocks function should now cover everything that remote.TestRemoteLocks does. The old state/remote package will probably be removed in the near future.

@yanndegat yanndegat force-pushed the swift_locking_support branch 4 times, most recently from 5428051 to be8f514 Compare July 17, 2018 14:10
@yanndegat
Copy link
Contributor Author

@jtopjian done

@yanndegat
Copy link
Contributor Author

@jbardin done

@jtopjian
Copy link
Contributor

I opened #18671 last week which I think should be merged before this. It does a bunch of vendor updates and gets the Swift backend aligned with the OpenStack provider as far as auth settings and such go. It would be a good foundation to apply additional features on top of.

@yanndegat yanndegat mentioned this pull request Feb 3, 2019
Merged
@yanndegat
Copy link
Contributor Author

closing in favor of #20211

@yanndegat yanndegat closed this Feb 3, 2019
@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@catsby catsby catsby left review comments

@jtopjian jtopjian jtopjian left review comments

@fatmcgav fatmcgav fatmcgav approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yanndegat @jbardin @jtopjian @catsby @fatmcgav @apparentlymart