-
Notifications
You must be signed in to change notification settings - Fork 438
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
Drop Kubernetes v1.26, and support Kubernetes v1.29 #2308
Drop Kubernetes v1.26, and support Kubernetes v1.29 #2308
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tenzen-y 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 |
// TODO: Once the below issue is resolved, we need to switch discovery-client to the built-in one. | ||
// https://github.com/kubernetes-sigs/controller-runtime/issues/2354 | ||
// https://github.com/kubernetes-sigs/controller-runtime/issues/2424 | ||
MapperProvider: apiutil.NewDiscoveryRESTMapper, |
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 issue is resolved in the controller-runtime v0.17.
Resources: corev1.ResourceRequirements{ | ||
Resources: corev1.VolumeResourceRequirements{ |
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.
Dedicated API is introduced into the PVC: kubernetes/kubernetes#118653
type ssaPatchAsStrategicMerge struct { | ||
client.Patch | ||
} | ||
|
||
func (*ssaPatchAsStrategicMerge) Type() types.PatchType { | ||
return types.StrategicMergePatchType | ||
} | ||
|
||
func wrapSSAPatch(patch client.Patch) client.Patch { | ||
if patch.Type() == types.ApplyPatchType { | ||
return &ssaPatchAsStrategicMerge{Patch: patch} | ||
} | ||
return patch | ||
} | ||
|
||
// ssaAsStrategicMergePatchFunc returns the patch interceptor. | ||
// TODO (tenzen-y): Once the fake client supports server-side apply, we should remove these interceptor. | ||
// REF: https://github.com/kubernetes/kubernetes/issues/115598 | ||
func ssaAsStrategicMergePatchFunc( | ||
ctx context.Context, | ||
cli client.WithWatch, | ||
obj client.Object, | ||
patch client.Patch, | ||
opts ...client.PatchOption, | ||
) error { | ||
return cli.Patch(ctx, obj, wrapSSAPatch(patch), opts...) | ||
} |
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.
As I described in the comments, we need to keep this interceptor until the fake client supports SSA since the certgenerator uses the SSA here:
katib/pkg/certgenerator/v1beta1/generator.go
Line 258 in 1365e47
err := c.kubeClient.Patch(ctx, newVWebhookConfig, client.Apply, client.FieldOwner(ssaFieldOwnerName), client.ForceOwnership) katib/pkg/certgenerator/v1beta1/generator.go
Line 285 in 1365e47
err := c.kubeClient.Patch(ctx, newMWebhookConfig, client.Apply, client.FieldOwner(ssaFieldOwnerName), client.ForceOwnership)
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.
Great, thanks for finding this!
/assign @andreyvelich @johnugeorge |
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.
Thanks for the update @tenzen-y!
I am wondering if we should also update the Kubernetes Python Client in SDK: https://github.com/kubeflow/katib/blob/master/sdk/python/v1beta1/setup.py#L24
My suggestion is to use the earliest K8s version that we support: 1.27 with the latest stable release of this version from Python Client: e.g. 27.2: https://github.com/kubernetes-client/python/releases/tag/v27.2.0
Thoughts @kubeflow/wg-training-leads @droctothorpe ?
type ssaPatchAsStrategicMerge struct { | ||
client.Patch | ||
} | ||
|
||
func (*ssaPatchAsStrategicMerge) Type() types.PatchType { | ||
return types.StrategicMergePatchType | ||
} | ||
|
||
func wrapSSAPatch(patch client.Patch) client.Patch { | ||
if patch.Type() == types.ApplyPatchType { | ||
return &ssaPatchAsStrategicMerge{Patch: patch} | ||
} | ||
return patch | ||
} | ||
|
||
// ssaAsStrategicMergePatchFunc returns the patch interceptor. | ||
// TODO (tenzen-y): Once the fake client supports server-side apply, we should remove these interceptor. | ||
// REF: https://github.com/kubernetes/kubernetes/issues/115598 | ||
func ssaAsStrategicMergePatchFunc( | ||
ctx context.Context, | ||
cli client.WithWatch, | ||
obj client.Object, | ||
patch client.Patch, | ||
opts ...client.PatchOption, | ||
) error { | ||
return cli.Patch(ctx, obj, wrapSSAPatch(patch), opts...) | ||
} |
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.
Great, thanks for finding this!
+1 |
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
58c7c3f
to
dcb289e
Compare
I updated the required Kubernetes version for the Python client as well since we don't have any objections. |
@andreyvelich Could you take another look? |
Thanks for the update @tenzen-y! |
/lgtm |
Sure. |
What this PR does / why we need it:
I updated supporting Kubernetes versions.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #2256
Checklist: