search

neondatabase / autoscaling

Postgres vertical autoscaling in k8s
Apache License 2.0
153 stars 21 forks source link

plugin: Replace readClusterState with existing watch events #904

Closed sharnoff closed 4 months ago

sharnoff commented 5 months ago

... and a few other commits tossed in for fun.

Probably best to review commit-by-commit.

The thing I'm most worried about is that this potentially may change buffer handling.

Might be a good idea to go for #840 first, but I suspect that might also have its own issues.

This is a precursor to #517, to minimize the number of places we need to change resource handling.

Omrigan commented 5 months ago

Do you mean these watch events? https://github.com/neondatabase/autoscaling/blob/76ceab257cad96787fa16ee1719fd760bc2ee541/pkg/plugin/plugin.go#L150-L160

I am trying to understand how this relates to #873. For #873 my understanding was to handle those callbacks from pod watcher, and get rid of VM watcher entirely.

sharnoff commented 5 months ago

Do you mean these watch events?

Broadly, watch events like those - but it only relies on the Pod watch events, I think. (and specifically, this PR waits for only the initial Pod "add" events to be completely processed)

I am trying to understand how this relates to #873.

Right, yeah fair. This PR should be ok - we only care about the Pod watch events. (unless I missed something - please feel free to double-check me no that)

sharnoff commented 4 months ago

Realized that the necessity of debe3b22e89af228af757094cad22bb26f316b80 (from #936) is partially due to 25bbe43ce8163d0290cbfb210c3b1a2eb13cb866 here — it adds Buffer always, but there's a handful of circumstances where we don't actually need it (like when a new VM is added but doesn't go through our scheduler)

sharnoff commented 4 months ago

Adapted debe3b22e89af228af757094cad22bb26f316b80 here as c8e250e4ebc0130e001cdd5f196af26ad209ffe7 to fix that issue. Impact should have been close to zero in practice, but wanted to fix it just to be safe. @Omrigan wdyt?

Omrigan commented 4 months ago

Are you going to do rebase merge? If so, tmp commit needs to be squashed somewhere, maybe into middle commit.