Add ContainerState and ContainerName for Sidecars

[danielhelfand]

Closes #2074

Changes

Aligns the SidecarState with StepState to make the ContainerState and ContainerName available on TaskRun statuses for sidecars. I can add documentation as well under TaskRun Status if there would be a good place for it.

Submitter Checklist

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Add ContainerState and ContainerName to SidecarState to make sidecar statuses available on TaskRun status

[bobcatfish]

This looks great @danielhelfand thanks for adding this!

I'm really glad to see the new unit tests, could we add another level of test as well? A couple options:

• A test at the reconciler level, which behaves like a unit test but is in a lot of ways an integration test since it brings so many components together
• An end to end test <-- i'm guessing (hoping?) there is an existing end to end side car test that could be updated to check for this status info

[danielhelfand]

• A test at the reconciler level, which behaves like a unit test but is in a lot of ways an integration test since it brings so many components together

Looking into TaskRuns at the reconciler level, I don't see any testing for sidecars, so this might be a good chance to add some testing there.

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	33.3%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[danielhelfand]

/hold

Unfortunately, there are issues related to #1347 with this approach. If a sidecar runs longer than all the Steps, the ContainerStatus is left as running for the SidecarStatus on the TaskRunStatus. I'll need to look into how to update the ContainerStatus for Sidecars in this situation.

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[danielhelfand]

The integration test failures are all similar to as follows:

I0222 17:12:59.655]       imageID: docker-pullable://tianon/true@sha256:009cce421096698832595ce039aa13fa44327d96beedb84282a69d3dbcf5a81b
I0222 17:12:59.655]       name: slow-sidecar
I0222 17:12:59.655]       terminated:
I0222 17:12:59.655]         containerID: docker://2f3f434d4a4986faa4145af3434e694aa24a48b030f6e514da5e209648de027e
I0222 17:12:59.655]         exitCode: 127
I0222 17:12:59.655]         finishedAt: "2020-02-22T17:05:00Z"
I0222 17:12:59.656]         message: 'OCI runtime create failed: container_linux.go:345: starting container
I0222 17:12:59.656]           process caused "exec: \"sleep\": executable file not found in $PATH": unknown'
I0222 17:12:59.658]         reason: ContainerCannotRun
I0222 17:12:59.658]         startedAt: "2020-02-22T17:05:00Z"

My guess is that showing this ContainerStatus information now when a Sidecar has been stopped using the nop-image is showing the failure of trying to run sleep or any command not on the nop-image.

What could be done, in order to avoid this is, is to set a Terminated status for the SidecarStatus that explains the Sidecar was stopped successfully using the nop-image.

[danielhelfand]

Instead of using current State (s.State) from ContainerStatus on Pod with stopped Sidecars, what could be done is to create a Terminated Status with the following information:

corev1.ContainerState{
    Terminated: &corev1.ContainerStateTerminated{
        ExitCode: 0,
	Reason:   "Completed",
        Message:  "Sidecar container successfully stopped by nop-image",
        StartedAt: <Use value from LastTerminationState instead of current State (s.LastTerminatedState.Terminated.StartedAt)>,
        FinishedAt: <Use value from LastTerminationState instead of current State (s.LastTerminatedState.Terminated.FinishedAt)>,
        ContainerID: <Use value from LastTerminationState instead of current State (s.LastTerminatedState.Terminated.ContainerID)>,
}

Using the LastTerminationStatus, if exit code == 137, it can be assumed that the sidecar was killed.

Using the LastTerminationStatus would also provide more accurate information about the Start/End time of the Sidecar as well as information about the actual image that was used for the Sidecar.

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/sidecar.go	Do not exist	66.7%
test/builder/task.go	81.9%	81.5%	-0.3

[danielhelfand]

/hold cancel

Should be ready for another look

[vdemeester]

Looks good to me
/cc @chmouel

[vdemeester]

/lgtm

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [sbwsg,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment