Enable sidecars to partake in substitution #1770

MLBMatt · 2019-12-19T15:01:14Z

This change enables sidecars to use variable subtitution and leverage
pipeline-specific params to execute whatever actions during a
pipelinerun.

Fixes #1761

Changes

This change enables sidecars to use variable subtitution and leverage
pipeline-specific params to execute whatever actions during a
pipelinerun.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

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

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

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

Task sidecars now partake in variable substitution.

googlebot · 2019-12-19T15:01:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

tekton-robot · 2019-12-19T15:01:30Z

Hi @MLBMatt. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

MLBMatt · 2019-12-19T15:06:26Z

@googlebot I signed it!

googlebot · 2019-12-19T15:06:29Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

imjasonh · 2019-12-19T15:07:22Z

/ok-to-test

Thanks for this! Can you add a YAML example that demonstrates this works? This serves as a runnable sample, and as a test that this continues to work as expected.

Also, please provide release notes for this user visible change.

MLBMatt · 2019-12-19T18:04:43Z

@imjasonh Added a yaml example to the docs, added a bit more implementation, updated more to the tests and added release notes.

MLBMatt · 2019-12-19T20:48:36Z

/retest

imjasonh · 2019-12-20T15:25:01Z

@imjasonh Added a yaml example to the docs, added a bit more implementation, updated more to the tests and added release notes.

Thanks for adding the example to the docs. If you add a working example to examples/taskrun/ we'll make sure the run succeeds during presubmit tests.

Something like sidecar-ready.yaml:

spec:
  taskSpec:
    inputs:
      params:
      - name: value
        default: default-value
    sidecars:
    - name: slow-sidecar
      image: ubuntu
      command:
      - sh
      - -c
      - echo $(inputs.params.value) > /shared/value && sleep infinity
      volumeMounts:
      - name: shared
        mountPath: /shared

    steps:
    - image: ubuntu
      # The step will only succeed if the sidecar resolved the param value.
      script: |
        #!/usr/bin/env bash
        VALUE=$(cat /shared/value)
        [[ $VALUE == $(inputs.params.value) ]]
      volumeMounts:
      - name: shared
        mountPath: /shared

(I haven't tested this, but something like it should work, and prove that params are resolved in the sidecar)

MLBMatt · 2019-12-20T17:55:11Z

@imjasonh I added an example, however I couldn't get it working locally because of, what looks like, the noop bug associated with sidecars. I couldn't get your sidecar-ready.yaml example working either, so I'm not sure if it's me or tekton.

ghost · 2019-12-20T19:12:49Z

@MLBMatt Can you describe a bit the problems you encountered? I was able to get your example working with the code on your branch but I'd like to understand the issue you're seeing before giving this a 👍

MLBMatt · 2019-12-20T19:15:50Z

@sbwsg I was defining a command for my sidecar however it looked like the sleep was never running correctly, was being replaced by the tianon/true image and complaining about not finding shell to execute.

I've seen that, in order for the sidecar to be killed correctly, the sidecar is modified to exit cleanly but for me, this looked to still be broken on my end? Maybe I don't have the latest controller version?

ghost · 2019-12-20T19:22:15Z

Ah ok great that's actually expected behaviour! In order to stop sidecars when the task steps finish we have to replace the image of the sidecar's container. Bit wonky but it (mostly) works until k8s supports sidecar lifecycle containers natively.

ghost · 2019-12-20T19:22:39Z

(Just to confirm - did the TaskRun end with a successful status?)

MLBMatt · 2019-12-20T19:27:45Z

@sbwsg Ah ok, yes the task container ended in a Completed state and looks all good. I think things were just getting muddled a bit.

edit: Well the pod itself ends in a ContainerCannotRun state, caused by trying to run /bin/sh on the tianon/true image. I'm not sure if Tekton evaluates this still to a successful run or not.

MLBMatt · 2019-12-20T19:31:10Z

I get a status Failed on the actual taskrun pod because:

Error: failed to start container "sidecar-value-sidecar": Error response from daemon: OCI runtime create failed: container_linux.go:345: starting container process caused "exec: \"/bin/bash\": stat /bin/bash: no such file or directory": unknown

This from the replaced tianon/true image trying to run the previous value-sidecar's command/args.

ghost · 2019-12-20T19:38:04Z

Tekton ignores the failure of the sidecar container. The TaskRun ends with a condition of Succeeded / True and the step-check-value Step has an exit code of 0 with reason Completed (at least on my machine). I think we're all good here in terms of this example.

Edit: sidecar pod -> sidecar container

MLBMatt · 2019-12-20T19:39:56Z

@sbwsg awesome! 🎉

ghost · 2019-12-20T19:42:23Z

/lgtm

imjasonh · 2019-12-22T14:46:00Z

pkg/apis/pipeline/v1alpha1/container_replacements.go

+	corev1 "k8s.io/api/core/v1"
+)
+
+func ApplyContainerReplacements(step *corev1.Container, stringReplacements map[string]string, arrayReplacements map[string][]string) {


Looks like this doesn't need to be exported, except to be tested in container_replacements_test.go.

Can we either test this as an unexported method, or only test the exported surface in apply_test.go?

@imjasonh it's also used in https://github.com/tektoncd/pipeline/pull/1770/files#diff-768a3668fda546db3140aa35963e1247R25, not sure if that still warrants not exporting (I'm super new to Go, so the import/export semantics elude me a bit right now)

Indeed, looks like this is used by the resources package as well and so needs to live on as exported for the time being 👍

examples/taskruns/sidecar-interp.yaml

ghost · 2020-01-10T17:18:21Z

@MLBMatt thanks for the PR, could you squash the commits, rebase on top of latest master, and ensure that the commit message continues to match the PR description you've included here (after all the squashing is done)?

I think otherwise this is looking ready to merge. Many thanks! 🙏

This change enables sidecars to use variable subtitution and leverage pipeline-specific params to execute whatever actions during a pipelinerun. Fixes tektoncd#1761

MLBMatt · 2020-01-10T17:24:06Z

@sbwsg squashed and rebased 👍

ghost · 2020-01-10T17:37:13Z

/approve

tekton-robot · 2020-01-10T17:37:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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]

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

ghost · 2020-01-10T17:52:11Z

/retest

ghost · 2020-01-10T18:45:58Z

/retest

ghost · 2020-01-10T20:18:53Z

/lgtm

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 19, 2019

tekton-robot requested review from afrittoli and dibyom December 19, 2019 15:01

googlebot added the cla: no label Dec 19, 2019

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 19, 2019

googlebot added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Dec 19, 2019

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 19, 2019

MLBMatt marked this pull request as ready for review December 19, 2019 18:03

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 19, 2019

tekton-robot assigned ghost Dec 20, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 20, 2019

imjasonh reviewed Dec 22, 2019

View reviewed changes

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 6, 2020

MLBMatt requested a review from imjasonh January 7, 2020 21:10

Enable sidecars to partake in substitution

6c8bf0a

This change enables sidecars to use variable subtitution and leverage pipeline-specific params to execute whatever actions during a pipelinerun. Fixes tektoncd#1761

MLBMatt force-pushed the master branch from 6242262 to 6c8bf0a Compare January 10, 2020 17:23

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2020

tekton-robot merged commit f157f33 into tektoncd:master Jan 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable sidecars to partake in substitution #1770

Enable sidecars to partake in substitution #1770

MLBMatt commented Dec 19, 2019 •

edited

Loading

googlebot commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

MLBMatt commented Dec 19, 2019

googlebot commented Dec 19, 2019

imjasonh commented Dec 19, 2019

MLBMatt commented Dec 19, 2019 •

edited

Loading

MLBMatt commented Dec 19, 2019

imjasonh commented Dec 20, 2019

MLBMatt commented Dec 20, 2019

ghost commented Dec 20, 2019

MLBMatt commented Dec 20, 2019 •

edited

Loading

ghost commented Dec 20, 2019

ghost commented Dec 20, 2019

MLBMatt commented Dec 20, 2019 •

edited

Loading

MLBMatt commented Dec 20, 2019 •

edited

Loading

ghost commented Dec 20, 2019 •

edited by ghost

Loading

MLBMatt commented Dec 20, 2019

ghost commented Dec 20, 2019

imjasonh Dec 22, 2019

MLBMatt Jan 6, 2020

ghost Jan 10, 2020

ghost commented Jan 10, 2020 •

edited by ghost

Loading

MLBMatt commented Jan 10, 2020

ghost commented Jan 10, 2020

tekton-robot commented Jan 10, 2020

ghost commented Jan 10, 2020

ghost commented Jan 10, 2020

ghost commented Jan 10, 2020

Enable sidecars to partake in substitution #1770

Enable sidecars to partake in substitution #1770

Conversation

MLBMatt commented Dec 19, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

googlebot commented Dec 19, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

tekton-robot commented Dec 19, 2019

MLBMatt commented Dec 19, 2019

googlebot commented Dec 19, 2019

imjasonh commented Dec 19, 2019

MLBMatt commented Dec 19, 2019 • edited Loading

MLBMatt commented Dec 19, 2019

imjasonh commented Dec 20, 2019

MLBMatt commented Dec 20, 2019

ghost commented Dec 20, 2019

MLBMatt commented Dec 20, 2019 • edited Loading

ghost commented Dec 20, 2019

ghost commented Dec 20, 2019

MLBMatt commented Dec 20, 2019 • edited Loading

MLBMatt commented Dec 20, 2019 • edited Loading

ghost commented Dec 20, 2019 • edited by ghost Loading

MLBMatt commented Dec 20, 2019

ghost commented Dec 20, 2019

imjasonh Dec 22, 2019

Choose a reason for hiding this comment

MLBMatt Jan 6, 2020

Choose a reason for hiding this comment

ghost Jan 10, 2020

Choose a reason for hiding this comment

ghost commented Jan 10, 2020 • edited by ghost Loading

MLBMatt commented Jan 10, 2020

ghost commented Jan 10, 2020

tekton-robot commented Jan 10, 2020

ghost commented Jan 10, 2020

ghost commented Jan 10, 2020

ghost commented Jan 10, 2020

MLBMatt commented Dec 19, 2019 •

edited

Loading

MLBMatt commented Dec 19, 2019 •

edited

Loading

MLBMatt commented Dec 20, 2019 •

edited

Loading

MLBMatt commented Dec 20, 2019 •

edited

Loading

MLBMatt commented Dec 20, 2019 •

edited

Loading

ghost commented Dec 20, 2019 •

edited by ghost

Loading

ghost commented Jan 10, 2020 •

edited by ghost

Loading