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 transaction support to API #715

Merged
merged 6 commits into from
Feb 8, 2020
Merged

Add transaction support to API #715

merged 6 commits into from
Feb 8, 2020

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Feb 6, 2020

These commits add transaction support to the API. The goal is allowing our services to commit their change sets without interfering with each other or with any user changes.

The API surface shifts slightly:

  • /settings/pending becomes /tx to show a requested transaction
  • The methods under /settings (commit, apply, commit_and_apply) move under /tx
  • Several methods have a new ?tx= parameter to specify the transaction, defaulting to "default" (which allows users to not think about this)
  • /tx/list is added to list transaction names

How it works:

  • The data store adds a level of directories under pending, each of which is a named transaction, e.g. "default".
    • Those directory names are encoded the same way keys are, so we don't have to care overmuch about the name format.
  • The startup services (moondog, sundog, etc.) use a shared "thar-boot" transaction.

Other notes for reviewers:

  • It might help to review the commits in order rather than the whole set. It looks long, but really it's a few simple changes repeated.
  • sundog had a loop in get_populated_settings, where it checks what's already set so as not to overwrite it, that would look at pending and committed settings. We no longer need to check pending settings because we run settings-committer as a pre-exec command for sundog. It's not strictly related to this PR, but that whole block changed for other reasons.
  • Committed::Pending was updated to store the relevant transaction name. It's a String because the alternatives I tried were worse. This means Committed can no longer be Copy, which is why there are so many changes just adding a & to a Committed - we almost never need ownership anyway.
  • The pending data store directory structure changed to add the transaction level, but storewolf always removes the pending directory on boot, so we shouldn't have to worry about migrations from/to the prior layout.

Further work for separate PRs:

  • We need to update the control container to include the enable-admin-container improvement - @etungsten said he could hold his version bump to include this too.
  • netdog can now use the API system (templates, etc.) instead of reinventing the wheel.

Testing done:

Existing and updated unit tests pass.

An AMI was still happy, pods ran, etc.

Further manual testing: I played around with transactions against the new API server. I had parallel transactions, confirmed that one would apply correctly and then another. I confirmed I could list and delete them. (It's an awful lot of output to put in a PR, and the output is identical to before; the only new API is /tx/list which returns a simple JSON list like ["tx1","tx2"].)

README.md Show resolved Hide resolved
workspaces/api/openapi.yaml Show resolved Hide resolved
Copy link
Contributor

@etungsten etungsten left a comment

Choose a reason for hiding this comment

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

💸

workspaces/api/apiserver/src/datastore/filesystem.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Show resolved Hide resolved
workspaces/api/apiserver/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

📻

README.md Outdated Show resolved Hide resolved
workspaces/api/apiserver/README.md Outdated Show resolved Hide resolved
@tjkirch
Copy link
Contributor Author

tjkirch commented Feb 7, 2020

This push addresses concerns from @etungsten and @zmrow; thanks!

@tjkirch
Copy link
Contributor Author

tjkirch commented Feb 8, 2020

This change puts the control container change in its own commit, per @bcressey.

@tjkirch tjkirch merged commit 696b221 into develop Feb 8, 2020
@tjkirch tjkirch deleted the transactions branch February 8, 2020 00:37
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.

5 participants