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 load lock file to prevent accidental re-loading of data to BQ #7138

Merged
merged 7 commits into from
Mar 16, 2021

Conversation

mmorgantaylor
Copy link
Member

@mmorgantaylor mmorgantaylor commented Mar 9, 2021

spec ops issue #248

process implemented here:

  • new task SetLoadLock is called at the beginning of ImportGenomes - it generates a UUID for the submission, writes that run_uuid to a lock file, and uploads that lock file to the output_directory (where the tsvs will be generated).
  • CreateImportTsvs and LoadTables take the run_uuid as an input, compare it against the contents of the lock file in the bucket, and only proceed if the uuids match. otherwise they exit out.
  • after all LoadTables tasks have completed, a new task ReleaseLoadLock is called that removes the lock file from the bucket (again only if the uuid in the lockfile matches this run)

tested and confirmed that:

@@ -338,6 +350,9 @@ task LoadTable {
echo "no ${FILES} files to process in $DIR"
fi

# remove load lock
echo "Removing load lock"
gsutil rm "${DIR}${LOCKFILE}" || exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

just wondering how we deal with this file still being there if the workflow crashes? Even if it's just a "here's why I'm not worried"

Copy link
Member Author

@mmorgantaylor mmorgantaylor Mar 9, 2021

Choose a reason for hiding this comment

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

I think it's a manual process - you have to go look and figure out what you want to do, based on what had happened when it crashed (e.g. you'd do something different if the bq load went through versus if it didn't). if you do want to re-run, you'd manually delete the lockfile. but definitely open to other ideas for processes? we talked a little bit in standup about how we could try to get fancy and infer the state of things, but landed that for now it could be a manual process of just preventing the bad thing (double loading) and requiring some manual investigation & remedying.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just paraphrase some of that and leave it as a comment for our future selves

.dockstore.yml Outdated
@@ -57,6 +57,7 @@ workflows:
branches:
- master
- ah_var_store
- mmt_lock_load
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want this committed?

Copy link
Member Author

Choose a reason for hiding this comment

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

not once i merge, but i'll remove it right before merging, so that it stays in dockstore in case i have to keep testing

@@ -209,6 +326,16 @@ task CreateImportTsvs {
command <<<
set -e

# check for existence of the correct lockfile
LOCKFILE="~{output_directory}/loadlock"
Copy link
Member Author

Choose a reason for hiding this comment

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

i might change the filename loadlock to LOCKFILE just for clarity. as attached as i am to the lock&load idea, i have already confused myself because loadlock is kind of a weird name

Copy link
Contributor

@ahaessly ahaessly left a comment

Choose a reason for hiding this comment

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

just one suggestion

scripts/variantstore/wdl/ImportGenomes.wdl Outdated Show resolved Hide resolved
@mmorgantaylor mmorgantaylor merged commit 869a173 into ah_var_store Mar 16, 2021
@mmorgantaylor mmorgantaylor deleted the mmt_lock_load branch March 16, 2021 01:55
mmorgantaylor added a commit that referenced this pull request Apr 6, 2021
)

* set lock at start of ImportGenomes run and check uuid at crucial steps
* use google docker for lockfile tasks, rename to LOCKFILE, add instruction for removing lockfile
* auth as SA if needed for lock and release tasks
This was referenced Mar 17, 2023
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.

3 participants