refactor: Only set `ARGO_TEMPLATE` env for init container. #13761

jswxstw · 2024-10-15T13:24:51Z

Motivation

duplicative env var ARGO_TEMPLATE result in etcd size 1MB limit

Modifications

Only set ARGO_TEMPLATE env for init container, wait container read it from the template file written by init container.

Verification

existing UT or E2E tests have covered it.

tooptoop4 · 2024-10-15T20:20:45Z

@jswxstw this is great but can u make this change enabled via an environment variable (ARGO_TEMPLATE_ONLY_ON_INIT)? @juliev0 mentioned one of their workflows in main container uses the variable: #8790 (comment) whereas 99% of users (including me) won't and would benefit from your PR

juliev0 · 2024-10-16T01:24:05Z

Thanks for noticing that @tooptoop4 ! I believe this is one motivation for your making #13742 configurable, right?

@jswxstw will it make the code too ugly to do that? I'll admit that my use of $ARGO_TEMPLATE environment variable was a hacky workaround for exceeding the maximum size of an argument, as opposed to a promise that Argo Workflows makes will exist. And admittedly, I was later in a separate situation in which for a different Workflow even this hack didn't work, as the environment variable size made the overall Container size too big as I recall. If needed, I can adjust our Workflow so it doesn't try to do this at all. I guess the only question is whether there's anyone else out there for which this will break.

jswxstw · 2024-10-16T02:19:47Z

@jswxstw will it make the code too ugly to do that?

@juliev0 Agree, I think we should exercise restraint in adding environment variables, as it could increase complexity and confuse users.
Also, #12325 has supported ARGO_TEMPLATE env offload by default, it will also affect users who use this env. @tooptoop4
I do think ARGO_TEMPLATE is an internal environment variable, which is not documented and users should not rely on it.

Signed-off-by: oninowang <[email protected]>

docs/environment-variables.md

Signed-off-by: oninowang <[email protected]>

agilgur5

So this is no longer a feature (unlike #13742, which added an env var) as it doesn't add anything; could be a fix or refactor depending on if we want to auto-backport it

I think we should exercise restraint in adding environment variables, as it could increase complexity and confuse users.

Agree.

I do think ARGO_TEMPLATE is an internal environment variable, which is not documented and users should not rely on it.

Also agree. I've said similar things elsewhere too and there is precedent for this.

If we wanted to be extra safe, we could make this a breaking fix! and leave a note in the upgrading guide.

juliev0 · 2024-10-16T23:45:50Z

Hey @shuangkun - do you want to review this since you added the configmap offloading PR which it's based on?

tooptoop4 · 2024-10-17T04:23:06Z

@jswxstw

when i tried to patch this got below error in the wait container:


time="2024-10-17T01:57:33 UTC" level=info msg="Starting Workflow Executor" version=v3.4.11
panic: unexpected end of JSON input
goroutine 1 [running]:
github.com/argoproj/argo-workflows/v3/cmd/argoexec/commands.checkErr({0x3517da0, 0xc000011a40})
        /go/src/github.com/argoproj/argo-workflows/cmd/argoexec/commands/root.go:146 +0x6b
github.com/argoproj/argo-workflows/v3/cmd/argoexec/commands.initExecutor()
        /go/src/github.com/argoproj/argo-workflows/cmd/argoexec/commands/root.go:101 +0x3b5
github.com/argoproj/argo-workflows/v3/cmd/argoexec/commands.waitContainer({0x3547bb8, 0xc0009a3e40})
        /go/src/github.com/argoproj/argo-workflows/cmd/argoexec/commands/wait.go:28 +0x4f
github.com/argoproj/argo-workflows/v3/cmd/argoexec/commands.NewWaitCommand.func1(0xc0008dbb80?, {0x2d958d7?, 0x0?, 0x0?})
        /go/src/github.com/argoproj/argo-workflows/cmd/argoexec/commands/wait.go:18 +0x2a
github.com/spf13/cobra.(*Command).execute(0xc0008dbb80, {0x4cda578, 0x0, 0x0})
        /go/pkg/mod/github.com/spf13/[email protected]/command.go:876 +0x67b
github.com/spf13/cobra.(*Command).ExecuteC(0xc0008da000)
        /go/pkg/mod/github.com/spf13/[email protected]/command.go:990 +0x3bd
github.com/spf13/cobra.(*Command).Execute(...)
        /go/pkg/mod/github.com/spf13/[email protected]/command.go:918
github.com/spf13/cobra.(*Command).ExecuteContext(0x3546178?, {0x3547bb8?, 0xc0009a3e40?})
        /go/pkg/mod/github.com/spf13/[email protected]/command.go:911 +0x4a
main.main()
        /go/src/github.com/argoproj/argo-workflows/cmd/argoexec/main.go:21 +0x9e

actually i think i'm just not building locally properly, how do you do it?

jswxstw · 2024-10-17T06:10:51Z

time="2024-10-17T01:57:33 UTC" level=info msg="Starting Workflow Executor" version=v3.4.11

It seems you are using the argoexec image of old version: v3.4.11. Not updated? @tooptoop4

tooptoop4 · 2024-10-17T06:26:40Z

@jswxstw what command do u use to build the controller locally?

jswxstw · 2024-10-17T06:35:04Z

@tooptoop4 I used make start UI=true, like developing locally.
Please note this, make argoexec-image will build latest image and import it into the k3d cluster:k3s-default by default.

jswxstw · 2024-10-17T07:22:15Z

So this is no longer a feature (unlike #13742, which added an env var) as it doesn't add anything; could be a fix or refactor depending on if we want to auto-backport it

@agilgur5 Agree, this PR should be a refactor, based on the feature configmap offloading and ARGO_TEMPLATE simplifying.

tooptoop4 · 2024-10-17T07:28:21Z

@tooptoop4 I used make start UI=true, like developing locally. Please note this, make argoexec-image will build latest image and import it into the k3d cluster:k3s-default by default.

ah this makes sense, i need to build the exec image too not just the controller!

terrytangyuan

This seems like a breaking change. I remember Intuit or other vendors depend on this. @juliev0 Do you remember?

tooptoop4 · 2024-10-17T19:26:55Z

This seems like a breaking change. I remember Intuit or other vendors depend on this. @juliev0 Do you remember?

accepted in #13761 (comment)

agilgur5 · 2024-10-17T23:30:57Z

This seems like a breaking change. I remember Intuit or other vendors depend on this.

It's also an undocumented, internal environment variable, so shouldn't have been relied on by users as mentioned by @jswxstw and I above

juliev0 · 2024-10-17T23:38:30Z

Thanks for keeping that in mind, though, @terrytangyuan!

terrytangyuan

Thanks!

shuangkun · 2024-10-18T15:10:12Z

Hey @shuangkun - do you want to review this since you added the configmap offloading PR which it's based on?

Sorry, I just found time now. This PR has been merged and it seems there is no problem.

jswxstw force-pushed the feat-8790 branch from 87e02d3 to 00a0853 Compare October 15, 2024 13:26

jswxstw added area/controller Controller issues, panics area/executor labels Oct 15, 2024

jswxstw force-pushed the feat-8790 branch from 00a0853 to 85d24fc Compare October 15, 2024 13:31

jswxstw marked this pull request as draft October 15, 2024 13:39

jswxstw added 2 commits October 16, 2024 10:58

feat: Only set ARGO_TEMPLATE env for init container.

0eaae26

Signed-off-by: oninowang <[email protected]>

fix(test): adapt ARGO_TEMPLATE env changes.

6aadfae

Signed-off-by: oninowang <[email protected]>

jswxstw force-pushed the feat-8790 branch from bf9a9a5 to 6aadfae Compare October 16, 2024 02:58

juliev0 reviewed Oct 16, 2024

View reviewed changes

docs/environment-variables.md Outdated Show resolved Hide resolved

fix: remove ARGO_TEMPLATE_WITH_INPUTS_PARAMETERS env.

c758c69

Signed-off-by: oninowang <[email protected]>

jswxstw marked this pull request as ready for review October 16, 2024 04:53

tooptoop4 approved these changes Oct 16, 2024

View reviewed changes

tooptoop4 mentioned this pull request Oct 16, 2024

feat: ARGO_TEMPLATE env var without inputs.parameters #13742

Merged

agilgur5 reviewed Oct 16, 2024

View reviewed changes

jswxstw changed the title ~~feat: Only set ARGO_TEMPLATE env for init container.~~ refactor: Only set ARGO_TEMPLATE env for init container. Oct 17, 2024

terrytangyuan reviewed Oct 17, 2024

View reviewed changes

terrytangyuan approved these changes Oct 18, 2024

View reviewed changes

terrytangyuan merged commit c9b1477 into argoproj:main Oct 18, 2024
30 checks passed

agilgur5 mentioned this pull request Oct 18, 2024

docs: new features in 3.6 #13739

Merged

tooptoop4 mentioned this pull request Oct 19, 2024

reduce pod definition size #13089

Open

8 tasks

Joibel mentioned this pull request Oct 24, 2024

docs: add 3.6 upgrading notes for postgres archive and ARGO_TEMPLATE #13803

Merged

jswxstw mentioned this pull request Nov 1, 2024

REQUEST: Promotion to Reviewer for @jswxstw argoproj/argoproj#337

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Only set `ARGO_TEMPLATE` env for init container. #13761

refactor: Only set `ARGO_TEMPLATE` env for init container. #13761

jswxstw commented Oct 15, 2024 •

edited

Loading

tooptoop4 commented Oct 15, 2024

juliev0 commented Oct 16, 2024 •

edited by agilgur5

Loading

jswxstw commented Oct 16, 2024

agilgur5 left a comment •

edited

Loading

juliev0 commented Oct 16, 2024 •

edited by agilgur5

Loading

tooptoop4 commented Oct 17, 2024 •

edited

Loading

jswxstw commented Oct 17, 2024

tooptoop4 commented Oct 17, 2024

jswxstw commented Oct 17, 2024

jswxstw commented Oct 17, 2024 •

edited

Loading

tooptoop4 commented Oct 17, 2024

terrytangyuan left a comment

tooptoop4 commented Oct 17, 2024

agilgur5 commented Oct 17, 2024 •

edited

Loading

juliev0 commented Oct 17, 2024

terrytangyuan left a comment

shuangkun commented Oct 18, 2024

refactor: Only set ARGO_TEMPLATE env for init container. #13761

refactor: Only set ARGO_TEMPLATE env for init container. #13761

Conversation

jswxstw commented Oct 15, 2024 • edited Loading

Motivation

Modifications

Verification

tooptoop4 commented Oct 15, 2024

juliev0 commented Oct 16, 2024 • edited by agilgur5 Loading

jswxstw commented Oct 16, 2024

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

juliev0 commented Oct 16, 2024 • edited by agilgur5 Loading

tooptoop4 commented Oct 17, 2024 • edited Loading

jswxstw commented Oct 17, 2024

tooptoop4 commented Oct 17, 2024

jswxstw commented Oct 17, 2024

jswxstw commented Oct 17, 2024 • edited Loading

tooptoop4 commented Oct 17, 2024

terrytangyuan left a comment

Choose a reason for hiding this comment

tooptoop4 commented Oct 17, 2024

agilgur5 commented Oct 17, 2024 • edited Loading

juliev0 commented Oct 17, 2024

terrytangyuan left a comment

Choose a reason for hiding this comment

shuangkun commented Oct 18, 2024

refactor: Only set `ARGO_TEMPLATE` env for init container. #13761

refactor: Only set `ARGO_TEMPLATE` env for init container. #13761

jswxstw commented Oct 15, 2024 •

edited

Loading

juliev0 commented Oct 16, 2024 •

edited by agilgur5

Loading

agilgur5 left a comment •

edited

Loading

juliev0 commented Oct 16, 2024 •

edited by agilgur5

Loading

tooptoop4 commented Oct 17, 2024 •

edited

Loading

jswxstw commented Oct 17, 2024 •

edited

Loading

agilgur5 commented Oct 17, 2024 •

edited

Loading