Registry server: wrap mutating actions in transactions #700

timburks · 2022-08-25T22:04:32Z

I've been testing with remote databases and very high parallelism, logging and handling errors that arose from that, and saw some issues related to our lack of transaction support.

This PR wraps all mutating operations in action handlers in transactions (stage 1 of #706). The transaction wrapper also translates errors into gRPC errors, which with other proposed changes would allow us remove the majority of grpcErrorForDBError calls in the storage package.

Since delete actions are wrapped in transactions, transactions are removed from the deletion support code in the storage package (stage 2 of #706).

Stages 3-5 of #706 are left for future PRs.

codecov · 2022-08-25T22:09:47Z

Codecov Report

Merging #700 (a1ece2a) into main (e9c6f8c) will increase coverage by 1.02%.
The diff coverage is 70.89%.

❗ Current head a1ece2a differs from pull request most recent head aa02e3e. Consider uploading reports for the commit aa02e3e to get more accurate results

@@            Coverage Diff             @@
##             main     #700      +/-   ##
==========================================
+ Coverage   55.68%   56.70%   +1.02%     
==========================================
  Files          93       93              
  Lines        7959     8008      +49     
==========================================
+ Hits         4432     4541     +109     
+ Misses       3063     3028      -35     
+ Partials      464      439      -25

Impacted Files	Coverage Δ
server/registry/actions_deployment_revisions.go	`43.06% <50.00%> (+6.48%)`	⬆️
server/registry/server.go	`45.58% <50.00%> (+0.42%)`	⬆️
server/registry/actions_spec_revisions.go	`46.20% <52.50%> (+8.03%)`	⬆️
server/registry/actions_apis.go	`73.10% <78.84%> (+9.01%)`	⬆️
server/registry/actions_versions.go	`73.10% <78.84%> (+9.01%)`	⬆️
server/registry/actions_artifacts.go	`81.60% <80.00%> (+5.88%)`	⬆️
server/registry/actions_deployments.go	`72.78% <80.00%> (+4.94%)`	⬆️
server/registry/actions_specs.go	`75.89% <81.35%> (+3.76%)`	⬆️
server/registry/actions_projects.go	`80.48% <84.00%> (+7.27%)`	⬆️
... and 7 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

This ensures that associated blobs are also stored correctly.

…actions.

…nd project actions.

… up after the mutating transaction completes.

timburks · 2022-08-30T23:23:02Z

log/interceptor/interceptor.go

 			logger.Info("Success.")
-		case codes.Internal:
-			logger.Error("Internal error.")
+		case codes.Canceled:


This adds support for all gRPC status codes so that logging won't report "User error" for the unrecognized ones. It's in this commit because I used it for testing but I'll make a separate PR for just this change (to interceptor.go) to hopefully commit before this one.

timburks · 2022-08-30T23:25:16Z

server/registry/internal/storage/errors.go

 	if _, ok := status.FromError(err); ok {
 		return err
 	}
+	if err == context.DeadlineExceeded {


This file adds support for new errors that were encountered in load-testing of the server to verify the changes in this PR. I think they are off-topic enough to put in a separate PR that just modifies errors.go and would be merged prior to this one.

timburks · 2022-08-30T23:37:12Z

@seaneganx if we merge this before you're back, PTAL anyway, comment on anything, and we'll address it in issues/followup PRs.

theganyo · 2022-08-31T16:13:18Z

server/registry/actions_artifacts.go

+	var response *rpc.Artifact
+	if err := s.runWithTransaction(ctx, func(ctx context.Context, db *storage.Client) error {
+		// Creation should only succeed when the parent exists.
+		switch parent := parent.(type) {


nit: Probably subjective, but might this be easier to read like this?

var err error switch parent := parent.(type) { case names.Project: _, err = db.GetProject(ctx, parent) case names.Api: _, err = db.GetApi(ctx, parent) case names.Version: _, err = db.GetVersion(ctx, parent) case names.Spec: _, err = db.GetSpec(ctx, parent) case names.Deployment: _, err = db.GetDeployment(ctx, parent) } if err != nil { return err }

Agreed, thanks.

theganyo · 2022-08-31T16:22:27Z

server/registry/server.go

 	return s.storageClient, nil
 }

+func (s *RegistryServer) runWithTransaction(ctx context.Context, fn func(ctx context.Context, db *storage.Client) error) error {


nit: Personally, I would prefer runInTransaction() or asTransaction().

Thanks, I'll change this to "runInTransaction"

theganyo · 2022-08-31T19:50:03Z

That last commit (a1ece2a) adds code without supporting tests. Is it reasonable to test?

timburks · 2022-08-31T20:14:55Z

@theganyo let's discuss that -- I was just looking into retry configuration before adding a comment about that change. We can trigger the concurrency problem by running the bulk uploaders, e.g. registry upload bulk discovery, but I think this is too big to put in our CI flow. It might be better to directly construct a test by forcing a set of concurrent updates -- maybe starting several goroutines that all update the same resource at once? I think that's out of scope for this PR and would rather 1) leave this change in and file an issue for testing or 2) pull the last commit and address it in a different PR. [EDIT: I'll do 2) and start a new PR]

The change in that commit is observable here which explicitly ignores the (weird) AlreadyExists errors that concurrent updates can trigger. With the change, those errors are reported as Aborted and are retried by the client libraries (because we configured this).

theganyo · 2022-08-31T20:23:52Z

I've been worried that we don't have a way of reliably triggering the concurrency / transactional conditions that we've been running into - and thus we'd be unable to verify that we've eliminated the issues. That said, if we can reliably trigger our concurrency issues through the bulk uploader and we don't have the means to more specifically test at this point, perhaps that's good enough for now.

timburks · 2022-08-31T20:29:04Z

server/registry/actions_specs.go

-		// with improvements closer to the database level.
-		updateSpecMutex.Lock()
-		defer updateSpecMutex.Unlock()
-		log.Debugf(ctx, "Acquired lock after blocking for %v", time.Since(before))


Removing this here, but will address the underlying problem in #715

timburks added 3 commits August 25, 2022 11:36

hyper-detailed error reporting and an update wrapped in a transaction.

e11b434

wrap api and version updates in transactions

30c402b

Merge branch 'main' of github.com:apigee/registry into tx

052930a

timburks changed the title ~~Wrap database updates in transactions~~ Registry server: wrap update actions in transactions Aug 25, 2022

timburks marked this pull request as draft August 25, 2022 22:05

timburks added 5 commits August 25, 2022 15:22

wrap artifact creation and replacement in transactions

dffb67b

This ensures that associated blobs are also stored correctly.

Add transactions to spec and deployment actions.

f825f4e

Merge branch 'main' of github.com:apigee/registry into tx

e5d4254

Fix golangci-lint errors

6325b71

Update mutating API actions to run in transactions.

aa94162

timburks force-pushed the tx branch from e138114 to aa94162 Compare August 30, 2022 18:35

timburks added 10 commits August 30, 2022 12:24

Update mutating project actions to use transactions aligned with api …

98df2e8

…actions.

Update mutating version actions to use transactions, align with api a…

c84a293

…nd project actions.

Update mutating deployment actions to use transactions.

7317585

Update mutating spec actions to use transactions.

d65fdfa

Update mutating artifact actions to use transactions.

114e716

Update mutating deployment revision actions to use transactions.

bf44d3a

Update mutating spec revision actions to use transactions.

6136d35

correctly get newest remaining spec/deployment revision by looking it…

9d30969

… up after the mutating transaction completes.

Remove labels from grpcErrorForDBError

d8496c0

remove extra logging statement from grpcErrorForDBError

9a6cd38

timburks force-pushed the tx branch from 8f29269 to 9a6cd38 Compare August 30, 2022 22:36

Update logs and error reporting to handle more situations.

6afc01c

timburks commented Aug 30, 2022

View reviewed changes

Merge branch 'main' of github.com:apigee/registry into tx

c37ff9f

timburks changed the title ~~Registry server: wrap update actions in transactions~~ Registry server: wrap mutating actions in transactions Aug 30, 2022

timburks marked this pull request as ready for review August 30, 2022 23:35

timburks requested a review from theganyo August 30, 2022 23:35

timburks requested a review from seaneganx August 30, 2022 23:35

Add context to db sessions used for deletion.

8f5fa15

theganyo approved these changes Aug 31, 2022

View reviewed changes

timburks added 2 commits August 31, 2022 10:11

Rename runWithTransaction to runInTransaction

3870106

Restructure switch statement as suggested.

ce5ffe6

This was referenced Aug 31, 2022

Update logging interceptor to print all gRPC status codes. #713

Merged

Add handlers for more possible errors that can arise during load-testing #714

Merged

Merge branch 'main' of github.com:apigee/registry into tx

aa02e3e

timburks force-pushed the tx branch from 27d4979 to a1ece2a Compare August 31, 2022 19:05

timburks force-pushed the tx branch from a1ece2a to aa02e3e Compare August 31, 2022 20:20

timburks mentioned this pull request Aug 31, 2022

Registry server: report AlreadyExists errors in updates as Abort to cause them to be retried #715

Merged

timburks commented Aug 31, 2022

View reviewed changes

timburks merged commit 0a574b1 into apigee:main Aug 31, 2022

timburks mentioned this pull request Sep 1, 2022

Registry server: wrap get/modify/save update implementations in transactions. #623

Closed

timburks deleted the tx branch October 19, 2022 16:32

Registry server: wrap mutating actions in transactions #700

Registry server: wrap mutating actions in transactions #700

Uh oh!

Conversation

timburks commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

timburks Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timburks Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timburks commented Aug 30, 2022

Uh oh!

theganyo Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

timburks Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

timburks Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

theganyo Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

timburks Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

timburks Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

theganyo commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timburks commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theganyo commented Aug 31, 2022

Uh oh!

timburks Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

timburks commented Aug 25, 2022 •

edited

Loading

codecov bot commented Aug 25, 2022 •

edited

Loading

timburks Aug 30, 2022 •

edited

Loading

timburks Aug 30, 2022 •

edited

Loading

theganyo commented Aug 31, 2022 •

edited

Loading

timburks commented Aug 31, 2022 •

edited

Loading