Proposal: Phase out TaskRun's .status.podName

[imjasonh]

Background

Today, a TaskRun identifies the name of its underlying Pod primarily using .status.podName field. This is used in the TaskRun controller to determine whether a Pod has already been created for a given TaskRun, and by the CLI (and probably the dashboard) to get logs from the Pod.

The Pod that gets created also gets an OwnerReference pointing to the TaskRun that owns it (that it was created from). Owner References are largely used to identify resources that can be garbage collected (e.g., when a TaskRun is deleted, also delete its Pod).

Proposal

Instead of storing the TaskRun<->Pod relationship on both objects, only store the OwnerReference on the Pod. In the TaskRun controller, we can use the PodsLister to list Pods owned by this TaskRun (of which there should be at most one). The CLI and dashboard can also perform this lookup when printing logs or other Pod details (resource utilization, etc.).

One benefit of this is that it removes a Kubernetesism from the Tekton API -- TaskRuns might not always be executed using Pods, and encouraging a reliance on .status.podName will make that change harder to adapt to in the future.

Other Kubernetesisms (e.g., Step States, embedding the corev1.Container type) can be phased out alongside .status.podName, to make the Tekton API more execution-platform-agnostic.

[bobcatfish]

I like the idea of phasing out kubernetesisms but also I use the pod name super frequently when trying to debug TasksRuns, is there a shorthand I can use to get the pod from the TaskRun if we remove this?

[imjasonh]

If tkn learns to use the OwnerReference instead, tkn tr logs should just keep working. Maybe tkn tr pod is in order?

There's probably some gross bash one-liner that'd work instead of the gross bash one-liner I use today (kubectl logs $(kubectl get taskrun <foo> -ojsonpath={.status.podName}))

[vdemeester]

/area api
/kind feature
/kind api-change

[ghost]

Was this fixed by #1709? Is it safe to close?

[bobcatfish]

I think so! I know I've stopped using relying on it at least XD

[vdemeester]

I am pretty sure it's not, following #1709 we had #1971 and #1975

/cc @imjasonh because I don't think we re-re-verted the change ?

Also, the field is still their and maybe relied on (I know OpenShift Devconsole still relies on it for now).

[AlanGreene]

We're also using it in dashboard to construct the API for logs. If there's another approach we can take to do this (ideally without having to list pods to find a match) we could switch.

[vdemeester]

We're also using it in dashboard to construct the API for logs. If there's another approach we can take to do this (ideally without having to list pods to find a match) we could switch.

Using tekton.dev/* labels on pods 👼

[AlanGreene]

Using tekton.dev/* labels on pods 👼

That's exactly what I was hoping to avoid since it's another resource type we need to request and process rather than just using a field that's already available to us in the TaskRun. We can make it work though if the podName field is going away.

[imjasonh]

It's not going away if people strongly depend on it. 😅

I guess this was more of a discussion/what-if, so if there's enough pushback I'm happy to reconsider.

[vdemeester]

It's not going away if people strongly depend on it. sweat_smile

I guess this was more of a discussion/what-if, so if there's enough pushback I'm happy to reconsider.

I am not pushing back 😝 I was just saying that it's still there an some tool are still using it. We can still remove it (I wouldn't mind tbh), but we will need to re-follow the deprecation policy (and hopefully make sure we don't break ourselves 😝 )

[afrittoli]

Along with this, I think we should add a new condition to the TaskRun - i.e. Running - so that at reconcile level we can know that we should pull updates from the underlying resource (i.e. pod).

[josephlewis42]

I think it's worth keeping the podName on the status for two reasons:

• The performance penalty of removing it and expecting a client to reach to the underlying resource to get logs is high. In my use-case this could require listing hundreds or thousands of Pods. This is especially true if the other K8s-isms are going to be removed in which case debugging will mean going to the Pod to investigate.
• As mentioned in the proposal, OwnerReferences are largely used for GC. It's an assumption that there will never be more than one Pod that matches any criteria you put out. It's handy to be able to attach a resource to another one knowing it'll be GC'd when the owner is deleted.

If the Tekton API needs to be portable, you could use a TypedLocalObjectReference instead. That would allow the specificity of a pointer without tying it to any particular runtime.

[bobcatfish]

@afrittoli mentioned custom tasks in this context as well (#215) - if we want to customtaskruns to have a similar structure to taskruns, maybe we dont want podName in the status?

[afrittoli]

I will remove the label for now, until we have an agreement on it.
@imjasonh perhaps would you like to bring it back to the API WG for discussion?

[tekton-robot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[tekton-robot]

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

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.

[vdemeester]

/remove-lifecycle rotten
/remove-lifecycle stale
/reopen

[tekton-robot]

@vdemeester: Reopened this issue.

In response to this:

/remove-lifecycle rotten
/remove-lifecycle stale
/reopen

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]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[vdemeester]

/remove-lifecycle stale
/lifecycle frozen

[lbernick]

Discussed in today's API WG: having the pod name in the TaskRun status is useful for retrieving logs and for dashboard/cli. Longer term better solution is to leverage the results API, to provide a way for people to retrieve statuses and logs of TaskRuns. Once we have something like this in place, we will revisit removing this field from the TaskRun API.

/close

[tekton-robot]

@lbernick: Closing this issue.

In response to this:

Discussed in today's API WG: having the pod name in the TaskRun status is useful for retrieving logs and for dashboard/cli. Longer term better solution is to leverage the results API, to provide a way for people to retrieve statuses and logs of TaskRuns. Once we have something like this in place, we will revisit removing this field from the TaskRun API.

/close

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.