feat(controller) Emissary executor. #4925
Conversation
alexec
force-pushed
the
emissary
branch
3 times, most recently
from
264b6c5 to
e8a679b
Compare
alexec
force-pushed
the
emissary
branch
5 times, most recently
from
f4378ac to
037f3fe
Compare
alexec
force-pushed
the
emissary
branch
2 times, most recently
from
09d6241 to
cb8886e
Compare
| } | ||
| volumeMountVarArgo = apiv1.VolumeMount{ | ||
| Name: volumeVarArgo.Name, | ||
| MountPath: "/var/run/argo", |
There was a problem hiding this comment.
Consider moving to a constant (similarly for other strings)?
| if len(c.Command) == 0 { | ||
| c.Command = woc.getCommandFor(c.Image) | ||
| } | ||
| if len(c.Command) == 0 { |
There was a problem hiding this comment.
Readability suggestion: consider checking length of woc.getCommandFor(c.Image) instead so that these two same if conditions can be merged.
| } | ||
| } | ||
|
|
||
| func (e emissary) isComplete(containerNames []string) bool { |
There was a problem hiding this comment.
| func (e emissary) isComplete(containerNames []string) bool { | |
| func (e emissary) isCompleted(containerNames []string) bool { |
| func (e emissary) CopyFile(_ string, sourcePath string, destPath string, _ int) error { | ||
| // this implementation is very different, because we expect the emissary binary has already compressed the file | ||
| // so no compression can or needs to be implemented here | ||
| // TODO - warn the user we ignored compression? |
There was a problem hiding this comment.
We can perhaps just add a note on this in the executor documentation
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
…proj#5091) Signed-off-by: Simon Behar <[email protected]> Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Co-authored-by: Yuan Tang <[email protected]> Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
cmd/argoexec/commands/emissary.go
Outdated
| command.Env = os.Environ() | ||
| command.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} | ||
|
|
||
| stdout, err := os.Create(varArgo + "/ctr/" + containerName + "/stdout") |
There was a problem hiding this comment.
I'm aware we only need to capture stdout for (a) main containers and (b) if there our outputs - v1.1
There was a problem hiding this comment.
could use ioutil.Discard for that
cmd/argoexec/commands/emissary.go
Outdated
| } | ||
| } | ||
| } else { | ||
| println("not saving outputs - not main container") |
There was a problem hiding this comment.
this will be mingled with the containers logs, it might be good to make it conventional, e.g.
#argo msg=not saving outputs
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
There was a problem hiding this comment.
I think there is something missing in the implementation. As far as I can tell, we can infer an image's entrypoint with the configmap setting but we are not able to infer the image's args.
Based on past experience, the interplay between command and args is nuanced. e.g. "If you supply a command but no args for a Container, only the supplied command is used". So we would need to replicate this behavior in the emissary. See kubernetes docs:
https://kubernetes.io/docs/tasks/inject-data-application/define-command-argument-container/#notes
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
|
@jessesuen I've updated this PR to allow you to specify both command and args in the config. Arguments are only actually need under the scenario when the command is missing. I worked through the 4 rules to determine this. |
Signed-off-by: Alex Collins <[email protected]>
| # The command for each image, needed when the command is not specified and the emissary executor is used. | ||
| # https://argoproj.github.io/argo-workflows/workflow-executors/#image-command-index | ||
| images: | | ||
| argoproj/argosay:v1: [cowsay] | ||
| argoproj/argosay:v2: [/argosay] | ||
| docker/whalesay:latest: [cowsay] | ||
| python:alpine3.6: [python3] | ||
|
|
||
|
|
There was a problem hiding this comment.
I think this doc needs to be corrected.
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
|
Let's do this!!! |
|
Hooray! Thanks for your work on this! @alexec |
#2679 #4766