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

[:warning:] bump k8s dependencies to 1.19 in 0.6.x release #1266

Conversation

varshaprasad96
Copy link
Member

@varshaprasad96 varshaprasad96 commented Nov 17, 2020

This PR bumps k8s dependencies in 0.6.x release of controller-runtime to 1.19.4.

Reason:
The current v2 plugin in Kubebuilder uses controller-runtime 0.6.x. If its possible to make patch releases of controller-runtime having upper versions of Kubernetes, it would be helpful to upgrade v2 plugin to use latest releases of k8s without any breaking changes which controller-runtime v0.7.0 may cause. This would also be helpful to update Operator SDK to support latest releases of Kubernetes, without bumping controller-runtime to 0.7.0.

cc: @joelanford

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 17, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @varshaprasad96. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: varshaprasad96
To complete the pull request process, please assign joelanford after the PR has been reviewed.
You can assign the PR to them by writing /assign @joelanford in a comment when ready.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 17, 2020
@varshaprasad96
Copy link
Member Author

varshaprasad96 commented Nov 17, 2020

@joelanford @alvaroaleman @vincepri - Can you please have a look on whether we can upgrade k8s dependencies with patch version releases of controller-runtime.

@coderanger
Copy link
Contributor

This would be a compat break, so I don't think we should do it.

@vincepri
Copy link
Member

/hold

1.19 is going to come in v0.7.x

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2020
@vincepri
Copy link
Member

@varshaprasad96 We usually wait for the next minor release before updating, the current mainline branch has v0.19.x, but we usually don't backport minor kubernetes version bumps because it could cause breaking changes to clients using controller runtime

@joelanford
Copy link
Member

SDK maintainers were discussing various options for getting to 1.19 and then 1.20 shortly after it GAs. This was one option that we wanted to explore, even if just for completeness sake. I turned up two potential breaking API changes in my quick perusal of the v1.19 notes (there could be more):

The unused series.state field, deprecated since v1.14, is removed from the events.k8s.io/v1beta1 and v1 Event types. (#90449, @wojtek-t) [SIG Apps]

The alpha DynamicAuditing feature gate and auditregistration.k8s.io/v1alpha1 API have been removed and are no longer supported. (#91502, @deads2k) [SIG API Machinery, Auth and Testing]

Also just to be transparent if we did go down this path, the intention would also be to bump v0.6.x to Kubernetes 1.20 provided there are no API breaks in that release either. The benefit with this approach would be that users could use newer versions of Kubernetes without having to migrate their projects to handle breaking changes in controller-runtime. Downside of course is the aforementioned potential for API breaks.


Another option would be what's been suggested here and what we've traditionally done: minor versions of controller-runtime that have breaking changes and/or Kubernetes version bumps. I think the implication is that there could be up to 4 new kubebuilder go plugin versions every year to handle breaking changes in controller-runtime if we always allow breaking changes on master (do we have a policy on that?).

The v0.7.0 (for 1.19) and then subsequent v0.8.0 (for 1.20) option requires a new version of the Kubebuilder go plugin because of the breaking changes in controller runtime.

Right now that would be the v3-alpha plugin. IMO, before we can make that the default in Kubebuilder (and Operator SDK) and start migrating users, we need to make that plugin stable so users can migrate and trust that it won't break.

There are some outstanding items in the kubebuilder v3 plugin milestone and possibly other breaking changes we need to make to project version 3-alpha depending on whether we want to get phase 2 plugins in before v3 is stable. But perhaps that's held until project version 4 or we find a backwards compatible way to introduce it to project version 3 🤷‍♂️

TL;DR:
Anyway, this is a very longwinded way of saying that we don't have a great story around getting releases out of controller-runtime and kubebuilder to quickly add support for Kubernetes releases when they GA, and I think we need one. 🙂

@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 17, 2020
@k8s-ci-robot
Copy link
Contributor

@varshaprasad96: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-controller-runtime-apidiff-release-0-6 e62dfe2 link /test pull-controller-runtime-apidiff-release-0-6
pull-controller-runtime-test-release-0-6 e62dfe2 link /test pull-controller-runtime-test-release-0-6

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@joelanford
Copy link
Member

joelanford commented Nov 17, 2020

Ah. Looks like there are also some breaking changes in controller-runtime APIs due to the logr change.

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_controller-runtime/1266/pull-controller-runtime-apidiff-release-0-6/1328679793574547456#1:build-log.txt%3A71

Even if we were good with all of the other changes that bumping v0.6 to 1.19 would bring, I think it would be a non-starter to directly break a controller-runtime API in a patch release.

@varshaprasad96 I think this means we need to close this PR and make a push to:

  1. get v0.7.0 released with v1.19 support
  2. bump master to k8s v1.20 (IMO, we can do an initial PR that bumps to a 1.20 pre-release, so that its a quick PR to bump to 1.20 GA as soon as it drops)
  3. release v0.8.0 with v1.20 support and ideally no C-R breaking changes since v0.7.0.
  4. update kubebuilder v3-alpha plugin to use C-R v0.8.0
  5. finalize kubebuilder v3-alpha plugin and rename to v3
  6. finalize kubebuilder 3-alpha project version and rename to 3
  7. change kubebuilder default plugin to v3
  8. release kubebuilder v3

Thoughts?

@varshaprasad96
Copy link
Member Author

varshaprasad96 commented Nov 17, 2020

Based on the discussion, closing this PR in favor of #1268 which bumps master to k8s 1.20.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants