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

Fix concurrent workflow create bug by unique index #346

Merged
merged 2 commits into from
Oct 23, 2020

Conversation

ryli17
Copy link
Contributor

@ryli17 ryli17 commented Oct 21, 2020

Description

When multiple process concurrently create workflow, the code to access workflow_worker_map is not concurrent safe. This PR fix this issue.

Why is this needed

During workflow creation, new rows may be add to table workflow_worker_map in the same transaction. The original design use INSERT ... SELECT. This doesn't work concurrently with SERRIELIZABLE isolation.

Fixes: #

  • Create unique index on workflow_worker_map table
  • Use INSERT ... ON CONFLICT DO NOTHING; to avoid table lock.

How Has This Been Tested?

Manually as #342.

How are existing users impacted? What migration steps/scripts do we need?

  • No impact
  • database migrate to 202010221010-add-unique-index.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #346 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #346   +/-   ##
=======================================
  Coverage   23.79%   23.79%           
=======================================
  Files          14       14           
  Lines        1244     1244           
=======================================
  Hits          296      296           
  Misses        928      928           
  Partials       20       20           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12fba51...08062ac. Read the comment docs.

@kqdeng kqdeng self-requested a review October 21, 2020 21:47
Copy link
Contributor

@kqdeng kqdeng left a comment

Choose a reason for hiding this comment

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

Can you squash the commits?

db/migration/202009171251-init-database.go Outdated Show resolved Hide resolved
db/migration/202009171251-init-database.go Outdated Show resolved Hide resolved
db/workflow.go Show resolved Hide resolved
@ryli17 ryli17 force-pushed the master branch 2 times, most recently from 6a401d4 to 8274ad6 Compare October 23, 2020 04:57
@ryli17
Copy link
Contributor Author

ryli17 commented Oct 23, 2020

squashed the commits.

@ryli17 ryli17 requested review from kqdeng and mmlb October 23, 2020 05:32
gianarb
gianarb previously approved these changes Oct 23, 2020
db/workflow.go Show resolved Hide resolved
@ryli17 ryli17 force-pushed the master branch 2 times, most recently from 639fb5e to 1e9fc80 Compare October 23, 2020 20:08
@ryli17 ryli17 changed the title Fix concurrent workflow create bug Fix concurrent workflow create bug by unique index Oct 23, 2020
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Oct 23, 2020
@mergify mergify bot merged commit 3b230d5 into tinkerbell:master Oct 23, 2020
mergify bot added a commit that referenced this pull request Oct 26, 2020
## Description

After PR #346 merged, raise back isolation level from repeatable read to serializable.
Also add create unique index ddl to tinkerbell-init.sql.

## How Has This Been Tested?

- Manually

## How are existing users impacted? What migration steps/scripts do we need?

- No

## Checklist:

I have:

- [ ] updated the documentation and/or roadmap (if required)
- [ ] added unit or e2e tests
- [ ] provided instructions on how to upgrade
@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
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