-
Notifications
You must be signed in to change notification settings - Fork 728
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
feature: add argo workflows list-runs command #1416
base: master
Are you sure you want to change the base?
Conversation
metaflow/plugins/argo/argo_client.py
Outdated
# including the following: spec.workflowTemplateRef.name, status.phase | ||
# Therefore we use labels for filtering the runs, but this has required us to add a label for the workflow-template name | ||
# making the solution not backwards compatible. | ||
filters = ["workflows.argoproj.io/workflow-template=%s" % name] + [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you verify that this solution works when the "name" is rather large?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change this to a hash of the name as discussed. Kubernetes has a max length of 63 for labels, but argo workflow names can be 253 characters long
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! few minor comments
Some thoughts and observations on this feature before shipping. Creating workflows through Argo web UI has a validation that checks that the metadata name does not exceed 63 characters (so it fits in a label). So Argo is much stricter with naming than what we are with submitted workflows. For the Metaflow use case, we can not start truncating workflow names all the way down to 63 characters though, mainly because of how project branches are named. As an example, these two flows would result in a collision with their workflow template names if truncated to 63 characters:
which is a very real possibility with the latest changes of allowing email addresses as usernames for deployed flows. For the scope of this PR, the only solution for adding discoverability to workflows via the Kubernetes API is to introduce a custom label that uses a hash of the workflow template name, as was implemented. |
def _label_hash(name): | ||
# Hash a name for use as a Kubernetes label. | ||
# Use the maximum allowed 63 characters for the hash to minimize collisions. | ||
# Preserve part of the name for legibility purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after some thought, preserving legibility is probably not a priority for a label, as these are not meant for human consumption to begin with. If we want to store the template name as well, this can be done in the annotations which do not have the same length limitations. I don't see a use case for storing the full template name at this point though
this feature is somewhat blocked until #1521 is solved. After that we have a canonical way of generating the workflow-template name label, and can use that for finding the runs. |
addresses part of #1387
Note: due to API limitations, this feature required adding a custom label to workflows with the workflow-template as a value, as labels are the only available filter for custom fields on objects. This makes the feature not backwards compatible, as the necessary label does not exist on older runs.