Repository navigation
[Backport release-1.6] fix(monitoring): spread monitoring replicas over nodes - #4991
Open
europrinter (yankawai) wants to merge 4 commits into
Open
europrinter (yankawai) wants to merge 4 commits into
europrinter (yankawai) wants to merge 4 commits into
Conversation
The VMCluster, VLCluster, VTCluster and VMAlertmanager the chart renders carry no affinity and no topology spread, and vm-operator copies both fields onto the pods as written without adding any of its own. Nothing in those specs kept two replicas of one tier off the same node. With replicationFactor 2 both copies of a series can then sit on one node, and on a node-local class the volumes keep them there, so losing that node loses both copies. A StatefulSet pod on a lost node is not replaced elsewhere until its Pod or Node object is deleted, which holds for the Alertmanager replicas without a volume as well. The tiers that run as StatefulSets, vmstorage, vmselect, vlstorage, vtstorage and the Alertmanager, now get a per-node spread with whenUnsatisfiable DoNotSchedule and nodeTaintsPolicy Ignore. The tiers that run as Deployments, vminsert, vlinsert, vlselect, vtinsert and vtselect, get the same spread with ScheduleAnyway, since their pods are replaced on another node without help. A platform that sets scheduling.globalAppTopologySpreadConstraints gets that policy for every tier instead, with the tier's own selector added to each constraint that has none. vm-operator rolls a changed StatefulSet one pod at a time. A replica on a node-local volume whose peer shares its node cannot be placed again under the new rule, so its new pod stays Pending and the rollout of that cluster stops there until the replica is moved. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]> (cherry picked from commit a4cb00e)
release-1.6 has no VictoriaTraces backend, so the suite picked from main cannot render templates/vtraces/vtraces.yaml there, and its two VTCluster cases fail with document not found. The other nine cases stay as they are. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/L
A globalAppTopologySpreadConstraints value that did not parse, or that held no topologySpreadConstraints list, gave every tier the chart default without any sign, so the platform's policy was silently not applied. The render now fails and names the parse error, as the chart already does for an empty logsStorages. The tests also pin that a selector the policy sets itself is kept as written. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]> (cherry picked from commit 0dbbb3b)
The helper gave a platform constraint the tier's selector through a merge inside ternary, and ternary evaluates both branches, so the merge also ran for a constraint that brings a selector of its own. merge writes into its first argument and recurses into the maps both sides hold. The first argument held the shared $selector map, so the labels or matchExpressions of that own selector were written into $selector itself. Every constraint without a selector holds that same map and the list is rendered once at the end, so all of them came out with the foreign terms added, before or after the constraint that brought them. Such a constraint selected only those of the tier's pods that also match the other selector, and on pods without those labels it spread nothing. Only a policy that mixes constraints with and without a selector is affected. The default and other policies render as before. The merge now goes into a deep copy of the constraint, the order the keycloak chart uses for its database constraints, so $selector is never written. Two new cases pin a constraint without a selector after and before one with its own, the first with matchLabels and the second with matchExpressions. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]> (cherry picked from commit 33783b4)
1 of 12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Backport of #4989 to
release-1.6, in four commits. The first, the third and the fourth are cherry-picked from a4cb00e, 0dbbb3b and 33783b4 with their cherry-pick references.vmcluster.yamlandvlogs.yamlare the same on this branch andalerta.yamldiffers in one unrelated line, so those hunks apply as they are.templates/vtraces/vtraces.yamldoes not exist here, and the conflict on it was resolved by leaving it out. The second commit drops the two VictoriaTraces cases from the new suite, which cannot render on this branch. The third makes a malformedglobalAppTopologySpreadConstraintsfail the render, and the fourth stops a selector that a policy constraint sets itself from being merged into the selector the chart adds to the other constraints. Both apply without conflicts.The vmstorage, vmselect and vlstorage replicas and the Alertmanager get a per-node spread with
whenUnsatisfiable: DoNotScheduleandnodeTaintsPolicy: Ignore, vminsert, vlinsert and vlselect the same spread withScheduleAnyway, and a setglobalAppTopologySpreadConstraintsreplaces both. This branch ships the same vm-operator v0.68.4 as main, so the upgrade behaviour and the cost next to a tainted node described in #4989 apply unchanged. The operator replaces the pods of each changed StatefulSet one at a time. A replica on a node-local volume whose peer of the same tier shares its node cannot be placed again, so its new pod stays Pending and the rollout of that VMCluster or VLCluster stops there until the replica is moved, which means recreating its volume. In a patch release that is worth a line in the upgrade notes. Two of the clusters where we found the replicas sharing nodes run v1.6.5 and one runs v1.6.4, which also renders these resources without a placement rule.On
release-1.6the same command gives 9 suites and 167 tests. Against its templates the new suites fail all 16 cases, and against def0688 only the two new ones. Each of the 18 mutations listed in #4989 that apply here fails at least one case. With default valueshelm templatediffers fromrelease-1.6only in the newtopologySpreadConstraintsblocks, apart from the alerta key and Grafana password the chart generates at random, and the added template lines are byte for byte those of #4989. The rendered specs decode strictly into the operator's v0.68.4 API types, and each selector matches only its own component's pod labels.Screenshots
Not a UI change.
Downstream repositories
Same templates and tests as #4989 without the VictoriaTraces parts, so no trigger map entry applies.
Release note