-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ Add scale subresource logic to fake client #2855
base: main
Are you sure you want to change the base?
✨ Add scale subresource logic to fake client #2855
Conversation
Welcome @TheSpiritXIII! |
Hi @TheSpiritXIII. 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 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-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: TheSpiritXIII 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 |
pkg/client/fake/client.go
Outdated
func applyScale(obj client.Object, scale *autoscalingv1.Scale) error { | ||
switch obj := obj.(type) { | ||
case *appsv1.Deployment: | ||
obj.Spec.Replicas = &scale.Spec.Replicas | ||
case *appsv1.ReplicaSet: | ||
obj.Spec.Replicas = &scale.Spec.Replicas | ||
case *corev1.ReplicationController: | ||
obj.Spec.Replicas = &scale.Spec.Replicas | ||
case *appsv1.StatefulSet: | ||
obj.Spec.Replicas = &scale.Spec.Replicas | ||
default: | ||
// TODO: CRDs https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#scale-subresource | ||
return fmt.Errorf("unable to extract scale from type %T", obj) | ||
} | ||
return nil | ||
} |
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.
Wouldn't this not allow people who make CRs who wish to use the sub resource scale be limited to only allow the types in the case statement available?
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.
Correct. This would be more work: we'd have to look-up the custom resource definition to get the scale replica field path. I propose we leave this for a future PR, since it also means writing more unit tests and I'm not sure I have bandwidth.
pkg/client/fake/client.go
Outdated
switch sw.subResource { | ||
case "scale": | ||
scale, isScale := subResource.(*autoscalingv1.Scale) | ||
if !isScale { | ||
return apierrors.NewBadRequest(fmt.Sprintf("got invalid type %t, expected Scale", subResource)) | ||
} | ||
if err := sw.client.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { | ||
return err | ||
} | ||
scaleOut, err := extractScale(obj) | ||
if err != nil { | ||
return err | ||
} | ||
*scale = scaleOut | ||
return nil | ||
default: | ||
return fmt.Errorf("fakeSubResourceClient does not support get for %s", sw.subResource) | ||
} |
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.
Having to use the subresource client to then use the actual client seems like we wouldn't need the sub resource client to do the GET. The value of the scale is indicative on the spec and the subresource client has to get that value from the actual client where it's derived.
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 implemented on the server-side as far as I know. I cannot just lookup on the client because the fake client doesn't know how to extract this information. It's something of a meta-resource.
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.
Fair and not criticizing the implementation but asking because you need to deal with this the way you are dealing with it.
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 a lot for looking into this, really appreciated! Few comments, we need to make really sure we behave the same way as the KAS does everywhere, because as soon as we release this, some ppl will start depending on whatever behavior it exhibits which might cause tests to incorrectly pass if this doesn't exactly match the kube apiserver behavior.
pkg/client/fake/client.go
Outdated
} | ||
scale, isScale := updateOptions.SubResourceBody.(*autoscalingv1.Scale) | ||
if !isScale { | ||
return apierrors.NewBadRequest(fmt.Sprintf("got invalid type %t, expected Scale", updateOptions.SubResourceBody)) |
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.
Is this the behavior of the KAS, to first complain with a bad request about the request body before checking if the main resource even exists? Please look it up and add a reference link to the relevant code in the KAS as comment.
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 was unable to find it; am I looking at the right place? https://github.com/search?q=repo%3Akubernetes%2Fapiserver+scale+-path%3Atest&type=code
Nonetheless, I was able to reproduce it with client.Client.Get
and your intuition is correct: it looks up the object first! Great catch.
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 think the upstream code is here: https://github.com/kubernetes/kubernetes/blob/fb6bbc9781d11a87688c398778525c4e1dcb0f08/pkg/registry/apps/deployment/storage/storage.go#L320
Not exactly a quick read though it seems.
9d4522d
to
1dc0cb5
Compare
Will try to give this a proper review this week, sorry for the delay. |
1dc0cb5
to
65442bb
Compare
/retest |
}, | ||
}, nil | ||
default: | ||
crd, err := getCustomResourceDefinition(ctx, c, obj) |
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.
Can we remove this code and directly error out? For status for example we have an explicit config option on the fake client. I don't know how that would best look like for scale given it requires more config than just a binary enable/disable, but I would prefer not adding any code related to that until we actually support that.
replicas = *obj.Spec.Replicas | ||
} | ||
return &autoscalingv1.Scale{ | ||
ObjectMeta: obj.ObjectMeta, |
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 think its only namespace, name, uid, resourceVersion and creationTimestamp we should copy over, not for example labels or annotations. Please update this and the corresponding tests.
|
||
scale := &autoscalingv1.Scale{Spec: autoscalingv1.ScaleSpec{Replicas: 2}} | ||
Expect(cl.SubResource(subResourceScale).Get(context.Background(), obj, scale)).ToNot(Succeed()) | ||
Expect(cl.SubResource(subResourceScale).Update(context.Background(), obj, client.WithSubResourceBody(scale))).ToNot(Succeed()) |
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.
Could we match the error rather than just checking for an error? Right now, this test would for example incorrectly pass if you did a GET for an object that doesn't exist
objOriginal.APIVersion = scale.APIVersion | ||
objOriginal.Kind = scale.Kind |
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.
These lines confuse me. Neither objOriginal
nor scale
should have either APIVersion
or Kind
set - Why is this needed? Same question for the update test.
}) | ||
|
||
It("should be able to Get scale subresources", func() { | ||
obj := &appsv1.Deployment{ |
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 would be good to test this for all object kinds where we support this, maybe you can loop over a list of deployment/replicaset/statefulset/daemonset for that?
objOriginal.APIVersion = scale.APIVersion | ||
objOriginal.Kind = scale.Kind | ||
objOriginal.ResourceVersion = scale.ResourceVersion | ||
objOriginal.Spec.Replicas = ptr.To(scale.Spec.Replicas) |
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 test needs to verify that they are equal, right now it doesn't do that
PR needs rebase. 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. |
Adds basic GET/UPDATE support to the scale sub-resource according to the docs: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client#SubResourceClientConstructor