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 support for resource request/limit annotations on k8s Gateways #52694

Closed

Conversation

aleks-andr
Copy link

@aleks-andr aleks-andr commented Aug 15, 2024

Please provide a description of this PR:
Related to feature request in #52692.

This PR adds support for overriding resource requests and limits on Deployments generated by native k8s Gateways, using the same annotations that are available for overriding resources on regular proxy sidecars.

For example, the following Gateway resource:

apiVersion: "gateway.networking.k8s.io/v1beta1"
kind: "Gateway"
metadata:
  name: test-gateway
  namespace: istio-system
  annotations:
    "sidecar.istio.io/proxyCPU": 123m
    "sidecar.istio.io/proxyMemory": 456Mi
spec:
  gatewayClassName: "istio"
  listeners:
    - name: "http"
      port: 80
      protocol: "HTTP"
      allowedRoutes:
        namespaces:
          from: "All"

will result in a ingress gateway Deployment with the following:

        resources:                                                                                                                                                                    
          requests:                                                                                                                                                                   
            cpu: 123m                                                                                                                                                                  
            memory: 456Mi  

as opposed to using the values from the global proxy configuration.

This allows users to vertically scale their Gateway deployments without having to resort to changing the global configuration (and thus inadvertently scaling all sidecars within the mesh).

⚠️ This is just a draft PR to showcase a concept. I have not performed any extensive testing on this beyond playing around with individual resources in a local k8s cluster.

@istio-policy-bot
Copy link

😊 Welcome @aleks-andr! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

Copy link

linux-foundation-easycla bot commented Aug 15, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 15, 2024
@istio-testing
Copy link
Collaborator

Hi @aleks-andr. Thanks for your PR.

I'm waiting for a istio 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.

@keithmattix
Copy link
Contributor

keithmattix commented Aug 15, 2024

I'm not sure this is the way we want to go about doing this (though I do think we need it as a feature). I would prefer that we remove or replace the sidecar part of the proxyCPU and proxyMemory annotations and make them generally apply to kube gateways. This way, users can leverage the infrastructure field of Gateway API to set these annotations to whatever they like. If a user has the perms to deploy a Gateway, they should be able to specify the resources it requires. If we want to use paramsRef with a revitalized ProxyConfig resource, that works as well

@aleks-andr
Copy link
Author

I'm not sure this is the way we want to go about doing this (though I do think we need it as a feature). I would prefer that we remove or replace the sidecar part of the proxyCPU and proxyMemory annotations and make them generally apply to kube gateways. This way, users can leverage the infrastructure field of Gateway API to set these annotations to whatever they like. If a user has the perms to deploy a Gateway, they should be able to specify the resources it requires. If we want to use paramsRef with a revitalized ProxyConfig resource, that works as well

Thanks for the feedback 🙏 I noticed the controller already reads the infrastructure labels and annotations and tried adjusting my implementation to use those directly. I did however notice during testing that spec.infrastructure is still under the Experimental channel of Gateway API, meaning that this feature would be unavailable for anyone not running the experimental branch (so most people). Not sure what're your thoughts on this, but to me it doesn't seem like the best approach to ship a feature that is locked behind experimental APIs.

As per your suggestion, I did go ahead and replace the (admittedly misleading) sidecar.istio.io with a more suitable gateway.istio.io and switched the casing from proxyCPULimit to proxy-cpu-limit for consistency with other Gateway-related annotations used by the controller.

@keithmattix
Copy link
Contributor

/cc @howardjohn for feedback.

The gateway infra annotations will hit standard next release so I'm less worried about that

@aleks-andr aleks-andr marked this pull request as ready for review September 5, 2024 13:04
@aleks-andr aleks-andr requested a review from a team as a code owner September 5, 2024 13:04
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Sep 5, 2024
@aleks-andr
Copy link
Author

aleks-andr commented Sep 5, 2024

Finally getting back to this now that I have a bit more time on my hands. I don't really have much to add to this implementation-wise, so looking for any feedback.

I did spend some time investigating how this would work for clusters running a Gateway API version without the infrastructure field support, and it looks like someone's already handled it here, making the controller use the annotations/labels on the Gateway itself as a fallback in cases where spec.infrastructure is missing.

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 16, 2024
@istio-policy-bot
Copy link

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2024-08-16. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/perf and scalability kind/enhancement lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while needs-ok-to-test size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants