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.

Sign up for GitHub

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

Jump to bottom

chore: Enable mod upgrade of controller-runtime, knative, and logr #2457

Merged
merged 6 commits into from
Sep 13, 2022
Merged

chore: Enable mod upgrade of controller-runtime, knative, and logr #2457

merged 6 commits into from
Sep 13, 2022
+232 −284

Conversation

jonathan-innis
Copy link
Contributor

Fixes #

Description

  • Enable mod upgrade of controller-runtime, knative, and logr

How was this change tested?

  • make test
  • make build
  • Manual Validation of Build

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

Release Note

None

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Sorry, something went wrong.

@jonathan-innis jonathan-innis added the blocked Unable to make progress due to some dependency label Sep 2, 2022
@netlify
Copy link

netlify bot commented Sep 2, 2022 •
edited
Loading

✅ Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 600bdf9
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/632007d9f0895a000967362a

@jonathan-innis
Copy link
Contributor Author

This PR is blocked until knative upgrades its packages to support v0.24.x of kubernetes api packages

@olemarkus
Copy link
Contributor

Do you actually need v0.24.x support for the API in knative? As far as I know, Karpenter doesn't use any 1.25.0 resources.
I used v0.25.x for #2354 and that works just fine.

@jonathan-innis
Copy link
Contributor Author

jonathan-innis commented Sep 2, 2022 •
edited
Loading

Do you actually need v0.24.x support for the API in knative? As far as I know, Karpenter doesn't use any 1.25.0 resources.
I used v0.25.x for #2354 and that works just fine.

It's client-go support, not necessarily the server K8s version. Your PR #2354 only upgrades the client-go package (and other k8s apis) to v0.22.13.

There is an incompatibility with an interface that Knative implements starting in v0.24.0 of K8s api packages which is blocking us from upgrading past and including that version.

@jonathan-innis
Copy link
Contributor Author

To be more explicit, the crux of the issue is the following:

  1. We can't upgrade to v0.11.x of controller-runtime due to #1752
  2. Upgrade to v0.12.x has a minimum requirement of v0.24.2 of the k8s api client packages
  3. Upgrade of v0.24.2 of k8s api client packages breaks compatability with knative because the latest knative version does not support the StorageV1Interface
# knative.dev/pkg/client/injection/kube/client
../../go/pkg/mod/knative.dev/pkg@v0.0.0-20220826162920-93b66e6a8700/client/injection/kube/client/client.go:13323:9: cannot use &wrapStorageV1{…} (value of type *wrapStorageV1) as type "k8s.io/client-go/kubernetes/typed/storage/v1".StorageV1Interface in return statement:
        *wrapStorageV1 does not implement "k8s.io/client-go/kubernetes/typed/storage/v1".StorageV1Interface (missing CSIStorageCapacities method)
Error: failed to publish images: error building "ko://github.com/aws/karpenter/cmd/controller": exit status 2

@olemarkus
Copy link
Contributor

Ah. Thanks for the info. I now remember I could not go up as far as I like on k8s deps due to knative and just had to go for one that had the entire policy/v1. Sorry for the noise.

@jonathan-innis
Copy link
Contributor Author

Knative package upgrade being tracked here

@jonathan-innis
Copy link
Contributor Author

Knative package is upgraded 🎉

@jonathan-innis jonathan-innis removed the blocked Unable to make progress due to some dependency label Sep 9, 2022
@jonathan-innis jonathan-innis marked this pull request as ready for review September 9, 2022 18:13
@jonathan-innis jonathan-innis requested a review from a team as a code owner September 9, 2022 18:13
@jonathan-innis jonathan-innis force-pushed the enable-mod-upgrade branch 2 times, most recently from 00ded71 to bde70b3 Compare September 9, 2022 18:32
tzneal
tzneal previously approved these changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@tzneal tzneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jonathan-innis jonathan-innis changed the title Enable mod upgrade of controller-runtime, knative, and logr chore: Enable mod upgrade of controller-runtime, knative, and logr Sep 12, 2022
tzneal
tzneal approved these changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@tzneal tzneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@tzneal tzneal merged commit cf6bb79 into aws:main Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzneal tzneal

@dewjam dewjam

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jonathan-innis @olemarkus @tzneal