terrytangyuan
commented
10 months ago Would adding default parameter value work?
Open DocX opened 10 months ago
terrytangyuan
commented
10 months ago Would adding default parameter value work?
stale[bot]
commented
9 months ago This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Jooong
commented
8 months ago Hi, it's also the same story with pod.name. For example, I tried to embed the following content to workflow template to produce prometheus metrics for every pod. Since pod.name is not resolved in non-pod type templates such as steps, I tried to deviate this by using expression ?. and ??. However, this always returns 'na' like the expression mentioned from this issue. Do you have any suggestion for this?
templateDefaults:
metrics:
prometheus:
- name: exec_duration_gauge
labels:
- key: workflow_name
value: "{{workflow.name}}"
- key: workflow_namespace
value: "{{workflow.namespace}}"
- key: pod_name
value: "{{=pod?.name ?? 'na'}}"
help: "Duration gauge by pod name"
gauge:
realtime: true
value: "{{duration}}"
agilgur5
commented
8 months ago Hmm, I wonder if the parser is getting confused by the safe conditional -- since it can't directly look up pod?.name like it can with pod.name. Though IIRC, the entire variable, such as pod, is just passed to expr.
Does re-writing it without the safe conditional short-hand, e.g. as (pod && pod.name) || 'na', work fine?
Jooong
commented
8 months ago Hmm, I wonder if the parser is getting confused by the safe conditional -- since it can't directly look up
pod?.namelike it can withpod.name. Though IIRC, the entire variable, such aspod, is just passed toexpr.Does re-writing it without the safe conditional short-hand, e.g. as
(pod && pod.name) || 'na', work fine?
Thanks for your suggestion, but it seems all node fails with this message.
error: cannot finish template replacement because the result was invalid JSON
There are more messages stated from summary > conditions in the web UI.
MetricsError: unable to substitute parameters for metric 'exec_duration_gauge': failed to evaluate expression: invalid operation: || (mismatched types bool and string) (1:19) | (pod && pod.name) || 'na' | ..................^
Tried {{=pod == nil ? 'na' : pod.name}} as well, but it's always evaluted as 'na'
agilgur5
commented
8 months ago Thanks for checking that!
That's strange that pod == nil would be returning true... only other guess I might have is that nil is a type, so type(pod) == nil ? 'na' : pod.name might be different?
Jooong
commented
8 months ago Thanks for checking that!
That's strange that
pod == nilwould be returningtrue... only other guess I might have is thatnilis a type, sotype(pod) == nil ? 'na' : pod.namemight be different?
I'm afraid this doesn't work either. Attaching the error message.
MetricsError: unable to substitute parameters for metric 'exec_duration_gauge': failed to evaluate expression: reflect: call of reflect.Value.Call on zero Value (1:1) | type(pod) == nil ? "na" : pod.name | ^I'm not really familiar with the source code, but I guess the string pod.name in the pod template is directly replaced to the actual pod name without definition of pod here?
agilgur5
commented
7 months ago I'm afraid this doesn't work either. Attaching the error message.
Thanks for testing this out and confirming!
I'm not really familiar with the source code
I know there's a good bit of expr code in scope.go and validate.go, but it actually appears to be scattered in many places. And that's just a search for "expr", which is not quite everything either; ProcessArgs is a common helper too for instance and does not use expr directly (it uses template.Replace, which uses expr in expressionReplace).
It honestly might be ripe for a refactor to consolidate more of the expression code into one place so that there are less bugs and it's easier to modify holistically. (Not to mention some existing large confusions with the different templating engines: #5142, #9529, #7831, etc). I've been meaning to jump into Argo's templating and start addressing lots of issues, but haven't had a chance to do so yet.
but I guess the string
pod.namein the pod template is directly replaced to the actual pod name without definition ofpodhere?
Oh nice find! I think you might be correct on that one, which would explain quite a bit.
I think this is the same for workflow.parameters (that variable is used in the setGlobalParameters function) and I think workflow.outputs.parameters as well. workflow itself seems to never be defined.
I think this is because the workflow and pod structs (for example) have a lot more data in them than what is used in expressions and there may not necessarily be a 1-to-1 match in naming either. EDIT: thinking about it a bit more, it might make sense to make a secondary facade struct solely for expressions that could make logic like this issue more straightforward / have less gotchas. Although I remember there being some rationale for it with workflow.parameters.json, c.f. #9742 and this line mentioned there... so easier said than done 😕
Pre-requisites
:latestWhat happened/what you expected to happen?
Running workflow with global output parameter, that is used in metrics label value with expression containing "??" always evaluates as if the parameter value is
nil. The reason for using the condition is to support step failing, when the outputs are not generated and in that case I want to fallback to"na".Running the workflow below, after completed, the value of
.spec.metricsisExpected value is:
To check, the value of
.status.outputsis correct as expected:When setting the expression to
"{{= workflow?.outputs?.parameters?.my_global_param }}"(i.e. removing the safe conditional), the evaluated value in the metric label is correct.Version
v3.4.9
Paste a small workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflows that uses private images.
Logs from the workflow controller
Logs from in your workflow's wait container