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

Resource names alignment #108

Merged
merged 2 commits into from
Nov 20, 2023
Merged

Conversation

grzpiotrowski
Copy link
Contributor

@grzpiotrowski grzpiotrowski commented Oct 23, 2023

This fixes the inconsistency in the naming of resources created by the operator, specifically the deployment and limits config map. Simply by adding the "limitador-" prefix to both of them.

As in the scenario described in the issue, the deployment becomes named limitador-cluster and the configmap imitador-limits-config-cluster.

Additionally there is an upgrade path provided for the deployment and configmap.

I was not sure if it's ok to simply delete and create a new configmap with the limitador- prefixed new name or should the contents be migrated from the old configmap to the new one.

That said, if we decide the upgrade path brings little value at this stage - I'm happy to drop it completely.

Fixes #105

Verification

Using the sample given in the issue - with redis.

make local-setup

Create redis deployment, service and secret:

echo "
apiVersion: v1
kind: Secret
metadata:
  name: redisconfig
stringData:
  URL: redis://redis.default.svc.cluster.local/0
type: Opaque
---
apiVersion: v1
kind: Service
metadata:
  name: redis
spec:
  ports:
  - port: 6379
    protocol: TCP
    targetPort: 6379
  selector:
    app: redis
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: redis
spec:
  replicas: 1
  selector:
    matchLabels:
      app: redis
  template:
    metadata:
      labels:
        app: redis
    spec:
      containers:
      - image: redis:latest
        name: redis
        ports:
        - containerPort: 6379
  " | kubectl apply -f -

Make sure you are on the older version of the limitador-operator on the main branch:

git checkout main

Deploy Limitador CR:

echo "
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: cluster
spec:
  affinity:
    podAntiAffinity:
      preferredDuringSchedulingIgnoredDuringExecution:
        - podAffinityTerm:
            labelSelector:
              matchLabels:
                app: limitador
                limitador-resource: cluster
            topologyKey: kubernetes.io/hostname
          weight: 100
        - podAffinityTerm:
            labelSelector:
              matchLabels:
                app: limitador
                limitador-resource: cluster
            topologyKey: topology.kubernetes.io/zone
          weight: 99
  limits:
    - conditions: []
      max_value: 400
      namespace: kuard
      seconds: 1
      variables:
        - per_hostname_per_second_burst
  listener:
    grpc:
      port: 8081
    http:
      port: 8080
  pdb:
    maxUnavailable: 1
  replicas: 3
  resourceRequirements:
    limits:
      cpu: 500m
      memory: 64Mi
    requests:
      cpu: 250m
      memory: 32Mi
  storage:
    redis:
      configSecretRef:
        name: redisconfig
" | kubectl apply -f -

Switch to the feature branch with the updated Deployment and Limits ConfigMap names:

git checkout resource-names

Check the Limits config map and Limitador deployment names:

watch kubectl get configmaps
watch kubectl get deployments

Deploy the new version of the operator:

make local-redeploy

New Limits config map with the name of limitador-limits-config-cluster should get created and the new deployment with the name limitador-cluster. Old resources get deleted.

Clean up:

make local-cleanup

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2023

Codecov Report

Merging #108 (5a9d701) into main (a0a88c2) will increase coverage by 0.58%.
The diff coverage is 80.43%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   69.56%   70.14%   +0.58%     
==========================================
  Files          14       15       +1     
  Lines        1025     1055      +30     
==========================================
+ Hits          713      740      +27     
- Misses        270      271       +1     
- Partials       42       44       +2     
Flag Coverage Δ
unit 71.64% <80.43%> (+0.51%) ⬆️

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) ∅ <ø> (∅)
pkg/log (u) 31.81% <ø> (ø)
pkg/reconcilers (u) 46.12% <ø> (ø)
pkg/limitador (u) 88.61% <100.00%> (+0.06%) ⬆️
controllers (i) 66.86% <ø> (+0.39%) ⬆️
Files Coverage Δ
controllers/limitador_controller.go 62.06% <ø> (+0.28%) ⬆️
pkg/limitador/k8s_objects.go 81.64% <100.00%> (+0.17%) ⬆️
pkg/upgrades/v0_7_0.go 78.57% <78.57%> (ø)

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.

Every thing is working as I expected.

The only changes I am asking for is to help our future self remove the required upgrade logic.

pkg/limitador/k8s_objects.go Outdated Show resolved Hide resolved
controllers/limitador_controller.go Outdated Show resolved Hide resolved
controllers/limitador_controller.go Outdated Show resolved Hide resolved
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.

Changes looks good to me

@eguzki
Copy link
Contributor

eguzki commented Nov 4, 2023

changes LGTM, just dropped some enhancements that could be IMO interesing. Feel free to skip those.

@grzpiotrowski grzpiotrowski force-pushed the resource-names branch 2 times, most recently from 70c1b13 to 3b0ea2a Compare November 9, 2023 17:12
@alexsnaps alexsnaps added this to the v0.7.0 milestone Nov 13, 2023
@alexsnaps
Copy link
Member

It'd be great to have this in the upcoming release

pkg/upgrades/v0_6_0.go Outdated Show resolved Hide resolved
controllers/limitador_controller.go Show resolved Hide resolved
controllers/limitador_controller.go Outdated Show resolved Hide resolved
controllers/limitador_controller.go Outdated Show resolved Hide resolved
@eguzki
Copy link
Contributor

eguzki commented Nov 15, 2023

it should be rebased on top of current main

@grzpiotrowski grzpiotrowski force-pushed the resource-names branch 3 times, most recently from e90417d to 31f08fb Compare November 17, 2023 11:17
eguzki
eguzki previously approved these changes Nov 20, 2023
@grzpiotrowski grzpiotrowski merged commit 49a4073 into Kuadrant:main Nov 20, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: To test
Development

Successfully merging this pull request may close these issues.

Different naming convention on resources created by limitador-operator
6 participants