-
Notifications
You must be signed in to change notification settings - Fork 174
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
Bump kubernetes 1.28 #751
Bump kubernetes 1.28 #751
Conversation
Welcome @wyike! |
Hi @wyike. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
@@ -80,7 +81,7 @@ func main() { | |||
} | |||
|
|||
fs := command.Flags() | |||
namedFlagSets := ccmOptions.Flags(app.ControllerNames(app.DefaultInitFuncConstructors), app.ControllersDisabledByDefault.List(), app.AllWebhooks, app.DisabledByDefaultWebhooks) | |||
namedFlagSets := ccmOptions.Flags(app.ControllerNames(app.DefaultInitFuncConstructors), app.ControllersDisabledByDefault.List(), names.CCMControllerAliases(), app.AllWebhooks, app.DisabledByDefaultWebhooks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New KCM and CCM change on 1.28: kubernetes/kubernetes#115813
test/e2e/go.mod
Outdated
@@ -166,15 +169,15 @@ require ( | |||
k8s.io/cli-runtime v0.27.2 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't bump the indirect dependencies( line 167-171, 173(k8s), and 176(controllerruntime) ) to 1.28 related.
They seem indirectly used by cluster api and helm. What's your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup should be fine
/assign @lubronzhan @sbueringer |
Could you create a new release file like https://github.com/kubernetes/cloud-provider-vsphere/tree/master/releases/v1.27 Also add helm chart changes, you can just do following steps |
Yes, planned to do in a follow up PR. Do you want me to include it to this PR together? |
Ok you can do it in follow up PR |
/ok-to-test |
@@ -163,18 +165,18 @@ require ( | |||
gopkg.in/yaml.v3 v3.0.1 // indirect | |||
k8s.io/apiextensions-apiserver v0.27.2 // indirect | |||
k8s.io/apiserver v0.27.2 // indirect | |||
k8s.io/cli-runtime v0.27.2 // indirect | |||
k8s.io/cli-runtime v0.28.0 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have bumped the indirect k8s.io/kubectl to 0.28.0 and several other indirect dependencies are updated automatically. Otherwise go test e2e_suite_test.go
will have compile error:
go test e2e_suite_test.go
# k8s.io/cli-runtime/pkg/resource
../../../../../../pkg/mod/k8s.io/cli-runtime@v0.27.2/pkg/resource/query_param_verifier.go:83:38: cannot use oapi (variable of type *"github.com/google/gnostic-models/openapiv2".Document) as *"github.com/google/gnostic/openapiv2".Document value in argument to supportsQueryParam
../../../../../../pkg/mod/k8s.io/cli-runtime@v0.27.2/pkg/resource/query_param_verifier.go:86:36: cannot use oapi (variable of type *"github.com/google/gnostic-models/openapiv2".Document) as *"github.com/google/gnostic/openapiv2".Document value in argument to supportsQueryParam
# k8s.io/kubectl/pkg/util/openapi
../../../../../../pkg/mod/k8s.io/kubectl@v0.27.2/pkg/util/openapi/openapi_getter.go:36:42: cannot use &CachedOpenAPIGetter{} (value of type *CachedOpenAPIGetter) as discovery.OpenAPISchemaInterface value in variable declaration: *CachedOpenAPIGetter does not implement discovery.OpenAPISchemaInterface (wrong type for method OpenAPISchema)
have OpenAPISchema() (*"github.com/google/gnostic/openapiv2".Document, error)
want OpenAPISchema() (*"github.com/google/gnostic-models/openapiv2".Document, error)
../../../../../../pkg/mod/k8s.io/kubectl@v0.27.2/pkg/util/openapi/openapi_getter.go:49:28: cannot use g.openAPIClient.OpenAPISchema() (value of type *"github.com/google/gnostic-models/openapiv2".Document) as *"github.com/google/gnostic/openapiv2".Document value in assignment
../../../../../../pkg/mod/k8s.io/kubectl@v0.27.2/pkg/util/openapi/openapi_getter.go:78:46: cannot use oapi (variable of type *"github.com/google/gnostic-models/openapiv2".Document) as *"github.com/google/gnostic/openapiv2".Document value in argument to NewOpenAPIData
../../../../../../pkg/mod/k8s.io/kubectl@v0.27.2/pkg/util/openapi/openapi.go:52:38: cannot use doc (variable of type *"github.com/google/gnostic/openapiv2".Document) as *"github.com/google/gnostic-models/openapiv2".Document value in argument to proto.NewOpenAPIData
FAIL command-line-arguments [build failed]
I didn't bump other k8s.io/xx or sigs.k8s.io/controller-runtime because bump either will throw other incompatible compile errors.
We can bump capi to 1.6 and sigs.k8s.io/controller-runtime to 0.16, along with other k8s.io/xxx to have good compatibility in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably bump to a version of CAPI main once the CR bump PR there merges (probably today). Just like we used a v1.5 pre-release version (l.185-188)
In general looks fine to me. You just might want to bump to a CAPI main version in a bit. I expect the CR bump there to merge in the next 1-3 hours |
CR bump is merged in CAPI: kubernetes-sigs/cluster-api#8999 |
Thanks @sbueringer I tried to point the capi to
I have to manually download v0.31.0 to workaround above however I see some packages including helm.sh/helm/v3 downgraded
Based on above changes, I meet more compatibility issues:
Will capi consider to bump go.opentelemetry.io/otel/metric package in future? I'm thinking to keep the dependencies of the e2e test in this PR as the current and try the bumps on capi after capi has newer go.opentelemetry.io/otel/metric dependencies. What do you think. |
@wyike I opened a PR against your fork to fix it up: wyike#1 As far as I can tell there are multiple paths how we depend on otel, e.g.:
Both paths are going through But it seems to help to pin otel: https://github.com/wyike/cloud-provider-vsphere/pull/1/files#r1303081043 I think neither ccm, nor CAPI, nor controller-runtime uses otel directly. It's all just through the apiserver dependency |
Thanks for the change |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lubronzhan, wyike 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 |
Since it's merged, we‘ll have a follow up PR to improve the capi bump in e2e test with Stefan's fixup wyike#1. |
What this PR does / why we need it:
Bump kubernetes dependencies to 1 .28
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #750Special notes for your reviewer:
Release note: