feat: Add exit code as output variable #2111

rdigiorgio · 2020-01-31T13:06:03Z

update OpenAPI spec, Go Outputs model and TS Outputs model
add an "exit-code-output-variable" example
implement "GetExitCode(containerID string) (int32, error)" for all executors

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
Optional. I've added My organization is added to the README.
I've signed the CLA and required builds are green.

simster7

Amazing work so far! This looks pretty thorough. I just have some initial comments and I'll ask you to write some tests for this

pkg/apis/workflow/v1alpha1/workflow_types.go

ui/src/models/workflows.ts

simster7 · 2020-01-31T18:47:48Z

workflow/executor/docker/docker.go

@@ -86,6 +89,37 @@ func (d *DockerExecutor) GetOutputStream(containerID string, combinedOutput bool
 	return reader, nil
 }

+func (d *DockerExecutor) GetExitCode(containerID string) (int32, error) {
+	cmd := exec.Command("docker", "inspect", containerID, "--format='{{.State.ExitCode}}'")
+	log.Info(cmd.Args)


I think this can be removed

You mean logging the command args ?

Either removing it or making it Debug and adding context to it would work. Maybe something like

Suggested change

log.Info(cmd.Args)

log.Debug("Exit code docker command args", cmd.Args)

workflow/executor/docker/docker.go

workflow/executor/k8sapi/k8sapi.go

workflow/executor/kubelet/kubelet.go

simster7 · 2020-01-31T18:59:35Z

Also seems like you need to run make codegen

- update OpenAPI spec, Go Outputs model and TS Outputs model - add an "exit-code-output-variable" example - implement "GetExitCode(containerID string) (int32, error)" for all executors

rdigiorgio · 2020-02-03T08:28:18Z

I see there is a generated class, a mock, that is not up to date with the interface it mocks.
It is the ContainerRuntimeExecutor, and I do not know how to use mockery to generate it.
I thought the make codegen would do it.

I edited it manually the first time, which is wrong. :)

simster7 · 2020-02-03T20:39:53Z

Hi @rdigiorgio nice catch! I have updated the mock for you, and will make a PR to make sure it gets updated with make codegen. Your PR is almost done, I just need to see a unit test and then it should be good to go :)

codecov · 2020-02-03T20:51:09Z

Codecov Report

Merging #2111 into master will decrease coverage by 0.01%.
The diff coverage is 11.90%.

@@            Coverage Diff             @@
##           master    #2111      +/-   ##
==========================================
- Coverage   11.62%   11.60%   -0.02%     
==========================================
  Files          84       84              
  Lines       32871    32932      +61     
==========================================
+ Hits         3820     3821       +1     
- Misses      28525    28584      +59     
- Partials      526      527       +1

Impacted Files	Coverage Δ
cmd/argo/commands/retry.go	`0.00% <0.00%> (ø)`
pkg/apis/workflow/v1alpha1/generated.pb.go	`0.46% <0.00%> (-0.01%)`	⬇️
pkg/apis/workflow/v1alpha1/workflow_types.go	`10.73% <0.00%> (-0.06%)`	⬇️
...kg/apis/workflow/v1alpha1/zz_generated.deepcopy.go	`0.00% <0.00%> (ø)`
server/workflow/workflow_server.go	`35.84% <0.00%> (ø)`
workflow/controller/steps.go	`63.38% <ø> (ø)`
workflow/executor/executor.go	`4.29% <0.00%> (-0.09%)`	⬇️
workflow/util/util.go	`27.38% <21.62%> (ø)`
workflow/controller/operator.go	`60.29% <28.57%> (-0.16%)`	⬇️
workflow/validate/validate.go	`74.34% <50.00%> (-0.07%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f95e23...c9a3587. Read the comment docs.

rdigiorgio · 2020-02-07T14:15:25Z

Ok @simster7 thanks.
I am not sure I will be able to create a unit test without further guidance, being a newbie using Golang.

Which part should be tested ?

executor implementations, making sure the correctly get the exit code ?
making sure the wait command captures the exit code ?
an end-to-end test with a workflow manifest and making sure the expected ext code is returned by a given step / DAG task ?

simster7 · 2020-02-28T17:55:25Z

Hi @rdigiorgio. So sorry for the delay in reviewing this. I tried to run this locally, but it didn't work. I think the issue is here:

https://github.com/rdigiorgio/argo/blob/e276fe91120ece9cf087354dcc11cc433595dca9/workflow/executor/pns/pns.go#L236

It seems that you are trying to compare the docker hash, which from the wait container logs is just a hashstring:

...
time="2020-02-28T17:48:13Z" level=error msg="executor error: could not find container with id: c410cb388b756febffd45fe9cb0ee17d4eba173acc51f63c962353085a93710e"
...

directly with a full container ID, which contains additional characters:

                    "terminated": {
                        "containerID": "docker://c410cb388b756febffd45fe9cb0ee17d4eba173acc51f63c962353085a93710e",
                        "exitCode": 123,
                        "finishedAt": "2020-02-28T17:48:12Z",
                        "reason": "Error",
                        "startedAt": "2020-02-28T17:48:12Z"
                    }

Perhaps change the line to use strings.Contains(...)?

In light of this, could you please make sure that the exit code scrapers work with all executors?

Docker
PNS
Kubelet
K8s API

The test could be a simple as an E2E test showing that the exit code gets saved to its corresponding location in NodeStatus.Outputs.

Thanks so much for all this work! Looking forward to your changes

CLAassistant · 2020-03-17T12:53:18Z

All committers have signed the CLA.

simster7 · 2020-03-18T15:33:23Z

@rdigiorgio Do you think you'll have time to finish this PR? We would love to get this merged!

salanki · 2020-03-29T04:58:57Z

Very excited about this functionality @rdigiorgio. Great work!

simster7 · 2020-03-31T00:03:47Z

@rdigiorgio one more gentle ping 🙂. If anyone from the community is interested in taking this up, let me know.

rdigiorgio · 2020-03-31T07:13:19Z

I am sorry, I am currently not able to spend time on this nor anything else than work for the moment. :(

simster7 · 2020-03-31T17:51:36Z

I am sorry, I am currently not able to spend time on this nor anything else than work for the moment. :(

No worries at all @rdigiorgio, I can take over this PR for you. You will still get the commit credit for this work. All the best and stay safe!

sonarcloud · 2020-04-14T19:58:33Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

2.6% Coverage
0.0% Duplication

rdigiorgio mentioned this pull request Jan 31, 2020

Expose exit code in as output parameter #1773

Closed

simster7 self-assigned this Jan 31, 2020

simster7 suggested changes Jan 31, 2020

View reviewed changes

rdigiorgio added 3 commits February 1, 2020 13:51

add exit code as output variable to any script / container template

b74ccf8

- update OpenAPI spec, Go Outputs model and TS Outputs model - add an "exit-code-output-variable" example - implement "GetExitCode(containerID string) (int32, error)" for all executors

use conventional comments for exit code property

89e5215

use int for exit code and add messages on executor errors

4711ef7

rdigiorgio force-pushed the feature/script-output-variable branch from 159ac94 to 4711ef7 Compare February 1, 2020 13:02

rdigiorgio added 3 commits February 1, 2020 14:49

execute "make codegen"

b970891

fixed protobuf bytes -> varint

ba150c1

fix compilation error

065c684

Update ContainerRuntimeExecutor mock

64e567e

Merge branch 'master' into HEAD

e276fe9

simster7 added 5 commits April 12, 2020 11:51

Merge branch 'master' into exit-code

382387f

Refactored code and fixed bugs

392aea7

Revert example change

078feee

Codegen

8ce922e

Merge branch 'master' into feature/script-output-variable

c9a3587

simster7 changed the title ~~feat: Add exit code as output variable to any script / container template. Closes #1773~~ feat: Add exit code as output variable Apr 14, 2020

simster7 approved these changes Apr 14, 2020

View reviewed changes

simster7 merged commit f3eeca6 into argoproj:master Apr 14, 2020

seddonm1 mentioned this pull request Apr 26, 2020

Exit code as output variable not resolving #2849

Closed

4 tasks

tumido mentioned this pull request Jan 11, 2021

feat: Expose exitCode to step level metrics #4861

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add exit code as output variable #2111

feat: Add exit code as output variable #2111

rdigiorgio commented Jan 31, 2020 •

edited

simster7 left a comment

simster7 Jan 31, 2020

rdigiorgio Feb 1, 2020

simster7 Feb 3, 2020

simster7 commented Jan 31, 2020

rdigiorgio commented Feb 3, 2020 •

edited

simster7 commented Feb 3, 2020

codecov bot commented Feb 3, 2020 •

edited

rdigiorgio commented Feb 7, 2020

simster7 commented Feb 28, 2020

CLAassistant commented Mar 17, 2020

simster7 commented Mar 18, 2020

salanki commented Mar 29, 2020

simster7 commented Mar 31, 2020

rdigiorgio commented Mar 31, 2020

simster7 commented Mar 31, 2020

sonarcloud bot commented Apr 14, 2020

	log.Info(cmd.Args)
	log.Debug("Exit code docker command args", cmd.Args)

feat: Add exit code as output variable #2111

feat: Add exit code as output variable #2111

Conversation

rdigiorgio commented Jan 31, 2020 • edited

simster7 left a comment

Choose a reason for hiding this comment

simster7 Jan 31, 2020

Choose a reason for hiding this comment

rdigiorgio Feb 1, 2020

Choose a reason for hiding this comment

simster7 Feb 3, 2020

Choose a reason for hiding this comment

simster7 commented Jan 31, 2020

rdigiorgio commented Feb 3, 2020 • edited

simster7 commented Feb 3, 2020

codecov bot commented Feb 3, 2020 • edited

Codecov Report

rdigiorgio commented Feb 7, 2020

simster7 commented Feb 28, 2020

CLAassistant commented Mar 17, 2020

simster7 commented Mar 18, 2020

salanki commented Mar 29, 2020

simster7 commented Mar 31, 2020

rdigiorgio commented Mar 31, 2020

simster7 commented Mar 31, 2020

sonarcloud bot commented Apr 14, 2020

rdigiorgio commented Jan 31, 2020 •

edited

rdigiorgio commented Feb 3, 2020 •

edited

codecov bot commented Feb 3, 2020 •

edited