Skip to content

feat(runs): apply the run base dir and raw data path settings - #8026

Open
davidlin20dev wants to merge 3 commits into
flyteorg:mainfrom
davidlin20dev:feat/settings-storage-paths
Open

davidlin20dev wants to merge 3 commits into
flyteorg:mainfrom
davidlin20dev:feat/settings-storage-paths

Conversation

@davidlin20dev

Copy link
Copy Markdown
Contributor

Tracking issue

Related to #7775 and #7932. Covers task 3.4 in CreateRun: storage.raw_data_path in full, run.run_base_dir for run metadata. UploadInputs does not read settings yet, so offloaded inputs still use the cluster default. That half is a separate PR.

Why are the changes needed?

run.run_base_dir and storage.raw_data_path can be stored and resolved, but nothing reads them, so a run's storage paths still come only from the request or the cluster-wide default. An admin cannot point a project's run data at their own bucket.

What changes were proposed in this pull request?

  • applyRunSettings fills RunSpec.run_base_dir and RunSpec.raw_data_storage.raw_data_prefix when the request leaves them unset. An explicit request value always wins, and a setting that is INHERIT or UNSET contributes nothing.
  • CreateRun needs no logic change. It already reads both fields and falls back to the configured prefix, and applyRunSettings runs before that, so the request, then settings, then config precedence holds without any extra code.
  • The paired TODOs are corrected. The one in CreateRun asked for exactly this and is narrowed to the remaining gap; the one in UploadInputs pointed at StorageSettings.run_base_dir, which is reserved and no longer exists, and claimed CreateRun still needed the work.

How was this patch tested?

  • Seven new rows in the applyRunSettings table, covering both fields: the setting is applied, an explicit request value wins, and INHERIT and UNSET contribute nothing. One row sends RawDataStorage present but with an empty prefix, which must still take the setting.
  • Existing runs/service and dataproxy/service tests pass unchanged.

Labels

  • added

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>
Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>
Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>
concurrency.GetState() == settings.SettingState_SETTING_STATE_VALUE {
spec.MaxActionConcurrency = uint32(concurrency.GetIntValue())
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we also validate the format of storage path here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants