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

Add node/service/check operations to transaction api #4869

Merged
merged 11 commits into from
Jan 22, 2019
Merged

Conversation

kyhavlov
Copy link
Contributor

This PR adds operations on health checks to the Transaction API. There's still more to do here around adding tests and validation of the input to the RPC endpoint, but I'm putting it up early so others can sanity check the approach.

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Overall I think this looks good. The minor refactors seem clean. The only slight concern in big picture is that it might seem a bit odd that transaction API supports KV and Check but nothing else. That said, this is the right way to do this and I don't want to expand scope any further for fixing the issue at hand so I think this is fine. Maybe when you come to update the docs we could point out that it's generic but that only KV and Check updates have been added so far and other resources will be added as the need to access them transactionally arises or something?

agent/consul/state/catalog.go Outdated Show resolved Hide resolved
@kyhavlov
Copy link
Contributor Author

@banks I was planning to add node operations as well - ESM can use that as it does a node deregister when reaping unhealthy nodes and it'd be useful to be able to group that into the transaction for clearing the node health entry in the KV store. After that I wanted to add service ops too so that this would add support for all the catalog operations. Service/node operations should be the same as the check ones (get, set, cas, delete, delete-cas) so I don't think there's anything unexpected there.

@pearkes pearkes added this to the 1.4.1 milestone Oct 30, 2018
@kyhavlov kyhavlov changed the title [WIP] Add check operations to transaction api [WIP] Add node/service/check operations to transaction api Dec 3, 2018
@kyhavlov kyhavlov changed the title [WIP] Add node/service/check operations to transaction api Add node/service/check operations to transaction api Dec 12, 2018
@kyhavlov kyhavlov requested a review from a team December 12, 2018 18:05
@kyhavlov
Copy link
Contributor Author

This is ready for review now - the only thing left is the docs update for the new supported operations of the txn endpoint.

There's one change in the client api package that's breaking as-is which is changing the type of a few fields in the health check catalog definition struct from ReadableDuration to time.Duration and adding methods to implement json marshaling and unmarshaling. This isn't a change to the api itself, just the way the go client was serializing these fields. The right call is probably to leave the fields as they are and add some new ones and custom logic to use those instead if they're provided, but I was wondering what others would think.

Other than that, this turned out fairly straightforward and required tests in a few different places to check that the new node/service/check ops matched the pattern of the KV ops while following the same ACL restrictions as registering through the catalog normally.

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Looking great. I found a handful of really minor things.

Also when updating API docs we should add docs for intentions in the txn as well as they seem to be missing from consul.io

agent/consul/acl.go Outdated Show resolved Hide resolved
agent/consul/acl.go Outdated Show resolved Hide resolved
agent/consul/acl.go Outdated Show resolved Hide resolved
agent/consul/catalog_endpoint.go Show resolved Hide resolved
agent/consul/catalog_endpoint.go Show resolved Hide resolved
agent/consul/state/catalog.go Outdated Show resolved Hide resolved
agent/consul/state/catalog.go Outdated Show resolved Hide resolved
agent/consul/state/catalog.go Outdated Show resolved Hide resolved
agent/consul/state/catalog.go Outdated Show resolved Hide resolved
agent/consul/state/catalog.go Outdated Show resolved Hide resolved
@kyhavlov kyhavlov requested a review from a team as a code owner January 16, 2019 00:54
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Docs looks good to me. Presumably we don't document intentions with Txn because its not available from the HTTP API?

@kyhavlov
Copy link
Contributor Author

@mkeeler That's right, it was just added internally for (so far) intention replication in enterprise.

@kyhavlov kyhavlov merged commit 5bdf130 into master Jan 22, 2019
@kyhavlov kyhavlov deleted the txn-checks branch January 22, 2019 19:16
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.

4 participants