Don't rely on .status.podName to find Pod associated with a TaskRun #1709
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/XL
|
This will need a rebase @imjasonh |
|
Should we include a deprecation notice for this? I guess the plan is to drop .status.podName eventually? |
This change doesn't directly impact external users, it just changes how the controller identifies a TaskRun's pod. If this is not a regression (performance or behavior) we can switch the CLI and dashboard to use this method too. At that point we can decide whether to (sloooowly) deprecate There isn't a lot of directly applicable benefit from doing this, at least not right now. In general the CLI/dashboard shouldn't have to rely on the fact of a Pod existing to stream logs, that's all. But this change by itself should be invisible to end users or downstream clients. |
imjasonh
force-pushed
the
podname
branch
2 times, most recently
from
edbf13a to
da288e0
Compare
|
@imjasonh shall I fix unit tests for it? Looks like metrics unit tests giving some trouble here. |
|
Yes, @imjasonh will reduce the that spamming as well. (really my bad, thank you for being patient on this 🙏) |
imjasonh
force-pushed
the
podname
branch
2 times, most recently
from
7d36f54 to
efd0f0e
Compare
|
I haven't noticed any serious performance regression, but under load or with millions of pods to query, it's possible it will be more noticeable. |
|
In response to this:
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. |
tekton-robot
added
the
approved
This adds Reconciler.getPod, which looks up the Pod for a TaskRun by performing a label selector query on Pods, looking for the label we apply to Pods generated by TaskRuns. If zero Pods are returned, it's the same as .status.podName being "". If multiple Pods are returned, that's an error. Also, clean up metrics_test.go a bit while I'm in that area
|
/test pull-tekton-pipeline-integration-tests |
|
[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:
Approvers can indicate their approval by writing |
ghost
mentioned this pull request
This adds Reconciler.getPod, which looks up the Pod for a TaskRun by
performing a label selector query on Pods, looking for the label we
apply to Pods generated by TaskRuns.
If zero Pods are returned, it's the same as .status.podName being "". If
multiple Pods are returned, that's an error.
Also, clean up metrics_test.go a bit while I'm in that area.
This moves us toward clients of Tekton querying Pods owned by TaskRuns, instead of the ownership information being "owned" by the TaskRun. Eventually, someday, TaskRuns might not have Pods at all, and internal code and clients not relying on this behavior will make that easier to
/hold work-in-progress
#1689
cc @vdemeester @hrishin
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe 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