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

✨ redis cached: batch size option #139

Merged
merged 1 commit into from
May 21, 2024
Merged

✨ redis cached: batch size option #139

merged 1 commit into from
May 21, 2024

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented May 16, 2024

What

Batch size was introduced in Limitador Kuadrant/limitador#318.

New command line argument for redis_cached backend storage: --batch-size <num-entries> Size of entries to flush in as single flush [default: 100]

$ docker run --rm -t quay.io/kuadrant/limitador:latest limitador-server limit.yaml redis_cached --help
Uses Redis to store counters, with an in-memory cache

Usage: limitador-server <LIMITS_FILE> redis_cached [OPTIONS] <URL>

Arguments:
  <URL>  Redis URL to use

Options:
      --batch-size <batch>          Size of entries to flush in as single flush [default: 100]
      --flush-period <flush>        Flushing period for counters in milliseconds [default: 1000]
      --max-cached <max>            Maximum amount of counters cached [default: 10000]
      --response-timeout <timeout>  Timeout for Redis commands in milliseconds [default: 350]
  -h, --help                        Print help

This PR exposes batch-size at the Limitador CRD

Verification steps

dev setup

make local-setup

deploy redis

kubectl create namespace redis-ns
kubectl -n redis-ns apply -f - <<EOF
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: redis
  labels:
    app: redis
spec:
  selector:
    matchLabels:
      app: redis
  template:
    metadata:
      labels:
        app: redis
    spec:
      containers:
        - name: redis
          image: redis
---
apiVersion: v1
kind: Service
metadata:
  name: redis
spec:
  selector:
    app: redis
  ports:
    - name: redis
      port: 6379
      protocol: TCP
      targetPort: 6379
EOF

Deploy the limitador CR with redis-cached storage with response timeout set to 500ms

k apply -f - <<EOF
---
apiVersion: v1
kind: Secret
metadata:
  name: redisconfig
stringData:
  URL: redis://redis.redis-ns:6379
type: Opaque
EOF
k apply -f - <<EOF
---
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador
spec:
  storage:
    redis-cached:
      configSecretRef:
        name: redisconfig
      options:
        batch-size: 500
  limits:
    - conditions: []
      max_value: 2
      namespace: toystore-app
      seconds: 30
      variables: []
EOF

Check deployment is correct

kubectl wait --timeout=300s --for=condition=Ready limitador limitador

Should return

limitador.limitador.kuadrant.io/limitador condition met

Check command line

kubectl get deployment/limitador-limitador  -o yaml | yq '.spec.template.spec.containers[0].command'

should look like this

- limitador-server
- --http-port
- "8080"
- --rls-port
- "8081"
- /home/limitador/etc/limitador-config.yaml
- redis_cached
- $(LIMITADOR_OPERATOR_REDIS_URL)
- --batch-size
- "500"

@eguzki eguzki requested a review from a team May 16, 2024 15:28
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.15%. Comparing base (f586167) to head (b2d58ac).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #139      +/-   ##
==========================================
- Coverage   85.52%   85.15%   -0.38%     
==========================================
  Files          19       19              
  Lines         988      990       +2     
==========================================
- Hits          845      843       -2     
- Misses         94       96       +2     
- Partials       49       51       +2     
Flag Coverage Δ
integration 79.49% <100.00%> (-0.37%) ⬇️
unit 66.51% <100.00%> (+0.10%) ⬆️

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

Components Coverage Δ
api/v1alpha1 (u) 100.00% <ø> (ø)
pkg/helpers (u) 83.87% <ø> (ø)
pkg/log (u) 94.73% <ø> (ø)
pkg/reconcilers (u) 74.67% <ø> (ø)
pkg/limitador (u) 98.11% <100.00%> (+0.01%) ⬆️
controllers (i) 75.67% <ø> (-1.36%) ⬇️
pkg/upgrades 88.88% <ø> (ø)
Files Coverage Δ
api/v1alpha1/limitador_types.go 100.00% <ø> (ø)
pkg/limitador/redis_cache_storage_options.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Contributor

@KevFan KevFan left a comment

Choose a reason for hiding this comment

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

Verified, changes looks good to me 👍

@KevFan KevFan added the kind/enhancement New feature or request label May 20, 2024
@eguzki eguzki merged commit f4f05c9 into main May 21, 2024
14 checks passed
@eguzki eguzki deleted the redis-cached-new-params branch May 21, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants