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

[AN-440] Add symlink to /cromwell_root in GCP Batch #7722

Merged
merged 10 commits into from
Apr 8, 2025

Conversation

salonishah11
Copy link
Contributor

@salonishah11 salonishah11 commented Apr 4, 2025

Description

Jira ticket: https://broadworkbench.atlassian.net/browse/AN-440

In LifeSciences, the cromwell_root is located at /cromwell_root, but in the Batch backend it has moved to /mnt/disk/cromwell_root. WDLs that rely on the original path break when run on the Batch. To maintain backward and forward compatibility we create a symlink between /mnt/disk/cromwell_root and /cromwell_root.

Note: the creation of symlink does fail when run on images that don't allow root access, but in these cases the script.sh does continue to run remaining commands.

Release Notes Confirmation

CHANGELOG.md

  • I updated CHANGELOG.md in this PR
  • I assert that this change shouldn't be included in CHANGELOG.md because it doesn't impact community users

Terra Release Notes

  • I added a suggested release notes entry in this Jira ticket
  • I assert that this change doesn't need Jira release notes because it doesn't impact Terra users

@salonishah11 salonishah11 requested a review from a team as a code owner April 4, 2025 21:08
@salonishah11 salonishah11 changed the title Sps batch symlink [AN-440] Add symlink to /cromwell_root in GCP Batch Apr 4, 2025
Copy link
Collaborator

@aednichols aednichols left a comment

Choose a reason for hiding this comment

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

Nice find 👍

Clearly an improvement over the current situation, and a compact change that is easy to test.

Strong candidate for a changelog entry IMO.

CHANGELOG.md Outdated
@@ -9,6 +9,8 @@
* Add 30 GB default VM boot disk size to user-requested boot disk size; this ensures the VM has room for large user command Docker images.
* Fix a bug that caused Cromwell to treat immediate preemptions as failures.
* Automatically retry tasks that fail with transient Batch errors before the VM has started running (that is, before the task has cost the user money). These retries do not count against `maxRetries`.
* Symlink to `/cromwell_root` - In LifeSciences, cromwell_root is located at `/cromwell_root`, but in the Batch backend it has moved to `/mnt/disk/cromwell_root`. To ensure WDLs that rely on the original
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick - I would use "the Cromwell root directory that user scripts are run from" rather than "cromwell_root"

@salonishah11 salonishah11 merged commit d50673f into develop Apr 8, 2025
41 checks passed
@salonishah11 salonishah11 deleted the sps_batch_symlink branch April 8, 2025 22:03
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