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

Acquire a per-lock lease to make renew and revoke atomic wrt each other. #11122

Merged
merged 13 commits into from
Jun 10, 2021
Merged

Acquire a per-lock lease to make renew and revoke atomic wrt each other. #11122

merged 13 commits into from
Jun 10, 2021

Conversation

mgritter
Copy link
Contributor

This means we don't have to hold pendingLock during I/O.

Acquire a per-lock lease to make renew and revoke atomic wrt each other.
8d54217 
This means we don't have to hold pendingLock during I/O.
@mgritter mgritter marked this pull request as draft March 17, 2021 01:23
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 17, 2021 19:40 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 17, 2021 19:40 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 17, 2021 19:51 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 17, 2021 19:51 Inactive
@mgritter mgritter marked this pull request as ready for review March 18, 2021 17:07
swayne275
swayne275 approved these changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@swayne275 swayne275 left a comment

Choose a reason for hiding this comment

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

Approving, but did you ever get to the bottom of the potential deadlock you were seeing in tests?

@mgritter
Copy link
Contributor Author

Approving, but did you ever get to the bottom of the potential deadlock you were seeing in tests?

Yes, 1704a8d fixes it.
The problem is that when a token is revoked, revokeCommon is called on its lease twice. There's already a flag to prevent an infinite recursion, so that seems like the right thing to do to avoid the deadlock too. But it feels a little hacky, or like the double-revoke thing means that there's a design problem that could be refactored away.

@mladlow
Copy link
Collaborator

mladlow commented May 6, 2021

@sgmiller @swayne275 could one of you resolve the conflicts and merge this PR, since it seems like something we want, and Mark can't merge it?

@vercel vercel bot temporarily deployed to Preview – vault June 7, 2021 22:20 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 7, 2021 22:20 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 8, 2021 01:34 Inactive
@vercel vercel bot temporarily deployed to Preview – vault June 8, 2021 01:34 Inactive
swayne275
swayne275 reviewed Jun 8, 2021
View reviewed changes
@vercel vercel bot temporarily deployed to Preview – vault June 8, 2021 16:56 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 8, 2021 16:56 Inactive
swayne275
swayne275 reviewed Jun 8, 2021
View reviewed changes
Copy link
Contributor

@swayne275 swayne275 left a comment

Choose a reason for hiding this comment

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

looks good, just a few questions

@briankassouf @swayne275
Update vault/expiration.go
2105dba 
Co-authored-by: swayne275 <swayne275@gmail.com>
@vercel vercel bot temporarily deployed to Preview – vault June 8, 2021 17:23 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 8, 2021 17:23 Inactive
@briankassouf @swayne275
Update vault/expiration.go
3fbe11a 
Co-authored-by: swayne275 <swayne275@gmail.com>
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 8, 2021 17:25 Inactive
@vercel vercel bot temporarily deployed to Preview – vault June 8, 2021 17:25 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 8, 2021 17:35 Inactive
@vercel vercel bot temporarily deployed to Preview – vault June 8, 2021 17:35 Inactive
@briankassouf briankassouf merged commit 7a9c948 into master Jun 10, 2021
@briankassouf briankassouf deleted the expiration_per_lease_locking branch June 10, 2021 00:28
@briankassouf briankassouf added this to the 1.8 milestone Jun 10, 2021
briankassouf added a commit that referenced this pull request Jun 10, 2021
@sgmiller @briankassouf @swayne275
Acquire a per-lock lease to make renew and revoke atomic wrt each oth…
5323b68 
…er. (#11122)

* Acquire a per-lock lease to make renew and revoke atomic wrt each other.
This means we don't have to hold pendingLock during I/O.

* Attempted fix for deadlock in token revocation.

* Comment fix.

* Fix error checking in loadEntry.

* Add benchmark

* Add a few additional locking locations

* Improve benchmark slightly

* Update vault/expiration.go

Co-authored-by: swayne275 <swayne275@gmail.com>

* Update vault/expiration.go

Co-authored-by: swayne275 <swayne275@gmail.com>

* Add a lease lock into tidy

Co-authored-by: Scott Miller <smiller@hashicorp.com>
Co-authored-by: Brian Kassouf <bkassouf@hashicorp.com>
Co-authored-by: Brian Kassouf <briankassouf@users.noreply.github.com>
Co-authored-by: swayne275 <swayne275@gmail.com>
@briankassouf briankassouf mentioned this pull request Jun 10, 2021
Merged
briankassouf added a commit that referenced this pull request Jun 10, 2021
@briankassouf @sgmiller @swayne275
Acquire a per-lock lease to make renew and revoke atomic wrt each oth…
0a60b23 
…er. (#11122) (#11808)

* Acquire a per-lock lease to make renew and revoke atomic wrt each other.
This means we don't have to hold pendingLock during I/O.

* Attempted fix for deadlock in token revocation.

* Comment fix.

* Fix error checking in loadEntry.

* Add benchmark

* Add a few additional locking locations

* Improve benchmark slightly

* Update vault/expiration.go

Co-authored-by: swayne275 <swayne275@gmail.com>

* Update vault/expiration.go

Co-authored-by: swayne275 <swayne275@gmail.com>

* Add a lease lock into tidy

Co-authored-by: Scott Miller <smiller@hashicorp.com>
Co-authored-by: Brian Kassouf <bkassouf@hashicorp.com>
Co-authored-by: Brian Kassouf <briankassouf@users.noreply.github.com>
Co-authored-by: swayne275 <swayne275@gmail.com>

Co-authored-by: Mark Gritter <mgritter@hashicorp.com>
Co-authored-by: Scott Miller <smiller@hashicorp.com>
Co-authored-by: swayne275 <swayne275@gmail.com>
ncabatoff added a commit that referenced this pull request Oct 18, 2021
@ncabatoff
Add missing CL entry for #11122.
c5e86a9
ncabatoff added a commit that referenced this pull request Oct 18, 2021
@ncabatoff
Add missing CL entry for #11122. (#12854)
d3b7b97
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swayne275 swayne275 swayne275 approved these changes

@briankassouf briankassouf Awaiting requested review from briankassouf

@sgmiller sgmiller Awaiting requested review from sgmiller

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.8
Development

Successfully merging this pull request may close these issues.
5 participants
@mgritter @mladlow @sgmiller @briankassouf @swayne275