-
Notifications
You must be signed in to change notification settings - Fork 1.4k
⚠️ Bump to controller-runtime v0.22 & controller-tools v0.19 #12634
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 to controller-runtime v0.22 & controller-tools v0.19 #12634
Conversation
96348cb
to
d69e8a1
Compare
@@ -41,7 +41,7 @@ type etcd interface { | |||
AlarmList(ctx context.Context) (*clientv3.AlarmResponse, error) | |||
Close() error | |||
Endpoints() []string | |||
MemberList(ctx context.Context) (*clientv3.MemberListResponse, error) | |||
MemberList(ctx context.Context, opts ...clientv3.OpOption) (*clientv3.MemberListResponse, error) |
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.
@fabriziopandini We should check if we want to specify one of the options
/hold
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.
ffb77a6
to
5b9559d
Compare
@@ -116,10 +116,12 @@ type DockerClusterV1Beta2Status struct { | |||
// APIEndpoint represents a reachable Kubernetes API endpoint. | |||
type APIEndpoint struct { | |||
// Host is the hostname on which the API server is serving. | |||
// +optional |
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.
This is a fun one.
Context:
- topology controller filters out host if host is set to empty string
- so it will sent a v1beta1 DockerCluster without host being set
Why did this work before
- v1.10 (v1beta1): defaulting webhook was adding
host: ""
- v1.11 (v1beta1+v1beta2): defaulting webhook was adding
metadata.creationTimestamp: nil
=> then conversion v1beta2 => v1beta1 was invoked before OpenAPI validation and conversion addedhost: ""
- v1.12 (v1beta1+v1beta2): defaulting webhook does nothing => no additional conversion, so original v1beta1 DockerCluster hits OpenAPI validation =>
spec.controlPlaneEndpoint.host: Required value
Potential solutions:
- Stop filtering out empty string in topology controller
- => We can't do that because otherwise the topology controller would overwrite
host
values written by the InfraCluster provider
- => We can't do that because otherwise the topology controller would overwrite
- Changing v1beta1 OpenAPI validation to make host/port optional
- => should work. When getting v1beta1 DockerCluster it will still return
host: ""
in any case
- => should work. When getting v1beta1 DockerCluster it will still return
- Trying to keep defaulting webhook constant by always patching creationTimestamp, but this doesn't fix issues we might already have with v1.11 where we dropped defaulting webhooks entirely
Wider impact:
- We should think about if we have similar cases elsewhere. I.e. cases where in the past conversion was fixing up objects and they passed OpenAPI validation only because of that. We should consider also making these fields optional in v1beta1 OpenAPI validation in CAPI v1.12
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.
Discussed. We'll:
- make host + port optional in v1beta1 DockerCluster
- make host + port optional in v1beta1 APIEndpoint (core CAPI)
- Add an explanation to v1.11-to-v1.12.md
df9aaf7
to
99c51dd
Compare
99c51dd
to
8a80636
Compare
1c46b8c
to
d702cc4
Compare
d702cc4
to
c5f1f96
Compare
c5f1f96
to
0218f58
Compare
7a78ca0
to
94abd78
Compare
/test pull-cluster-api-e2e-blocking-main |
/assign @sivchari |
/assign @chrischdi |
/lgtm Thanks for working on this! |
LGTM label has been added. Git tree hash: 1bcf50a235b5eb46c23f8db5f6cf00cacd7e04c8
|
@@ -1555,6 +1556,7 @@ func (f *FakeClusterClass) Objs() []client.Object { | |||
} | |||
|
|||
clusterClass := clusterClassBuilder.Build() | |||
clusterClass.SetGroupVersionKind(clusterv1.GroupVersion.WithKind("ClusterClass")) |
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.
What about if we unset the gvk right after we use it for computing the UID a few lines below?
This could prevent this to hide wrong assumption in the codebase due to the fact that we are setting GVK in a typed object
util/collections/machine_filters.go
Outdated
@@ -101,12 +100,12 @@ func InFailureDomains(failureDomains ...string) Func { | |||
|
|||
// OwnedMachines returns a filter to find all machines owned by specified owner. | |||
// Usage: GetFilteredMachinesForCluster(ctx, client, cluster, OwnedMachines(controlPlane)). | |||
func OwnedMachines(owner client.Object) func(machine *clusterv1.Machine) bool { | |||
func OwnedMachines(owner *controlplanev1.KubeadmControlPlane) func(machine *clusterv1.Machine) bool { |
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.
godoc seems out of date
(might be we should also rename the func / move it to KCP)
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'll change the func so it still works for other CP providers that have Machines
@fabriziopandini Thx, should be all fixed, PTAL |
/test pull-cluster-api-e2e-blocking-main |
@sbueringer: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
lgtm pending a check on test failures |
It is a flake, re running tests |
LGTM label has been added. Git tree hash: c54e41d4facb8007629cc90233b54df4db891571
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #12325