feat: Add workflow var for originName #10745

Joibel · 2023-03-24T12:28:31Z

Add a global workflow variables to allow generic template metrics to identify which workflow they originate from.

Issue #6772 asks for the template name, but we cannot always know what this is (simplest unknowable is kubectl apply of a workflow with a generateName). So provide that by deduction as originName. See the tests added in this commit for what we're aiming for there.

I have added both variables and tested them by invoking very simple workflows. The regex used to generate originName from the Name has a test suite, and has been checked from simple workflows also.

Fixes #6772

rohankmr414

LGTM, requested a minor typo fix change

docs/variables.md

rohankmr414

LGTM!

terrytangyuan · 2023-04-26T23:14:13Z

docs/variables.md

 | `workflow.namespace` | Workflow namespace |
+| `workflow.mainEntrypoint` | Workflow's initial entrypoint |


This is static. What's the use case to have this available?

The original problem was distinguishing which high level workflow has invoked us. This is not a static for a WorkflowTemplate called from a another place.
Given this use in a workflowTemplate:

apiVersion: argoproj.io/v1alpha1 kind: WorkflowTemplate metadata: name: say-main-entrypoint spec: entrypoint: echo templates: - name: echo container: image: alpine command: [echo] args: ["{{workflow.mainEntrypoint}}"]

I can distinguish my caller:

apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: generateName: foo- spec: entrypoint: foo templates: - name: foo steps: - - name: step templateRef: name: say-main-entrypoint template: echo

results in a log of foo

apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: generateName: bar- spec: entrypoint: bar templates: - name: bar steps: - - name: step templateRef: name: say-main-entrypoint template: echo

results in a log of bar

It fulfills at least some of the original need in #6772 in a different way, and in a less naughty fashion than the templateName option. @terrytangyuan - if you're happy that it's actually useful, I'll add some more documentation to this PR as it's obvious I haven't explained what use this is.

isubasinghe · 2023-04-28T07:38:09Z

LGMT

juliev0 · 2023-05-01T22:26:48Z

docs/variables.md

@@ -243,7 +243,9 @@ For `Template`-level metrics:
 | Variable | Description|
 |----------|------------|
 | `workflow.name` | Workflow name |
+| `workflow.templateName` | Workflow template name. This works only for template names, it may give wrong answers for non-generated names |


At the risk of being verbose, I wonder if we should call this workflowTemplateName? When I see templateName, I think of the individual step, not the WorkflowTemplate

From this link: "The terms WorkflowTemplate and template have created an unfortunate naming collision and have created some confusion in the past."

I know what you mean. To fit the two part naming of all the rest of the variables how about workflowTemplate.name?

For my understanding, does this also represent the CronWorkflow name for a Workflow generated by a CronWorkflow?

@juliev0, yes, it should do. Ideally it would always be able to derive the name from the parent, but as the names are generated and this attempts to reverse that, I'm basing it on what others in #6772 have found works.

So, in that case using workflowTemplate in the name would also be inaccurate. I'm thinking something like origin, or otherwise dividing this into multiple variables. What do you think?

So workflow.origin is your proposal? Or workflow.originName to demonstrate it's the name of something perhaps.

workflow.originName sounds good to me. If you'd like you can wait a few days to see if anyone chimes in first, for risk of your changing it once and then needing to change it again.

juliev0 · 2023-05-08T15:58:51Z

workflow/controller/operator_test.go

+	} {
+		assert.Equal(t, expected, getTemplateNameFromName(input))
+	}
+}


So, it looks like if there's a Workflow whose actual name is for example template-name-1234567890, we will assume that the origin is template-name, right?

Yes, that's the point of these tests. Show how originName will behave given a specific name being fed to it as the workflow name.

Oh, sorry, let me clarify my question. What I meant was, say there's a Workflow that wasn't created from a WorkflowTemplate or a CronWorkflow, and it just happens to be named in a way in which it has the "-" followed by a certain number of letters or numbers. In that case, the actual origin should be the entire Workflow name, right?

It should, but it won't be. We're reverse engineering the name. The same might happen for a workflow submitted with a generateName: or even just a name that matches. I'd prefer to fix this with some "buyer beware" stickers in the documentation - if you want to fool this you're welcome to but you won't get what you want. We could get the controller generated workflows to add annotations of their original name so that we can then re-injest that as the correct name, but that is a fair amount of work to avoid users deliberately being silly when they can just read the instructions instead and not fool the regex. And it still won't work in all cases where workflows are being injected by other tooling including kubectl and argo-events.

The result of a Workflow submitted with a generateName might actually be pretty good - if your generateName is foobar- I guess the origin would be foobar, right?

The result of a Workflow submitted with a generateName might actually be pretty good - if your generateName is foobar- I guess the origin would be foobar, right?

Yes. But it's impossible to distinguish between name: foobar-123467890 and generateName: foobar-.

Just thinking about the alternative - I guess somebody would need to include their own sprig function to perform the same transformation you did, right? I recognize that would make the Workflow more complex.

They can already do this, and are... see the issue #6772 this is trying to assist with. We don't need to support overriding the regex in this facility as users can and are already implementing that.

The reliable answer is to use the alternative I've provided in this PR, the entrypoint name instead.

I like the entrypoint functionality. Are you thinking of maybe just including that in this PR then?

I'd prefer to include both.
mainEntrypoint is the 'right answer', but it requires work and requires that users write all their workflows with a differing entrypoint, which is not initially likely, as probably they all have them called main or at least haven't given them good, unique names. Adding controls to ensure mainEntrypoint is unique for all your workflows ever is problematic.
Because of this, the alternative originName is a quick fix for people who want a nice distinguishing name with minimal effort and has been shown to work in the field effectively. We can then determine whether we should put more effort into that solution when issues arise, but also point at mainEntrypoint in the docs. Perhaps we can implement workflow automatically generated annotations from common sources if it proved a popular but flawed feature.

Let me do the documentation that's needed from Terry's first comment, and that might help.

Okay, thanks. I'll leave it to @terrytangyuan to decide if we should include a variable that should only be used in certain circumstances and won't work if used in the wrong circumstance, if it provides value to people. (I'm not sure if we've come across that type of thing before)

juliev0 · 2023-05-08T16:12:38Z

workflow/controller/operator_test.go

+	woc.operate(ctx)
+	assert.Equal(t, "whalesay", woc.globalParams[common.GlobalVarWorkflowMainEntrypoint])
+}
+


Is it possible to add a test for the more interesting cases involving WorkflowTemplate and mainEntryPoint?

Yes, I'm pretty sure that's doable, I will do once we've settled on functionality.

terrytangyuan · 2023-05-16T17:37:05Z

We discussed it today at the contributors meeting. My concern is that this is one-off solution and introduces confusion to users when it doesn't work for other cases. Any thoughts? @sarabala1979

Issue argoproj#6772 asks for the template name, but we cannot always know what this is (simplest unknowable is kubectl apply of a workflow with a generateName). This is a split from PR argoproj#10745 to just provide the variable mainEndpoint, which is set to the name of the invoked entrypoint, which is controllable, knowable, and could be used in the same way, and seems like a useful thing to have anyway. Signed-off-by: Alan Clucas <[email protected]>

stale · 2023-06-18T00:55:31Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

Issue argoproj#6772 asks for the template name, but we cannot always know what this is (simplest unknowable is kubectl apply of a workflow with a generateName:) So provide that by deduction as originName. See the tests added in this commit for what we're aiming for there. Addresses argoproj#6772. Signed-off-by: Alan Clucas <[email protected]>

Signed-off-by: Alan Clucas <[email protected]>

stale · 2023-08-12T13:34:31Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

stale · 2023-09-17T03:50:48Z

This issue has been closed due to inactivity. Feel free to re-open if you still encounter this issue.

Joibel · 2023-09-18T13:53:54Z

I'm finding more use cases for this, so as a basic proposal I'm planning to add a label to the sources of workflows that we control that can supply this originName. The label will be used where present to provide this variable.

juliev0 · 2023-12-08T19:29:55Z

Any plans to continue this?

Joibel · 2023-12-11T13:27:09Z

Not as is. I'll write up a proposal as an issue instead for discussion.

Joibel force-pushed the templateName branch from a14c9b3 to 16baf55 Compare March 24, 2023 12:30

terrytangyuan changed the title ~~WIP~~ feat: Add workflow vars for template name and entrypoint Mar 24, 2023

Joibel force-pushed the templateName branch 5 times, most recently from f148456 to 9f7e93e Compare March 24, 2023 20:09

Joibel marked this pull request as ready for review March 24, 2023 20:41

rohankmr414 suggested changes Mar 28, 2023

View reviewed changes

docs/variables.md Outdated Show resolved Hide resolved

Joibel force-pushed the templateName branch from b7bed37 to 03b755d Compare March 28, 2023 11:47

tico24 requested a review from isubasinghe March 29, 2023 06:15

tico24 mentioned this pull request Mar 30, 2023

Release v3.4.6 cherry-pick candidates #10747

Closed

33 tasks

caelan-io requested a review from rohankmr414 April 4, 2023 15:03

rohankmr414 approved these changes Apr 25, 2023

View reviewed changes

terrytangyuan reviewed Apr 26, 2023

View reviewed changes

isubasinghe approved these changes Apr 28, 2023

View reviewed changes

juliev0 reviewed May 1, 2023

View reviewed changes

juliev0 reviewed May 8, 2023

View reviewed changes

Joibel mentioned this pull request May 28, 2023

feat: mainEntrypoint variable #11151

Merged

stale bot added the problem/stale This has not had a response in some time label Jun 18, 2023

Joibel force-pushed the templateName branch from 03b755d to 0d23c5a Compare June 20, 2023 20:09

stale bot removed the problem/stale This has not had a response in some time label Jun 20, 2023

Joibel changed the title ~~feat: Add workflow vars for template name and entrypoint~~ feat: Add workflow var for originName Jun 20, 2023

empty commit to retry tests

eed3d1f

Signed-off-by: Alan Clucas <[email protected]>

stale bot added the problem/stale This has not had a response in some time label Aug 12, 2023

stale bot closed this Sep 17, 2023

Joibel reopened this Sep 18, 2023

stale bot removed the problem/stale This has not had a response in some time label Sep 18, 2023

agilgur5 added the area/templating Templating with `{{...}}` label Sep 26, 2023

Joibel closed this Dec 11, 2023

Joibel deleted the templateName branch January 15, 2024 15:27

Joibel restored the templateName branch January 23, 2024 11:49

menzenski mentioned this pull request May 21, 2024

proposal: metrics overhaul including opentelemetry #12589

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add workflow var for originName #10745

feat: Add workflow var for originName #10745

Joibel commented Mar 24, 2023 •

edited

Loading

rohankmr414 left a comment

rohankmr414 left a comment

terrytangyuan Apr 26, 2023

Joibel Apr 27, 2023 •

edited by agilgur5

Loading

isubasinghe commented Apr 28, 2023

juliev0 May 1, 2023

juliev0 May 1, 2023

Joibel May 3, 2023

juliev0 May 4, 2023

Joibel May 5, 2023

juliev0 May 5, 2023

Joibel May 5, 2023

juliev0 May 5, 2023

juliev0 May 8, 2023

Joibel May 8, 2023

juliev0 May 8, 2023

Joibel May 11, 2023

juliev0 May 11, 2023

Joibel May 12, 2023

Joibel May 12, 2023

juliev0 May 12, 2023

Joibel May 13, 2023

juliev0 May 13, 2023

juliev0 May 8, 2023

Joibel May 11, 2023

terrytangyuan commented May 16, 2023

stale bot commented Jun 18, 2023

stale bot commented Aug 12, 2023

stale bot commented Sep 17, 2023

Joibel commented Sep 18, 2023

juliev0 commented Dec 8, 2023

Joibel commented Dec 11, 2023

		\| `workflow.namespace` \| Workflow namespace \|
		\| `workflow.mainEntrypoint` \| Workflow's initial entrypoint \|

feat: Add workflow var for originName #10745

feat: Add workflow var for originName #10745

Conversation

Joibel commented Mar 24, 2023 • edited Loading

rohankmr414 left a comment

Choose a reason for hiding this comment

rohankmr414 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joibel Apr 27, 2023 • edited by agilgur5 Loading

Choose a reason for hiding this comment

isubasinghe commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrytangyuan commented May 16, 2023

stale bot commented Jun 18, 2023

stale bot commented Aug 12, 2023

stale bot commented Sep 17, 2023

Joibel commented Sep 18, 2023

juliev0 commented Dec 8, 2023

Joibel commented Dec 11, 2023

Joibel commented Mar 24, 2023 •

edited

Loading

Joibel Apr 27, 2023 •

edited by agilgur5

Loading