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

auth: keep old revision in 'NewAuthStore' #7364

Merged
merged 2 commits into from
Feb 22, 2017
Merged

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Feb 21, 2017

When there's no changes yet (right after auth
store initialization), we should commit old revision.

Fix #7359.

Seems like commitRevision was introduced long time ago,
and we recently fixed it to have correct revision in authStore,
so it starts failing in our hash tests.

Do we have any reason to bump up revision for every commitRevision call,
even around store initialization?

auth/store.go Outdated
@@ -916,7 +916,7 @@ func NewAuthStore(be backend.Backend, indexWaiter func(uint64) <-chan struct{})
as.simpleTokenKeeper = NewSimpleTokenTTLKeeper(newDeleterFunc(as))
}

as.commitRevision(tx)
as.commit(tx)
Copy link
Contributor

@heyitsanthony heyitsanthony Feb 21, 2017

Choose a reason for hiding this comment

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

I think the initial version needs to commit with rev=1. Maybe:

if as.revision == 0 {
    as.commitRevision()
}

@@ -38,6 +38,33 @@ func dummyIndexWaiter(index uint64) <-chan struct{} {
return ch
}

// TestNewAuthStoreRevision ensures newly auth store
Copy link
Contributor

Choose a reason for hiding this comment

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

there probably should be an integration test that restarts the etcd server and checks the hash stays the same / fails without this patch

When there's no changes yet (right after auth
store initialization), we should commit old revision.

Fix etcd-io#7359.
@gyuho
Copy link
Contributor Author

gyuho commented Feb 22, 2017

@heyitsanthony PTAL. Thanks.

@heyitsanthony
Copy link
Contributor

lgtm defer to @mitake

Copy link
Contributor

@mitake mitake left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@mitake
Copy link
Contributor

mitake commented Feb 22, 2017

Do we have any reason to bump up revision for every commitRevision call, even around store initialization?

I added the commitRevision() call because the initialization phase can create buckets for the metadata. But it wouldn't be necessarily. Removing it won't produce problems.

@mitake
Copy link
Contributor

mitake commented Feb 22, 2017

@gyuho BTW can I ignore the fail of jenkins-proxy-ci? It seems to be the internal problem of the server. Or could you restart the CI?

@gyuho
Copy link
Contributor Author

gyuho commented Feb 22, 2017

Yeah proxy CI doesn't seem related to this fix.

Thanks.

@gyuho gyuho merged commit c0c4c7c into etcd-io:master Feb 22, 2017
@gyuho gyuho deleted the auth-revi branch February 22, 2017 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

restarted etcd returns different hash
3 participants