Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch vendor/ apimachinery to work on 1.22 #4164

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

vdemeester
Copy link
Member

@vdemeester vdemeester commented Aug 17, 2021

Changes

A new field is there in k8s 1.22, name subresource in the
managedfields. This proves to make knative based types to not be valid
on 1.22 and above, making tektoncd/pipeline as well as any other
component using knative/pkg to be broken on 1.22 and above.

Fixes #4158

Signed-off-by: Vincent Demeester [email protected]

/kind bug

Opening this PR to discuss what is the way forward, taking the following into account:

  • Any version of tektoncd/pipeline (or any other component using knative/pkg) won't work on kubernetes 1.22 and above (without this patch)
  • Adding this field will probably be backported to 1.21 and maybe even 1.22 apimachinery branches — so updating those deps would fix it, but the question is the timeframe
  • What should we do for previous releases ? (0.27, 0.26, …) A bigger question is, what is our support matrix for pipeline (should release X support the latest k8s released at that time or not ? …) ?
  • Assuming we would want this patch in (here or on release branches), what would be our "patch" management ?

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 or deleted (only if no user facing changes)

Release Notes

Backport adding Subresource field to  ManagedField entries in our `vendor/` folder to make tektoncd/pipeline work on k8s 1.22

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 17, 2021
@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 17, 2021
@tekton-robot
Copy link
Collaborator

@vdemeester: GitHub didn't allow me to request PR reviews from the following users: core.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @core

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
Copy link
Member Author

/cc @tektoncd/core-maintainers

@tekton-robot tekton-robot requested a review from a team August 17, 2021 07:49
@ghost
Copy link

ghost commented Aug 17, 2021

I'm tempted to suggest that we only patch 0.27 and 0.26 and maybe 0.25, declaring anything less as incompatible with Kubernetes 1.22+. Whatever we decide I'm happy to perform the patches on the older release branches since I'll be doing it for 0.27 anyway. We might also want to apply docs-only patches to the older releases so that users can be informed of the platform incompatibility?

What are you thinking with "patch management"? A shell script to apply the change with awk / sed? Or something totally different? I would've thought just having the change in our vendor tree would be enough but I might be misunderstanding our options here.

@vdemeester
Copy link
Member Author

I'm tempted to suggest that we only patch 0.27 and 0.26 and maybe 0.25, declaring anything less as incompatible with Kubernetes 1.22+. Whatever we decide I'm happy to perform the patches on the older release branches since I'll be doing it for 0.27 anyway. We might also want to apply docs-only patches to the older releases so that users can be informed of the platform incompatibility?

Right, I would have gone for 0.27 and 0.26 only initially, and document a larger set of others that there is an incompat. But there is one thing we need to figure out / document : "support" lifecycle, aka what is the lifecycle of a given release, when should we patch it, … I don't think we have this documented anywhere yet.

What are you thinking with "patch management"? A shell script to apply the change with awk / sed? Or something totally different? I would've thought just having the change in our vendor tree would be enough but I might be misunderstanding our options here.

Downstream (OpenShift Pipelines), we are using a folder (openshift/patches) and git format-patch and git am 🙃. Having the change only on the vendor folder makes it non-reproducible (or at least harder). Imaging we need to another fix in 0.27 with bumping a new dependency (fixing a CVE or something) ; if we don't have a patch to apply or something, there is no guarantee that our vendor patch that we did by hand well still be there.
The job pull-tekton-pipeline-build-tests fails for this exact reason today, I just made the change without making it reproducible when running ./hack/update* scripts.

@vdemeester vdemeester added this to the Pipelines v0.28 milestone Aug 23, 2021
@vdemeester
Copy link
Member Author

An alternative would be to do what knative did here, allowing unknown fields and get the validation through openapi schemas. But this needs main...vdemeester:schema-gen and as far as I tried, this only works with one version of CRD (trying this with multiple version and... it panics)

@ghost
Copy link

ghost commented Aug 23, 2021

Using git for the patching makes total sense to me given it's a tool already in our workflow.

/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2021
@ghost
Copy link

ghost commented Aug 23, 2021

ping @tektoncd/core-maintainers it would be good to get input from one other person before we move ahead with this. We'd like to release as part of 0.27.3 so PTAL.

@jerop jerop self-assigned this Aug 24, 2021
@vdemeester
Copy link
Member Author

I'll update the ./hack/update-deps.sh with this patch to apply and a warning that this is temporary, linking to an issue I'll create to track the resolution of this in k8s or knative/pkg (or naturally with dep bumps).

A new field is there in k8s 1.22, name subresource in the
managedfields. This proves to make knative based types to not be valid
on 1.22 and above, making tektoncd/pipeline as well as any other
component using knative/pkg to be broken on 1.22 and above.

Signed-off-by: Vincent Demeester <[email protected]>
This applies any patches that would be in `hack/patches` but not
committing them as it would be repeating the same commit over and
over.

Signed-off-by: Vincent Demeester <[email protected]>
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 25, 2021
@vdemeester vdemeester marked this pull request as ready for review August 25, 2021 11:11
@tekton-robot 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 Aug 25, 2021
@ghost
Copy link

ghost commented Aug 25, 2021

/approve

@tekton-robot
Copy link
Collaborator

[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:

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

@ghost ghost added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Aug 25, 2021
@jerop
Copy link
Member

jerop commented Aug 25, 2021

thanks for the fix @vdemeester!

please add some release notes

/lgtm

also documenting here that, as discussed in owners' meeting, we plan to patch tekton pipelines 0.27 because it's the only one released after k8s 1.22 was released -- however, we will consider patching older tekton pipelines releases if many users are using them with k8s 1.22

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2021
@vdemeester
Copy link
Member Author

please add some release notes

Oh indeed..

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 25, 2021
@tekton-robot tekton-robot merged commit 3cf7dd7 into tektoncd:main Aug 25, 2021
nikhil-thomas added a commit to nikhil-thomas/pipeline that referenced this pull request Aug 26, 2021
0003-Patch-vendor-apimachinery-to-work-on-1.22.patch has become obsolete as
tektoncd#4164 adds this change in upstream `vendor` directly

Signed-off-by: Nikhil Thomas <[email protected]>
@vdemeester vdemeester deleted the k8s_1_22 branch August 26, 2021 07:12
@ghost ghost removed the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pipelinerun doesn't show status on k8s 1.22
3 participants