Feat: Ignore revisionHistoryLimit field in StatefulSet for Prometheus, Alertmanager& ThanosRuler

[Azanul]

Description

Fixes #5712

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

• CHANGE (fix or feature that would cause existing functionality to not work as expected)
• FEATURE (non-breaking change which adds functionality)
• BUGFIX (non-breaking change which fixes an issue)
• ENHANCEMENT (non-breaking change which improves existing functionality)
• NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

Ignore updates revisionHistoryLimit field in the underlying StatefulSet allowing external services to manage revisionHistoryLimit

[Azanul]

@rouke-broersma @simonpasquier

[rouke-broersma]

My issue is with alertmanager, could you add the same for them?

[simonpasquier]

I'm not sure that we need to expose a revisionHistoryLimit field in the existing CRDs. IIUC the issue is that the Kubernetes API always returns a default value of 10 in the statefulset spec and if something/someone modifies the value then createSSetInputHash() generates a new hash value triggering a statefulset update.
I assume that if createSSetInputHash sets RevisionHistoryLimit to nil before generating the sts hash, it would solve the issue too.

[Azanul]

if createSSetInputHash sets RevisionHistoryLimit to nil before generating the sts hash, it would solve the issue too.

Purposefully ignoring any changes to RevisionHistoryLimit, I think that will work.

[Azanul]

@simonpasquier Why does the spec goes as an interface in Thanos createSSetInputHash?

[simonpasquier]

@simonpasquier Why does the spec goes as an interface in Thanos createSSetInputHash?

No concrete reason I believe. bd658c5#diff-c8f4eb63862907c2cd9e067b2cf70e03f4e134075bd28f507dade553850d4fa3L1627 is where we changed it for Prometheus.
And I'd even recommend to apply the same change to Thanos Ruler as it should help reducing the number of sync loops.

[Azanul]

I'd even recommend to apply the same change to Thanos Ruler as it should help reducing the number of sync loops.

Shall I do it in the same PR?

[rouke-broersma]

I'm not sure that we need to expose a revisionHistoryLimit field in the existing CRDs. IIUC the issue is that the Kubernetes API always returns a default value of 10 in the statefulset spec and if something/someone modifies the value then createSSetInputHash() generates a new hash value triggering a statefulset update.
I assume that if createSSetInputHash sets RevisionHistoryLimit to nil before generating the sts hash, it would solve the issue too.

I agree but note that it was your own comment that led to this solution instead of just ignoring the field 😜

#5712 (comment)

[simonpasquier]

@rouke-broersma yes I sometimes contradicts myself ;)

[Azanul]

Shall I do it in the same PR?

@simonpasquier

[simonpasquier]

@Azanul sorry for the late reply. Can you open another PR to update the Thanos Ruler's createSSetInputHash() method? I'd prefer to merge it first then you can rebase this PR...

[simonpasquier]

can you do the same change here?

[Azanul]

@Azanul sorry for the late reply. Can you open another PR to update the Thanos Ruler's createSSetInputHash() method? I'd prefer to merge it first then you can rebase this PR...

check out #5784

[Azanul]

@simonpasquier You didn't really have to point out comment's every instance, I'd have fixed it before re-requesting review 😅

[simonpasquier]

Thanks!