canonical / prometheus-k8s-operator

https://charmhub.io/prometheus-k8s
Apache License 2.0
21 stars 35 forks source link

Less frequent reloads and restarts #352

Closed sed-i closed 2 years ago

sed-i commented 2 years ago

Issue

Before this change, reload/restart could result in unnecessary down time.

Solution

Fixes #342.

Context

NTA.

Testing Instructions

Afaict, the feature is fully captured in the newly added utests. Please review those for completeness!

sed-i commented 2 years ago

@dstathis here's a summary of StoredState vs pull for determining whether a reload is needed.

StoredState Pull
Multiple files Easy to calculate hash Need to pull (and sort?) all data (config, certs AND alerts)
Tinkering Manual changes persist Manual changes disappear every _configure
Robustness Only push Pull (multiple files) then push

Given the above I prefer StoredState. Did I miss anything?

dstathis commented 2 years ago

@dstathis here's a summary of StoredState vs pull for determining whether a reload is needed.

StoredState Pull
Multiple files Easy to calculate hash Need to pull (and sort?) all data (config, certs AND alerts)
Tinkering Manual changes persist Manual changes disappear every _configure
Robustness Only push Pull (multiple files) then push

Given the above I prefer StoredState. Did I miss anything?

I'm not sure I understand why we would lose local changes. I will go ahead and approve though.

sed-i commented 2 years ago

Patching of a bunch of ops.testing._TestingPebbleClient internals gives me bad feelings, though. Do we really need this huge amount of mock patching?

That's the least evil I could come up with at the time. Open for other suggestions.