Only create & mount Downward API volume when necessary

[hWorblehat]

Changes

Addresses #4937

• Instead of always mounting a Downward API volume for reading the "ready" annotation on task Pods, we only create it if the pod will not be "immediately ready" (i.e. if it has sidecars).
• Add new await-sidecar-readiness feature flag, defaulted to true. Tasks will always be considered "immediately ready" if this is false.

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• Docs included if any changes are user facing
• Tests included if any functionality added or changed
• Follows the commit message standard
• Meets the Tekton contributor standards (including
  functionality, content, code)
• Release notes block below has been filled in
  (if there are no user facing changes, use release note "NONE")

Release Notes

Added an `await-sidecar-readiness` feature flag, which can be used to remove the of DownwardAPI volumes in TaskRun pods. (#4953, @hWorblehat)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: hWorblehat / name: Rowan Lonsdale (d7fb7e3)

[tekton-robot]

Hi @hWorblehat. 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.

[pritidesai]

/ok-to-test

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint.go	88.3%	88.8%	0.4
pkg/pod/pod.go	88.3%	88.6%	0.3

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint.go	88.3%	88.8%	0.4
pkg/pod/pod.go	88.3%	88.6%	0.3

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/pod/entrypoint.go	88.3%	88.8%	0.4
pkg/pod/pod.go	88.3%	88.4%	0.1

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/pod/entrypoint.go	88.3%	88.8%	0.4
pkg/pod/pod.go	88.3%	88.4%	0.1

[hWorblehat]

I think this is ready for an initial review now. Do I need to assign someone particular for that?

[hWorblehat]

/assign @imjasonh

Sorry to bug - is there anything else I need to do to prompt a review here, or is it just a case of waiting until people have some free time?

[hWorblehat]

/unassign @imjasonh
/cc @imjasonh

[lbernick]

hi @hWorblehat would you mind squashing into one commit please? (a bit more detail about that in the tekton commit message standards)

[hWorblehat]

Hi @lbernick,

Sorry for slow response, I've been off work this last week.

I'd seen the commit message standards, and had thought it made sense to leave the changes as 2 commits as they cover logically distinct behaviours:

1. Not creating/mounting the downwardAPI volume when it's known not to be needed.
2. Providing a feature flag to disable waiting for sidecars (therefore making the downwardAPI volume unneeded in all scenarios)

Still, I'm happy to squash this all into one commit if you'd prefer.

[lbernick]

I wonder if it would make sense to add a warning if someone has "await-sidecar-readiness=false" but they are using sidecars. If someone enables this they probably have a good reason but would still be nice to have. Can probably be tracked in a separate issue

[lbernick]

I'd seen the commit message standards, and had thought it made sense to leave the changes as 2 commits as they cover logically distinct behaviours

Possibly our commit message standards could use some improvement :) We typically prefer one commit per PR unless there's a strong reason not to. If you wouldn't mind squashing that would be great, LGTM other than that.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 [vdemeester]

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

[hWorblehat]

Hi @lbernick,

Squashed as requested. I've also rejigged the isPodReadyImmediately function as requested, and added a line of logging.

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	83.0%	81.8%	-1.2
pkg/pod/entrypoint.go	88.3%	88.8%	0.4
pkg/pod/pod.go	88.3%	88.8%	0.5

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	83.0%	81.8%	-1.2
pkg/pod/entrypoint.go	88.3%	88.8%	0.4
pkg/pod/pod.go	88.3%	88.8%	0.5

[lbernick]

awesome thanks!

/lgtm

[vdemeester]

/retest

[hWorblehat]

/retest