Sidecars doesn't get terminated when the binary is in the nop image #1347

chmouel · 2019-09-23T16:38:24Z

Expected Behavior

Sidecars get terminated along the main container

Actual Behavior

This is a followup to the discussion we had with @sbwsg on this issue :

Since the sidecar tests has been implemented we have seen some issues on our openshift based CI. The test would run waiting for a terminate state and fails waiting. Here is the test :

https://github.com/chmouel/tektoncd-pipeline/blob/chmouel-ci-test-1809/test/sidecar_test.go#L105-L107

We believe that we only just figured this out, It seems that it is because of the base image we are using that are based on a RHEL image called registry.access.redhat.com/ubi8/ubi:latest.

With KO the nop image is by default based according to https://github.com/google/ko#overriding-the-default-base-image on gcr.io/distroless/base:latestwhich has no /bin/sh while the RHEL image has.

We are guessing of what's happening is :

a main container runs with a sidecar container
the main and sidecar containers are both doing /bin/sh scripts like this :
https://github.com/chmouel/tektoncd-pipeline/blob/chmouel-ci-test-1809/test/sidecar_test.go#L49-L50
the main container gets killed, tekton controller sees that there is a sidecar container and replaces the main image name with a nop image and keep the same arguments.
If the nop container is able to run those arguments then it continue running instead of getting to killed state as it should be.

Steps to Reproduce the Problem

override the base image for nop with a container that has /bin/sh

diff --git a/.ko.yaml b/.ko.yaml
index 9b34cc27..27524d01 100644
--- a/.ko.yaml
+++ b/.ko.yaml
@@ -1,4 +1,5 @@
 baseImageOverrides:
+  github.com/tektoncd/pipeline/cmd/nop: google/cloud-sdk:alpine
   # TODO(christiewilson): Use our built base image
   github.com/tektoncd/pipeline/cmd/creds-init: gcr.io/knative-nightly/github.com/knative/build/build-base:latest
   github.com/tektoncd/pipeline/cmd/git-init: gcr.io/knative-nightly/github.com/knative/build/build-base:latest

Run the TestSideCar test :

% go test -failfast -v -count=1 -tags=e2e -ldflags '-X github.com/tektoncd/pipeline/test.missingKoFatal=false' ./test -timeout=20m --kubeconfig $KUBECONFIG -run TestSidecarTaskSupport

Additional Info

We probably want to figure why rewriting the Entrypoint is not possible.

/kind bug
/cc @sbwsg

The text was updated successfully, but these errors were encountered:

This is a very cheeky hack, sidecar is currently broken with our nop image so we just use nightly `nop` from upstream CI. `nop` should not change or do anything differently with a different base so we should be safe until tektoncd#1347 gets fixed. Signed-off-by: Chmouel Boudjnah <[email protected]>

chmouel · 2019-09-24T10:27:59Z

Related Kubernetes issue for a RFE allowing spec.Container.Command to be modified kubernetes/kubernetes#83059

ghost · 2019-09-24T12:32:02Z

Ah, great work figuring this out @chmouel! I'm wondering - what is the reason for overriding the nop image?

ghost · 2019-09-24T12:32:57Z

Oh wait, nevermind, I see that https://github.com/google/ko#overriding-the-default-base-image describes some reasons to do so.

chmouel · 2019-09-24T13:19:05Z

Our case is a bit different,

As a policy (and our CI enforces it) all our images needs to use our official distro so we have to base the nop image against a minimal RHEL container (called UBI).

ghost · 2019-09-24T13:42:01Z

Does UBI contain a kill binary? If so then this issue may be resolved by #1131 since, once implemented, that would no longer perform a nop image swap (and at the same time would introduce a nice simple contract for sidecar containers that want to be stopped "gracefully").

ghost · 2019-09-24T13:43:30Z

There is also the ongoing sidecar KEP which is progressively being implemented in Kubernetes. kubernetes/enhancements#753 (and which Tekton may eventually use "under the hood" to run the sidecars).

chmouel · 2019-09-24T14:19:35Z

@sbwsg ah really nice yes we do have a kill binary :

but if I understand your comment here #1131 (comment) you want to SIGKILL process 1 in the sidecar container before it gets replaced to nop. That sidecar container is whatever the user choose to be, we are not enforcing our base images on this one, it's only on the images we ship from tekton that we have a RHEL base.

So what I am trying to say is that if we come down to the bullet point number 3. from your comment where we endup doing the swap this would end up the same as what we have here.

Having said that I don't see any alternative and if we implement #1131 things would def be better,

I am still wondering why k8 allows us to change the image name and not the entrypoints,

kubernetes/enhancements#753 is definitively the way to go, glad to see something like this being worked on.

ghost · 2019-09-24T14:21:30Z

it's only on the images we ship from tekton that we have a RHEL base.

Ah yeah good point!

ghost · 2019-10-24T14:54:54Z

At least in the short term i think we should document this. I will do this today.

ghost · 2019-10-24T16:04:19Z

#1464

Sidecars are stopped by having their Image field swapped out to the `nop` image. When the nop image starts up in the sidecar container it is supposed to immediately exit because `nop` doesn't include the sidecar's command. However, when the `nop` image *does* contain the command that the sidecar is running, the sidecar container will actually never stop and the Task will eventually timeout. For most sidecars this issue will not manifest - the `nop` container that Tekton provides out of the box includes only a very limited set of commands. However, if a Tekton operator overrides the `nop` image when deploying the tekton controller (for example, because their organization requires images configured for Tekton to be built on their org's own base image) then there is a risk that `nop` will start offering more commands and therefore introduce a higher risk that a sidecar's command will be runnable by the `nop` image finally increasing the likelihood of Tasks with sidecars running until timeout. This issue is a known bug with the way sidecars operate at the moment and is being tracked in #1347 but should be documented clearly.

ghost · 2020-04-27T14:52:56Z

Now that this is documented I'm going to close this issue.

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 23, 2019

vdemeester added this to the Pipelines 0.8 🐱 milestone Sep 23, 2019

chmouel changed the title ~~SideCarding seems broken~~ Sidecars doesn't get terminated when the binary is in the nop image Sep 24, 2019

chmouel mentioned this issue Sep 24, 2019

Use GCR distroless for tektoncd nop openshift/release#5158

Closed

This was referenced Sep 24, 2019

CI Test openshift/tektoncd-pipeline#134

Closed

Use upstream nop image to fix sidecar tests. openshift/tektoncd-pipeline#136

Merged

Allow updating the Spec.Container.Command kubernetes/kubernetes#83059

Closed

This was referenced Sep 25, 2019

Update base image of nop Dockerfile openshift/tektoncd-pipeline#135

Closed

PR to test CI openshift/tektoncd-pipeline#133

Closed

afrittoli modified the milestones: Pipelines 0.8 🐱, Pipelines 0.9 🐱 Oct 22, 2019

ghost mentioned this issue Oct 24, 2019

Document bug with sidecar usage of nop image #1464

Merged

2 tasks

bobcatfish modified the milestones: Pipelines 0.9 🐱, Pipelines 1.1 / Post-beta 🐱 Oct 30, 2019

danielhelfand mentioned this issue Feb 22, 2020

Add ContainerState and ContainerName for Sidecars #2075

Merged

3 tasks

ghost closed this as completed Apr 27, 2020

bahetiamit mentioned this issue Oct 9, 2020

TestSidecarTaskSupport e2e test case fix for custom base image #3358

Closed

4 tasks

Tomcli mentioned this issue Mar 24, 2022

Tekton results with sidecar/post-processing step investigation kubeflow/kfp-tekton#878

Open

This issue was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sidecars doesn't get terminated when the binary is in the nop image #1347

Sidecars doesn't get terminated when the binary is in the nop image #1347

chmouel commented Sep 23, 2019 •

edited

Loading

chmouel commented Sep 24, 2019

ghost commented Sep 24, 2019

ghost commented Sep 24, 2019

chmouel commented Sep 24, 2019 •

edited

Loading

ghost commented Sep 24, 2019

ghost commented Sep 24, 2019 •

edited by ghost

Loading

chmouel commented Sep 24, 2019 •

edited

Loading

ghost commented Sep 24, 2019 •

edited by ghost

Loading

ghost commented Oct 24, 2019

ghost commented Oct 24, 2019

ghost commented Apr 27, 2020

Sidecars doesn't get terminated when the binary is in the nop image #1347

Sidecars doesn't get terminated when the binary is in the nop image #1347

Comments

chmouel commented Sep 23, 2019 • edited Loading

Expected Behavior

Actual Behavior

Steps to Reproduce the Problem

Additional Info

chmouel commented Sep 24, 2019

ghost commented Sep 24, 2019

ghost commented Sep 24, 2019

chmouel commented Sep 24, 2019 • edited Loading

ghost commented Sep 24, 2019

ghost commented Sep 24, 2019 • edited by ghost Loading

chmouel commented Sep 24, 2019 • edited Loading

ghost commented Sep 24, 2019 • edited by ghost Loading

ghost commented Oct 24, 2019

ghost commented Oct 24, 2019

ghost commented Apr 27, 2020

chmouel commented Sep 23, 2019 •

edited

Loading

chmouel commented Sep 24, 2019 •

edited

Loading

ghost commented Sep 24, 2019 •

edited by ghost

Loading

chmouel commented Sep 24, 2019 •

edited

Loading

ghost commented Sep 24, 2019 •

edited by ghost

Loading