Disk storage #96

eguzki · 2023-09-04T16:25:00Z

What

Deploy limitador with disk (persistent) storage.

Simplest deployment option for disk storage

apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador-sample
spec:
  storage: 
    disk: {}

TODO:

Doc
Fix existing tests
Add new tests
Discuss about replicas

Verification Steps

Simplest deployment

dev setup

make local-setup

Deploy the limitador CR with storage set to disk

k apply -f - <<EOF
---
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador-sample
spec:
  storage: 
    disk: {}
EOF

Check deployment is correct

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

Should return

limitador.limitador.kuadrant.io/limitador-sample condition met

Check command line

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

should look like this

- limitador-server
- /home/limitador/etc/limitador-config.yaml
- disk
- /var/lib/limitador/data

By default, 1Gi persistent volume is booked for limitador

kubectl get pvc

Should get

NAME                         STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
limitador-limitador-sample   Bound    pvc-3296fc91-8f2d-4f8a-b24b-3cba99304abe   1Gi        RWO            standard       28s

eguzki · 2023-09-04T16:52:56Z

api/v1alpha1/limitador_types.go

@@ -317,18 +321,3 @@ func (s *LimitadorStatus) Equals(other *LimitadorStatus, logger logr.Logger) boo
 func init() {
 	SchemeBuilder.Register(&Limitador{}, &LimitadorList{})
 }
-
-type PodDisruptionBudgetType struct {


moving this type to have func init() { as the last function in the file. Just personal taste.

eguzki · 2023-09-04T16:54:28Z

controllers/limitador_controller.go

@@ -248,16 +268,6 @@ func (r *LimitadorReconciler) reconcileLimitsConfigMap(ctx context.Context, limi
 	return nil
 }

-// SetupWithManager sets up the controller with the Manager.
-func (r *LimitadorReconciler) SetupWithManager(mgr ctrl.Manager) error {


moving this function to the end of the file. Just personal taste.

pkg/limitador/k8s_objects.go

codecov-commenter · 2023-09-08T11:23:28Z

Codecov Report

Merging #96 (5e81a02) into main (48f950c) will increase coverage by 14.82%.
The diff coverage is 91.22%.

@@             Coverage Diff             @@
##             main      #96       +/-   ##
===========================================
+ Coverage   53.97%   68.80%   +14.82%     
===========================================
  Files          12       14        +2     
  Lines         867      984      +117     
===========================================
+ Hits          468      677      +209     
+ Misses        363      267       -96     
- Partials       36       40        +4

Flag	Coverage Δ
integration	`66.06% <70.17%> (+7.54%)`	⬆️
unit	`70.19% <97.07%> (+18.76%)`	⬆️

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)	`44.19% <90.47%> (+17.59%)`	⬆️
pkg/limitador (u)	`88.46% <97.82%> (+16.44%)`	⬆️
controllers (i)	`66.06% <70.17%> (+7.54%)`	⬆️

Files	Coverage Δ
api/v1alpha1/limitador_types.go	`100.00% <ø> (ø)`
pkg/limitador/deployment_options.go	`100.00% <100.00%> (ø)`
pkg/limitador/disk_storage_options.go	`100.00% <100.00%> (ø)`
pkg/limitador/inmemory_storage_options.go	`100.00% <100.00%> (ø)`
pkg/limitador/redis_cache_storage_options.go	`100.00% <100.00%> (ø)`
pkg/reconcilers/deployment.go	`66.30% <100.00%> (+44.38%)`	⬆️
pkg/limitador/redis_storage_options.go	`96.66% <96.66%> (ø)`
pkg/reconcilers/base_reconciler.go	`33.92% <0.00%> (-0.62%)`	⬇️
pkg/limitador/k8s_objects.go	`81.37% <94.23%> (+9.64%)`	⬆️
controllers/limitador_controller.go	`60.94% <70.17%> (+10.23%)`	⬆️

... and 4 files with indirect coverage changes

eguzki · 2023-09-19T14:12:51Z

ready for review @Kuadrant/engineering

api/v1alpha1/limitador_types.go

didierofrivia · 2023-09-21T14:43:24Z

api/v1alpha1/limitador_types.go

+	PVC *PVCGenericSpec `json:"persistentVolumeClaim,omitempty"`
+
+	// +optional
+	Optimize *DiskOptimizeType `json:"optimize,omitempty"`


maybe we can add
// +kubebuilder:default=throughput

I would rather rely on Limitador's own defaults. If optimize is not set in the CR, it would not go in the command line and defaults would be applied by Limitador, not Limitador Operator.

bundle/manifests/limitador-operator.clusterserviceversion.yaml

adam-cattermole

Took me a little while to go through all the changes here but I've run through the verification and the code. I think it looks good from my side but perhaps you'd prefer a second review. Either way I'll leave my approve, nice job!

eguzki · 2023-09-28T17:29:43Z

Took me a little while to go through all the changes here but I've run through the verification and the code. I think it looks good from my side but perhaps you'd prefer a second review. Either way I'll leave my approve, nice job!

Thanks @adam-cattermole

Sorry for the "too big" PR. I refactored the options to have a cleaner deployment objects. I think that the outcome is a better code to extend and maintain.

There is no rush. Let's wait for the review from @didierofrivia

…ymentvolumemounts test

…e test

eguzki · 2023-10-02T08:12:54Z

rebased to fix merge conflicts (in the CSV)

didierofrivia

Great work! I've also tried it with optimize: throughput and the command sent to the limitador container was as expected. Also, the validation works when a different value than thoughput or disk is set. 🥇

eguzki requested review from didierofrivia and a team September 4, 2023 16:25

eguzki changed the title ~~Storage disk~~ Disk storage Sep 4, 2023

eguzki commented Sep 4, 2023

View reviewed changes

eguzki commented Sep 7, 2023

View reviewed changes

pkg/limitador/k8s_objects.go Outdated Show resolved Hide resolved

eguzki force-pushed the storage-disk branch from 1bbfc55 to 2d2ecf7 Compare September 8, 2023 11:19

eguzki force-pushed the storage-disk branch from 6394fae to 36819bf Compare September 8, 2023 13:31

alexsnaps mentioned this pull request Sep 11, 2023

rfc: configuration of kuadrant sub components Kuadrant/architecture#25

Merged

alexsnaps added the target/current label Sep 12, 2023

eguzki force-pushed the storage-disk branch from a50bcb6 to 1eca019 Compare September 19, 2023 12:20

eguzki marked this pull request as ready for review September 19, 2023 13:24

eguzki force-pushed the storage-disk branch from b5ba1ee to 89fb4b6 Compare September 19, 2023 14:01

didierofrivia reviewed Sep 21, 2023

View reviewed changes

api/v1alpha1/limitador_types.go Show resolved Hide resolved

didierofrivia reviewed Sep 21, 2023

View reviewed changes

adam-cattermole reviewed Sep 26, 2023

View reviewed changes

bundle/manifests/limitador-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

eguzki requested review from adam-cattermole and pehala September 26, 2023 16:51

adam-cattermole approved these changes Sep 28, 2023

View reviewed changes

eguzki requested a review from didierofrivia September 28, 2023 17:27

eguzki added 7 commits October 2, 2023 10:08

disk storage

cee95c1

deployment volumes and volumemounts mutator

968fb1f

update bundles

a75a6e8

fix rebase effects

8dbf51b

fix default disk pvc resources

f25ec7a

fix tests

17abe82

validation to reject multiples replicas when disk storage enabled

ba41e57

eguzki added 17 commits October 2, 2023 10:10

fix lint issues

121a140

pkg/reconcilers/deployment_test.go: volume and volumemount mutator test

49f5a1f

pkg/reconcilers/deployment_test.go: command mutator test

7760076

pkg/reconcilers/deployment_test.go: deployment generic mutator test

05e3231

pkg/limitador/deployment_options_test.go: deploymentvolumes and deplo…

1091f09

…ymentvolumemounts test

pkg/limitador/disk_storage_options_test.go disk storage test

096668b

pkg/limitador/inmemory_storage_options_test.go: inmemory storage test

ebdafb3

pkg/limitador/redis_storage_options_test.go: redis storage test

c1fe752

pkg/limitador/redis_cache_storage_options_test.go redis cached storag…

499daab

…e test

pkg/limitador/k8s_objects_test.go: objects tests

0bb1fdc

little enhancement in PVC

ae7d386

fix lint issues

3b37e34

minKubeVersion: 1.26.0

aa07abc

controllers/limitador_controller_test.go integration tests

1ada0f4

doc: disk storage

be49f00

deployment strategy recreated when storage disk

9121833

kubernetes min version 1.25.0

5e81a02

eguzki force-pushed the storage-disk branch from b59e909 to 5e81a02 Compare October 2, 2023 08:12

didierofrivia approved these changes Oct 2, 2023

View reviewed changes

eguzki merged commit c76af84 into main Oct 2, 2023

eguzki deleted the storage-disk branch October 2, 2023 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disk storage #96

Disk storage #96

eguzki commented Sep 4, 2023 •

edited

Loading

eguzki Sep 4, 2023

eguzki Sep 4, 2023

codecov-commenter commented Sep 8, 2023 •

edited

Loading

eguzki commented Sep 19, 2023

didierofrivia Sep 21, 2023

eguzki Sep 25, 2023

adam-cattermole left a comment

eguzki commented Sep 28, 2023

eguzki commented Oct 2, 2023

didierofrivia left a comment

Disk storage #96

Disk storage #96

Conversation

eguzki commented Sep 4, 2023 • edited Loading

What

Verification Steps

Simplest deployment

eguzki Sep 4, 2023

Choose a reason for hiding this comment

eguzki Sep 4, 2023

Choose a reason for hiding this comment

codecov-commenter commented Sep 8, 2023 • edited Loading

Codecov Report

eguzki commented Sep 19, 2023

didierofrivia Sep 21, 2023

Choose a reason for hiding this comment

eguzki Sep 25, 2023

Choose a reason for hiding this comment

adam-cattermole left a comment

Choose a reason for hiding this comment

eguzki commented Sep 28, 2023

eguzki commented Oct 2, 2023

didierofrivia left a comment

Choose a reason for hiding this comment

eguzki commented Sep 4, 2023 •

edited

Loading

codecov-commenter commented Sep 8, 2023 •

edited

Loading