neonvm-controller: make IPAM code production-ready #1278

Omrigan · 2025-02-21T14:33:34Z

Split into commits, to be merged with rebase.

Fixes #1281

github-actions · 2025-02-21T14:37:29Z

No changes to the coverage.

HTML Report

Click to open

cicdteam · 2025-02-21T16:09:14Z

I haven't gone into the changes in depth, but I just want to ask what issues are being addressed?

Omrigan · 2025-02-23T13:53:45Z

I haven't gone into the changes in depth, but I just want to ask what issues are being addressed?

Key issues are:

Leader election times out when the rate of allocations is significant.
When a release of an IP fails, it is not retried, so we have 13k stale allocations.
k8s client is recreated on every request.

Plus a few other things.

sharnoff

Partial review, just skimming through -- broadly looks good, need to dig a little deeper into the new core of the ipam actions.

One thing that I'm not sure about: What happens if we get a lot of reconcile operations simultaneously waiting on ipam operations? Should we have some mechanism to immediately bail if there's too many other threads also waiting? (so that, under load, other operations don't completely grind to a halt as well)

neonvm-controller/cmd/main.go

pkg/neonvm/controllers/vm_controller.go

pkg/neonvm/ipam/ipam.go

Omrigan

Applied suggestions from Em

pkg/neonvm/controllers/vm_controller.go

pkg/neonvm/ipam/ipam.go

pkg/neonvm/ipam/allocate.go

pkg/neonvm/ipam/ipam.go

neonvm-controller/cmd/main.go

sharnoff · 2025-02-27T18:16:50Z

(assigned you for now @Omrigan, please feel free to assign back to me after resolving comments if you think it's ready for another round of review)

sharnoff

More thorough review, lots of comments -- broadly looks ok. If there's smaller commits that I haven't commented on, I'm happy to approve them separately if that would help (reducing surface area of the diff, etc.)

go.mod

neonvm-controller/cmd/main.go

pkg/neonvm/controllers/vm_controller.go

pkg/neonvm/ipam/allocate.go

pkg/neonvm/ipam/ipam.go

Leader election is too heavy to run it on every IP allocation. We can achieve the same with a leader election on neonvm-controller + a local mutex. Signed-off-by: Oleg Vasilev <[email protected]>

We must not leak IP addresses. The release will be retried. Also, we have to make it idempotent. Signed-off-by: Oleg Vasilev <[email protected]>

Leader election is too heavy to run it on every IP allocation. We can achieve the same with a leader election on neonvm-controller + a local mutex. Signed-off-by: Oleg Vasilev <[email protected]>

We will know if it is broken when we try to use it. Signed-off-by: Oleg Vasilev <[email protected]>

Non-functional change. Signed-off-by: Oleg Vasilev <[email protected]>

Two things: 1. Use NamespacedName. 2. Make the loop less verbose Signed-off-by: Oleg Vasilev <[email protected]>

This supresses the following warning, which would be introduced by the next commit: exitAfterDefer: os.Exit will exit, and `defer ipam.Close()` will not run Signed-off-by: Oleg Vasilev <[email protected]>

There is no need to create it for every request. Signed-off-by: Oleg Vasilev <[email protected]>

Non-functional change. Preparation to have a cleanup action. Signed-off-by: Oleg Vasilev <[email protected]>

Signed-off-by: Oleg Vasilev <[email protected]>

The previous version is marked deprecated. Signed-off-by: Oleg Vasilev <[email protected]>

Otherwise all reconcilliation workers can be stuck Signed-off-by: Oleg Vasilev <[email protected]>