go-ipam icon indicating copy to clipboard operation
go-ipam copied to clipboard

feat(k8s): Added Kubernetes ConfigMap storage backend

Open ThomasK33 opened this issue 3 months ago • 4 comments

I added a ConfigMap-based storage backend.

In a Kubernetes environment with multiple replicas attempting to perform IPAM at the same time, using in-memory storage can lead to race conditions. Alternatively, using an external database would require managing an additional component. To avoid these issues, ConfigMap-based storage can be used to keep the state in Kubernetes, allowing all replicas to access the same configuration/state.

ThomasK33 avatar Mar 26 '24 13:03 ThomasK33

Hi @ThomasK33,

thanks for you contribution. Some comments on this.

  • i can not see on the first place how this approach prevents race conditions, there is no "leader-election" or similar in the write calls
  • for the sake of simple day-two-operations, we have the https://github.com/metal-stack/backup-restore-sidecar which starts postgres or redis as a persistence backend and creates backups and manages restore in case of catastrophic failures.
  • The tests actually only cover the Create and Update, all other cases are not covered.
  • Why store the prefixes as key in the configmap data, storing all as json would simplify the code and does not require to replace "/" with "-" for example.

WDYT ?

majst01 avatar Mar 26 '24 14:03 majst01

Hey, thanks for the quick feedback.

i can not see on the first place how this approach prevents race conditions, there is no "leader-election" or similar in the write calls

Kubernetes objects have a resourceVersion in their metadata section. Say multiple replicas fetch an object at the same time and then try updating it simultaneously. Then the apiserver will accept one update and reject the two other updates, as those "older" updates do not contain a newer resourceVersion. (This is similar to the optimistic locking in the codebase here.) So, while it's not a leader election per se, it has a distributed write pattern that will fail on parallel writes with older resourceVersion so that a reconciler will eventually retry.

for the sake of simple day-two-operations, we have the metal-stack/backup-restore-sidecar which starts postgres or redis as a persistence backend and creates backups and manages restore in case of catastrophic failures.

Yeah, that looks nice. In this particular case, it's about being able to embed go-ipam into an existing Go application running in a Kubernetes cluster. So, it's not possible to add yet another component.

The tests actually only cover the Create and Update, all other cases are not covered.

Great callout, thanks. I completely missed the testing_test.go file containing all the storage backends and just copied and pasted the unit tests of the in-memory implementation for this one.

I added the config map storage to the testing_test.go file and removed the hand-written unit tests.

Why store the prefixes as key in the configmap data, storing all as json would simplify the code and does not require to replace "/" with "-" for example.

Yep, that was quick & dirty based on the etcd implementation. I've refactored it to store prefixJson maps into configmap keys so that the configmap keys in data represent the namespaces, and the objects inside are a mapping of prefix CIDR --> prefixJson.

ThomasK33 avatar Mar 27 '24 12:03 ThomasK33

Hey @majst01, I just saw that the linter failed at missing json tags:

CleanShot 2024-03-27 at 14 41 55@2x

Would you like me to add JSON tags to all the fields? Also, should those be the Go-style PascalCase or camelCase tags?

ThomasK33 avatar Mar 27 '24 13:03 ThomasK33

Hey @majst01, I just saw that the linter failed at missing json tags:

CleanShot 2024-03-27 at 14 41 55@2x

Would you like me to add JSON tags to all the fields? Also, should those be the Go-style PascalCase or camelCase tags?

I already created a PR for this: #136 so no need for you to take care of that.

majst01 avatar Apr 02 '24 05:04 majst01