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

feat: resource requirements #83

Merged
merged 5 commits into from
Sep 1, 2023
Merged

Conversation

KevFan
Copy link
Contributor

@KevFan KevFan commented Jul 31, 2023

Description

  • Use default and allow specifying resource requirements for limitador pod
  • Use ginkgo v2 as v1 is deprecated
  • Increase unit test coverage of /api

Closes: #74

Verification

Functionality is generally tested by the included tests already but you can run through the following for a more thorough verification:

  • Checkout this branch
  • Deploy local cluster and build
make local-setup
  • Deploy limitador cr with no resource requirements
kubectl apply -f config/samples/limitador_v1alpha1_limitador.yaml
  • Verify default limits are set
kubectl get deployments limitador-sample -o json | jq '.spec.template.spec.containers[0].resources'

image

  • Patch Limitatdor CR with resource limits
kubectl apply -f -<<EOF                                          
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador-sample
spec:
  listener:
    http:
      port: 8080
    grpc:
      port: 8081
  limits:
    - conditions: ["get_toy == 'yes'"]
      max_value: 2
      namespace: toystore-app
      seconds: 30
      variables: []
  resourceRequirements:
    limits:
      cpu: 200m
      memory: 400Mi
    requests:
      cpu: 101m
      memory: 201Mi         
EOF
  • Verify resource limits specified are set
kubectl get deployments limitador-sample -o json | jq '.spec.template.spec.containers[0].resources'

image

  • Patch CR with empty resource requirements (i.e. no limits)
kubectl apply -f -<<EOF                                                                           
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador         
metadata:               
  name: limitador-sample
spec:      
  listener:     
    http:       
      port: 8080
    grpc:       
      port: 8081                      
  limits:                             
    - conditions: ["get_toy == 'yes'"]
      max_value: 2           
      namespace: toystore-app
      seconds: 30      
      variables: []    
  resourceRequirements: {}
EOF
  • Verify resource limits are empty
kubectl get deployments limitador-sample -o json | jq '.spec.template.spec.containers[0].resources'

image

  • Patch CR to set resource requirements back to nil
kubectl apply -f -<<EOF                                                                           
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador         
metadata:               
  name: limitador-sample
spec:      
  listener:     
    http:       
      port: 8080
    grpc:       
      port: 8081                      
  limits:                             
    - conditions: ["get_toy == 'yes'"]
      max_value: 2           
      namespace: toystore-app
      seconds: 30      
      variables: []    
EOF
  • Verifiy default limits are applied
kubectl get deployments limitador-sample -o json | jq '.spec.template.spec.containers[0].resources'

image

@KevFan KevFan force-pushed the resourceRequirements branch from 5369350 to 4f7df34 Compare July 31, 2023 11:27
@KevFan KevFan added kind/enhancement New feature or request participation/good first issue Good for newcomers target/current size/medium area/api CRD or other public API related labels Jul 31, 2023
@KevFan KevFan force-pushed the resourceRequirements branch from 4f7df34 to c1ea432 Compare July 31, 2023 12:54
@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2023

Codecov Report

Merging #83 (12485ea) into main (e110bb7) will increase coverage by 6.56%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
+ Coverage   43.31%   49.87%   +6.56%     
==========================================
  Files          11       12       +1     
  Lines         718      812      +94     
==========================================
+ Hits          311      405      +94     
  Misses        369      369              
  Partials       38       38              
Flag Coverage Δ
unit 49.87% <100.00%> (+6.56%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
api/v1alpha1/limitador_types.go 100.00% <100.00%> (ø)
controllers/limitador_controller.go 42.13% <100.00%> (+0.65%) ⬆️
pkg/limitador/k8s_objects.go 67.87% <100.00%> (+0.19%) ⬆️
pkg/reconcilers/deployment.go 24.24% <100.00%> (+24.24%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@KevFan KevFan force-pushed the resourceRequirements branch 7 times, most recently from abd3000 to 7ef1644 Compare August 1, 2023 08:23
Comment on lines +41 to +52
defaultResourceRequirements = &corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("250m"),
corev1.ResourceMemory: resource.MustParse("32Mi"),
},
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("500m"),
corev1.ResourceMemory: resource.MustParse("64Mi"),
},
}
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on #74

@KevFan KevFan marked this pull request as ready for review August 1, 2023 13:02
@KevFan KevFan requested a review from a team August 1, 2023 13:02
@KevFan KevFan force-pushed the resourceRequirements branch from 78f0803 to 293ce1d Compare August 2, 2023 16:02
Copy link
Contributor

@Boomatang Boomatang left a comment

Choose a reason for hiding this comment

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

Most of this looks good to me, there is a few questions in the comments.

On the point of making the assumption that container will be the first in the list is a concern. I have tried adding other containers to the deployment and the operator allows it to happen. When the extra container is the first in the list the operator rewrites the configuration to what we expect the configuration to be. It does not change the name of the container and leaves the old container there. If the extra container is added in the second position on the list the operator will leave the configuration on changed. This is possible a large issue and effects more than just this PR.

@@ -0,0 +1,76 @@
# Resource Requirements

The default resource requirement for _**Limitador**_ deployments is specified in [Limitador v1alpha1 API reference](../api/v1alpha1/limitador_types.go)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand how Kuadrant.io is working, this file will be included in the documentation. My concern is the Limitador v1alpha1 API reference link will be a broken link on the site. Do you know if this is the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't know will this be the case

Copy link
Member

Choose a reason for hiding this comment

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

These links used to work fine... and should still in the new website, but I haven't checked explicitly. Tho I know authorino uses relative links like these a lot, and these were fine on docs.kuadrant.io

Copy link
Contributor

Choose a reason for hiding this comment

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

I have noticed we don't document the API. Is this the best place of the resource requirements or should there be a more targeted API documentation?

Copy link
Contributor Author

@KevFan KevFan Aug 11, 2023

Choose a reason for hiding this comment

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

I'm not sure tbh, I'm just matching https://github.com/Kuadrant/limitador-operator/blob/main/doc/storage.md which documents Storage seperately 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Are the (authorino|limitador)-operator docs on the website? quickly looking I couldn't find them 🤔

Copy link
Member

Choose a reason for hiding this comment

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

@KevFan KevFan force-pushed the resourceRequirements branch 3 times, most recently from 63a0bf9 to 3449a78 Compare August 16, 2023 10:00
@KevFan KevFan force-pushed the resourceRequirements branch from 3449a78 to 12485ea Compare August 30, 2023 14:14
@KevFan
Copy link
Contributor Author

KevFan commented Aug 30, 2023

Rebased with main to resolve conflict

Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

LGTM
good job

@KevFan KevFan force-pushed the resourceRequirements branch from 12485ea to 5554b69 Compare August 31, 2023 09:41
@KevFan
Copy link
Contributor Author

KevFan commented Aug 31, 2023

Rebased with main to resolve conflict again

@KevFan KevFan merged commit beaa93e into Kuadrant:main Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api CRD or other public API related kind/enhancement New feature or request participation/good first issue Good for newcomers size/medium target/current
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Production-ready: Configure deployment resources
5 participants