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.

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

Merged

Conversation

TheSpiritXIII
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 17, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @TheSpiritXIII!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 17, 2024
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 17, 2024
Comment on lines 1387 to 1489
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
}
Copy link
Member

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?

Copy link
Contributor Author

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.

Comment on lines 1085 to 1138
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)
}
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

@alvaroaleman alvaroaleman left a 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 Show resolved Hide resolved
}
scale, isScale := updateOptions.SubResourceBody.(*autoscalingv1.Scale)
if !isScale {
return apierrors.NewBadRequest(fmt.Sprintf("got invalid type %t, expected Scale", updateOptions.SubResourceBody))
Copy link
Member

@alvaroaleman alvaroaleman Jun 18, 2024

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.

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

pkg/client/fake/client.go Show resolved Hide resolved
pkg/client/fake/client.go Outdated Show resolved Hide resolved
@alvaroaleman
Copy link
Member

Will try to give this a proper review this week, sorry for the delay.
/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 24, 2024
@TheSpiritXIII
Copy link
Contributor Author

/retest

pkg/client/fake/client.go Outdated Show resolved Hide resolved
pkg/client/fake/client.go Outdated Show resolved Hide resolved
pkg/client/fake/client_test.go Outdated Show resolved Hide resolved
pkg/client/fake/client_test.go Outdated Show resolved Hide resolved
pkg/client/fake/client_test.go Show resolved Hide resolved
pkg/client/fake/client_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2024
@alvaroaleman
Copy link
Member

@TheSpiritXIII will you be able to rebase and resolve the remaining comments?

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2024
@TheSpiritXIII
Copy link
Contributor Author

@alvaroaleman I rebased and addressed all comments. Please let me know if there's anything else! Thanks.

@TheSpiritXIII
Copy link
Contributor Author

/retest

Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

Thank you!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 61efdf2398de62581609c830c8f1b13e809c36aa

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, TheSpiritXIII

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2024
@k8s-ci-robot k8s-ci-robot merged commit 7394f7b into kubernetes-sigs:main Aug 3, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants