fix(frontend): retrieve archived logs from correct location

[droctothorpe]

Description of your changes:
This PR solves #10036 by making it possible to inject the AWF keyFormat, which determines where archived workflow container logs are saved, into the UI via configs.ts / env vars so that the UI retrieves them from the correct location.

Currently, for v2, the UI is looking in the wrong object store path (which is hard-coded) and coming up empty. We could update the hard-coded path, but loading it from configs.ts makes it more future proof and flexible in case end users want to modify the keyFormat for any reason.

This is an interim solution that's still a huge improvement over the current state of affairs (logs are inaccessible in v2 once the AWF CR has been GCed), but it has two important drawbacks that we want to delineate:

1. It doesn't show logs for the dag driver or the task driver(s). This can be helpful if there are issues in the driver rather than the component code. The only way to debug this currently is to interact directly with the pods via the K8s API. This PR doesn't introduce this limitation. It just doesn't solve for it.
2. If the AWF keyFormat is updated in both the workflow-controller-configmap and the UI (via configs.ts / env vars), the logs of new runs will be accessible, but the logs of old runs will no longer be accessible. In defense of this PR as an interim solution, the logs of NO runs are currently accessible.

In spite of these drawbacks, we still think it may be worthwhile to merge this PR as an interim fix since keyFormat is rarely modified and this will solve 99% of use cases for a really significant regression (with, at present, 21 👍 emojis).

Looking forward to feedback. Thanks!

PS. We can add support for additional AWF template tags, as long as the actual parameters are available to the handler. We can also list supported tags via comments in the configmap.

PPS. I am not a frontend engineer.

Checklist:

• The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

[thesuperzapper]

@droctothorpe if we are going to do this interim fix can we please change the implementation slightly:

1. Use the go template package (which is what Argo uses) rather than native string replacement, so we:
   • allow for key formats that use non perfect spaces {{ workflow.name}}, which is still valid
   • fail to start KFP if an unsupported key format template is passed (e.g. using an unsupported variable/function)
2. Also support {{ workflow.namespace }} because this is a very common element of keyFormat to allow for namespace isolation by IAM scoped to object store prefixes

[droctothorpe]

1. Use the go template package (which is what Argo uses) rather than native string replacement, so we:

   • allow for key formats that use non perfect spaces {{ workflow.name}}, which is still valid

Apologies if I'm missing something obvious, but we can't use the Go templating package in typescript. Is there a well-maintained typescript package for handling go templates that you're aware of? Is it worth adding a dep to avoid a half dozen string replacements?

2. Also support {{ workflow.namespace }} because this is a very common element of keyFormat to allow for namespace isolation by IAM scoped to object store prefixes

I added a TODO for this (since namespaces are not supported in the existing implementation), but should be able to incorporate it into this PR.

[droctothorpe]

We added namespace tag handling and validated that it works as expected, which is a good step towards improved user isolation for log artifacts.

We also added template tag validation. The node server throws an error which bubbles up to the UI and provides clear remediation instructions.

[thesuperzapper]

@droctothorpe can you try and make it support arbitrary spaces within the {{ xxx }}, probably using a regex like {{\s*some_token\s*}}

It's very common to format go templates like{{ xxxxx }} and not like {{xxxx}}, and it would be helpful if users can use the exact same value as their Argo Workflows keyFormat config.

For example, these should all be equivalent:

• {{aaaa}}
• {{ aaaa}}
• {{aaaa }}
• {{ aaaa }}
• {{ aaaa }}

[thesuperzapper]

@droctothorpe also, just to be clear, does this affect KFP V1 pipelines?

If so, we should support separate keyFormat configs for KFP V1 vs V2, because many deployments store their V1 and V2 pipeline logs separately.

[droctothorpe]

@thesuperzapper addressed your penultimate comment with .replace(/\s+/g, '') since the keyFormat should not contain any whitespace anyway.

To your second point, my understanding is that keyFormat is defined at the AWF level rather than the KFP level and is set globally for both v1 and v2. This PR just instructs the UI where to look for the logs. The UI has no say over where the logs are actually being saved.

Apologies for the delayed response. Had to pivot to some dask-kubernetes / xgboost issues for two weeks.

[thesuperzapper]

@droctothorpe why not just use the approach I described in #11010 (comment)?

While I agree removing spaces might seem like a solution, if keyFormat ever adds functions (beyond the default ones with every go template) removing spaces will break the inputs to those functions.

Also, while it would be a little crazy, users might want to include spaces in their S3 object store names.

[droctothorpe]

@droctothorpe why not just use the approach I described in #11010 (comment)?

While I agree removing spaces might seem like a solution, if keyFormat ever adds functions (beyond the default ones with every go template) removing spaces will break the inputs to those functions.

Also, while it would be a little crazy, users might want to include spaces in their S3 object store names.

This was my rationale:

Bucket names can consist only of lowercase letters, numbers, dots (.), and hyphens (-).

https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html

Bucket names can only contain lowercase letters, numeric characters, dashes (-), underscores (_), and dots (.). Spaces are not allowed.

https://cloud.google.com/storage/docs/buckets#:~:text=Your%20bucket%20names%20must%20meet,Spaces%20are%20not%20allowed.

https://github.com/minio/minio-go/blob/cc7769f319debdee18b5b304e2b35166b0165fdb/pkg/s3utils/utils.go#L346

[zijianjoy]

NIT: Run npm run format to pass presubmit test.

[droctothorpe]

I'm going to do some manual validation against v1 to make sure that this didn't introduce any breaking changes.

[droctothorpe]

Manual validation evidence

KFP v2

The run code:

from kfp import dsl
from kfp.compiler.compiler import Compiler
from kfp import Client


@dsl.component()
def foo():
    from random import random

    print(random())


@dsl.pipeline()
def pipeline():
    task1 = foo()
    task1.set_caching_options(False)


# Compiler().compile(pipeline_func=pipeline, package_path="two.yaml")
client = Client()
client.create_run_from_pipeline_func(pipeline)

Logs in the UI:

Deleting the workflow:

> k delete workflows --all && k get workflows
workflow.argoproj.io "pipeline-fzhgs" deleted
No resources found in kubeflow namespace.

Logs still available in the UI:

KFP v1

The run code:

from kfp import Client
from kfp import dsl

# from kfp.compiler.compiler import Compiler
from kfp.components import create_component_from_func


def foo():
    from random import random

    print(random())


foo_comp = create_component_from_func(foo)


@dsl.pipeline()
def pipeline():
    task1 = foo_comp()
    task1.set_caching_options(False)


# Compiler().compile(pipeline_func=pipeline, package_path="two.yaml")
client = Client()
client.create_run_from_pipeline_func(pipeline, arguments={})

Logs in the UI:

Deleting the workflow:

> k delete workflows --all && k get workflows
workflow.argoproj.io "pipeline-vkq4t" deleted
No resources found in kubeflow namespace.

Logs are NOT available in the UI from the logs tab. They must be accessed via output artifacts. This is a known bug that we could probably fix pretty easily, though I recommend doing so in a discrete PR since this PR doesn't introduce this bug, it just doesn't fix it.

A new regression

I decided to run another manual validation and found an unrelated regression.

In the master branch, if you create a run then delete the pod but preserve the workflow manifest, it still fails to retrieve logs!

Unless I'm somehow misconfiguring something, I think it's parsing the AWF manifest incorrectly. All of the workflow parsing logic here seems to be deeply misaligned with the actual workflow structure.

I started working on a fix but it was quite complicated and seemed like scope creep so I figured it might make more sense to tackle it in a standalone PR. That being said, the fact that retrieving artifact keys from the workflow doesn't work arguably increases the urgency of this PR since we always need to fallback to the final handler now when the pod is GCed.

[droctothorpe]

Screw it. @quinnovator 🧙 and I decided to just fix the workflow parsing as part of this PR. Confirmed it works. May have to update tests. Additional commits coming soon.

[droctothorpe]

@zijianjoy, @quinnovator and I decided to just fix getPodLogsStreamFromWorkflow in this PR. It broke as a result of changes to the AWF manifest structure that were probably introduced a while ago. We updated the interfaces and brought the logic up to speed.

To summarize, the frontend tries to get logs from the following three locations in order:

1. Directly from the K8s API.
2. Extracting the artifact path from the AWF manifest (if the pods are GCed but the AWF manifest is not).
3. Inferring the artifact path directly (if the AWF manifest is GCed).

Both 2 and 3 were broken! This PR fixes both. 🎉

[zijianjoy]

/lgtm
/approve

Thank you @droctothorpe for the contribution and validation!

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zijianjoy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• frontend/OWNERS [zijianjoy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment