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

[#148 Implement TES Task Runner] Implement SAS resolution strategy for DRS #150

Closed
Tracked by #148
giventocode opened this issue Mar 22, 2023 · 1 comment · Fixed by #655
Closed
Tracked by #148

[#148 Implement TES Task Runner] Implement SAS resolution strategy for DRS #150

giventocode opened this issue Mar 22, 2023 · 1 comment · Fixed by #655
Labels
bug Something isn't working TES Priority: P3 Groomed to a Priority 3 issue
Milestone

Comments

@giventocode
Copy link
Contributor

Overview
The TES Task Runner provides an extensibility mechanism to enable just-in-time SAS token resolution on the Batch Nodes. The goal of this story is to implement a new SAS resolution strategy when DRS URLs are provided.

AC:

  • Must implement the current DRS approach as SAS resolution strategy in the runner.
  • Should consider removing the dependency with DRS downloader and perform the resolution calls directly.
  • Must add options to the runner to enable DRS SAS resolution .
  • Must refactor the scheduler to use this strategy when applicable.

DoD:

  • Code merge into main, with unit tests that demonstrate the functionality.
  • Documented options for the runner and the API if applicable,
@giventocode giventocode added the linked issue Related issues label Mar 22, 2023
@giventocode giventocode added this to the 4.2 milestone Mar 23, 2023
@giventocode giventocode modified the milestones: 4.2, 4.4.0 May 9, 2023
@ngambani ngambani added contextualized and removed linked issue Related issues labels Jul 7, 2023
@ngambani ngambani changed the title Implement SAS resolution strategy for DRS [#148 Implement TES Task Runner] Implement SAS resolution strategy for DRS Oct 3, 2023
@BMurri BMurri modified the milestones: 4.4.0, next Nov 29, 2023
@BMurri BMurri added the bug Something isn't working label Nov 29, 2023
@BMurri
Copy link
Collaborator

BMurri commented Nov 29, 2023

Notes: Consider the following WRT the DRS Localizer (which is the current DRS approach):

  • Due to the reason for DRS Locator container image has changed the name of the env vars to access the DRS location service #391 & Fix DRS resolver env var name #390 some of the command-line options we are using are no longer valid (specifically --vault-name and --secret-name. Both should be removed, along with all the configuration that drives them. See Rename Martha~ to something like DrsResolver #392.
  • The DRS Localizer actually allows all of the DRS downloads to be passed via the command-line in just one call (rather than serially calling the localizer repeatedly for each call). That option is named --manifest-path and requires one argument, a CSV file with two columns (Url, Path) and one row per input file. The file should be without headers and should follow RFC-4180 formatting.
  • The DRS Localizer uses another option (--identity-client-id) for authorization (pass the ClientId of the azure managed identity) which we should be setting. This means that the container running the localizer needs to not be blocked.
  • The DRS Localizer uses the following environment variables we have not been using: DRS_RESOLVER_NUM_RETRIES, DRS_RESOLVER_WAIT_INITIAL_SECONDS, DRS_RESOLVER_WAIT_MAXIMUM_SECONDS, DRS_RESOLVER_WAIT_MULTIPLIER, DRS_RESOLVER_WAIT_RANDOMIZATION_FACTOR. We should consider whether we should provide any of these, or simply continue to use the defaults.

The following is provided because it is no longer showing up in the source code:

DRS files are identified via this line of code: task.Inputs.Where(f => f?.Url?.StartsWith("drs://", StringComparison.OrdinalIgnoreCase) == true)

The docker image is available in BatchScheduler as cromwellDrsLocalizerImageName, the resolver URL (found currently in Options.MarthaOptions.Url) is passed double-quoted via the DRS_RESOLVER_URL environment variable, the command-line option --access-token-strategy azure is always provided, and the container should run with the same volume mounts options as the executor. When not using the manifest file, the Url and Path should be passed as command-line arguments in that same order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working TES Priority: P3 Groomed to a Priority 3 issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants