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

[jswxstw]

Fixes #8790

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]

@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: https://github.com/argoproj/argo-workflows/issues/8790#issuecomment-1743614552 whereas 99% of users (including me) won't and would benefit from your PR

[juliev0]

Thanks for noticing that @tooptoop4 ! I believe this is one motivation for your making https://github.com/argoproj/argo-workflows/pull/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]

@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.

[juliev0]

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

[tooptoop4]

@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/cobra@v1.5.0/command.go:876 +0x67b
github.com/spf13/cobra.(*Command).ExecuteC(0xc0008da000)
        /go/pkg/mod/github.com/spf13/cobra@v1.5.0/command.go:990 +0x3bd
github.com/spf13/cobra.(*Command).Execute(...)
        /go/pkg/mod/github.com/spf13/cobra@v1.5.0/command.go:918
github.com/spf13/cobra.(*Command).ExecuteContext(0x3546178?, {0x3547bb8?, 0xc0009a3e40?})
        /go/pkg/mod/github.com/spf13/cobra@v1.5.0/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]

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]

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

[jswxstw]

@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]

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]

@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!

[tooptoop4]

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

accepted in https://github.com/argoproj/argo-workflows/pull/13761#discussion_r1802282712

[agilgur5]

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]

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

[shuangkun]

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.