Changes

This commit adds a v1beta1 version of the CustomRun CRD, and support to the webhook.

Fixes issue #5154 Fixes issue #5153 Fixes issue #5532

/cc @jerop /kind misc

Submitter Checklist

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

n/a Has Docs included if any changes are user facing
n/a Has Tests included if any functionality added or changed
[x] Follows the commit message standard
[x] Meets the Tekton contributor standards (including functionality, content, code)
[x] Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
[x] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
n/a Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

Sep 12 '22 16:09 vsinghai

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

Sep 12 '22 16:09 tekton-robot

do I need conversions, like stated here, for v1alpha1.Runs to v1beta1.CustomRuns?

Sep 12 '22 18:09 vsinghai

/assign

Sep 12 '22 19:09 XinruZhang

do I need conversions, like stated here, for v1alpha1.Runs to v1beta1.CustomRuns?

From the API compatibility policy perspective, it's okay that we don't convert it (per Rule #2 in K8s API Deprecation Policy) (meaning we deprecate the entire alpha track of the Custom Task Run).

The reason that PipelineRun needs that conversion is we want to support both v1beta1 and v1 version at the same time.

Sep 12 '22 19:09 XinruZhang

/ok-to-test

Sep 13 '22 14:09 dibyom

do I need conversions, like stated here, for v1alpha1.Runs to v1beta1.CustomRuns?

From the API compatibility policy perspective, it's okay that we don't convert it (per Rule #2 in K8s API Deprecation Policy) (meaning we deprecate the entire alpha track of the Custom Task Run).

The reason that PipelineRun needs that conversion is we want to support both v1beta1 and v1 version at the same time.

I think we would ideally like to have conversion here because it will allow us to support v1alpha1 Runs and v1beta1 CustomRuns at the same time. Technically it's not necessary since v1alpha1 Run is in alpha but it feels a bit user hostile to drop support for Runs at the same time we're adding support for CustomRuns. It will force people to copy all their Run definitions somewhere, delete them, upgrade tekton pipelines, and then recreate their old run definitions as CustomRuns. (This would also be the case if we rename the "Kind" of the existing CRD to "CustomRun".)

Unfortunately I'm not sure if there's an easy way to rename a CRD/implement conversion between two different "Kinds". One thing we could try is to add a v1beta1 version of Run that deserializes to the CustomRun go struct, in addition to adding a CustomRun CRD with a v1beta1 version, and probably deprecate the Run CRD after giving people a release or two to convert to the new CRD. (I'm not totally sure if this would work)

Another idea would be to have the new CRD and update the pipelinerun reconciler code to allow reconciling both v1alpha1 Runs and v1beta1 custom runs, and not implement conversion. We would probably want to remove support for v1alpha1 runs after a release or two. I'm fairly sure this would work.

happy to help here but want to hear others' thoughts first -- @tektoncd/core-maintainers is this making sense/do you know how to rename a CRD?

Sep 13 '22 15:09 lbernick

I've never actually seen anyone rename a CRD - I'm pretty sure you can't convert between two different Kinds, yeah. Some research is definitely needed here.

Sep 13 '22 20:09 abayer

I would say the main focus is how do we want our users to conceive the "alpha" feature.

IMHO alpha version track is designed to serve as experimental features, meaning we really don't want our users to rely on alpha features in their prod env. They are only suppose to use it for experiments and they should expect the alpha features can be different, backward incompatible across two adjacent releases.

However, IIUC, this "alpha" feature has existed for a long time and some users have already heavily relied on it (have they?), we can't ignore this real life situation, in this case, I strongly recommend we need to support both for a while, giving them enough buffer time to migrate.

But in order to form a formal standard around stability of each version track, I would suggest it might be better if we explicitly tell users that supporting both alpha and beta versions this time is an exception, this decision is not based on the guidance we'd like to follow, but based on the reality that we haven't been actively promoting alpha features to beta in the past. While we are planning to do it in the future.

Sep 13 '22 20:09 XinruZhang

Thanks for the great conversation @lbernick @XinruZhang @abayer . I would like to ask a couple of questions regarding this situation. To my understanding, v1alpha1.Runs and v1beta1.CustomRuns, in theory, should be mutually exclusive. Therefore, I am not clear as to why we should add the conversion, when we should rather push the user to start using CustomRuns while still providing support for Runs (for a limited amount of time). In the end, the whole point of the K8 version control is to deprecate the less stable versions, i.e. v1alpha1 (where Runs is stored), and keep upgrading to stable versions. If we are trying to upgrade Runs to CustomRuns from v1alpha to v1beta1, but still allow the users to be able to use the v1alpha1 feature, doesn't that negate the whole point of constantly trying to upgrade our K8 versioning? I understand @lbernick point that it would be harsh to the user to suddenly drop Runs out of the blue and force everyone to update their code, but would a slow push of CustomRuns paired with telling users that Runs will no longer be supported help solve that issue?

Just providing some food for thought here, let me know what you think!

Sep 14 '22 14:09 vsinghai

Thanks for the great conversation @lbernick @XinruZhang @abayer . I would like to ask a couple of questions regarding this situation. To my understanding, v1alpha1.Runs and v1beta1.CustomRuns, in theory, should be mutually exclusive. Therefore, I am not clear as to why we should add the conversion, when we should rather push the user to start using CustomRuns while still providing support for Runs (for a limited amount of time).

The main purpose of implementing conversion is so that we only have to support one golang version in our reconciler code, because each served version (version that users can use) gets converted into the same stored version (version stored in etcd and used by our reconciler code). If we aren't able to implement conversion, our reconciler code will have to support both v1alpha1 Run and v1beta1 CustomRun. Contrast this to PipelineRun: our stored version is v1beta1, but once we start serving v1, it will be converted into a v1beta1 PipelineRun for use in our reconciler, so the reconciler code only needs to handle one version at a time.

I agree we'd like to ask users to swap to customruns while still supporting runs for a short window.

In the end, the whole point of the K8 version control is to deprecate the less stable versions, i.e. v1alpha1 (where Runs is stored), and keep upgrading to stable versions. If we are trying to upgrade Runs to CustomRuns from v1alpha to v1beta1, but still allow the users to be able to use the v1alpha1 feature, doesn't that negate the whole point of constantly trying to upgrade our K8 versioning? I understand @lbernick point that it would be harsh to the user to suddenly drop Runs out of the blue and force everyone to update their code, but would a slow push of CustomRuns paired with telling users that Runs will no longer be supported help solve that issue?

I agree we'd like to remove support for Runs (I'm not sure exactly what you mean by a "slow push" here). The question is just how best to create a release where we support both Runs and CustomRuns, before creating a release where we remove support for Runs.

Sep 14 '22 15:09 lbernick

/cc @dibyom

please review

Sep 14 '22 16:09 vsinghai

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop

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~~ [jerop]

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

Sep 14 '22 16:09 tekton-robot

I'm not sure this PR should go in yet-- we're exposing a CRD that doesn't actually do anything when you create it

Sep 14 '22 16:09 lbernick

/hold

Sep 15 '22 08:09 vdemeester

Here is a doc with the roadmap for this PR. The doc summarizes the lengthy conversation we have had in this PR and what the team has agreed upon for next steps for the PR.

Once again, thank you @lbernick @XinruZhang and @abayer for the great discussion!

Sep 15 '22 21:09 vsinghai