operator: check config reload happened before completing reconcile#2399
operator: check config reload happened before completing reconcile#2399AndrewChubatiuk wants to merge 1 commit into
Conversation
AndrewChubatiuk
commented
Jul 13, 2026
AndrewChubatiuk
commented
- check config reload event happened before marking reconcile as complete for vmagent, vmauth, vmalertmanager and vmalert
- force waiting for vmauth config reload event in vmdistributed
94c8719 to
181498d
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
181498d to
399ff3a
Compare
|
@cubic-ai-dev |
@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete. |
f10eaee to
9a303fb
Compare
There was a problem hiding this comment.
All reported issues were addressed
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
1d64c4f to
b5c70ae
Compare
3825d28 to
7cc4bcf
Compare
| // GetConfigReloadMetricNames returns metrics, that vmagent exposes for config-reload completion check | ||
| func (cr *VMAgent) GetConfigReloadMetricNames() []string { | ||
| return []string{ | ||
| "vm_promscrape_config_last_reload_success_timestamp_seconds", |
There was a problem hiding this comment.
We may want to add those as consts and reuse them here - it would prevent us from making typos here
There was a problem hiding this comment.
there's only one metric vm_promscrape_config_last_reload_success_timestamp_seconds, which is used in multiple places. not sure consts make sense
There was a problem hiding this comment.
Right, in that case its probably extra. I wonder if we have e2e tests for these to prevent us from referencing metric names which don't exist
There was a problem hiding this comment.
i can add a warning to logs that metric doesn't exist and exit from check
| // that the merged config was written to a Secret. It's a no-op unless | ||
| // WaitForConfigReload is enabled on the CR and the CR type reports a | ||
| // config-reload-completion metric. | ||
| func WaitForConfigReload(ctx context.Context, rclient client.Client, cr configReloadWaitable, after time.Time) error { |
There was a problem hiding this comment.
We should pass metric names here (and default to cr.GetConfigReloadMetricNames if unset). This would enable us to check exact metric reload time (in case multiple events are fired within one second). I think this is what https://github.com/VictoriaMetrics/operator/pull/2399/changes#r3569190534 is about too
There was a problem hiding this comment.
sorry, don't get your idea
There was a problem hiding this comment.
If I read this correctly whenever one secret is updated we'll check for all metrics - although we could check for just one metric in some cases. It could help us when multiple metrics are being updated simultaneously and I think there is a situation where we'll issue an extra reload
289cb8a to
6aa74ac
Compare
6aa74ac to
f39a29b
Compare
2 participants
