fix(operator): set ingester minimum available pods based on replication factor #19517
+106
−27
Conversation
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
btaani
requested review from
a team,
JoaoBraveCoding,
periklis and
xperimental
as code owners
|
Bayan Taani seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
btaani
changed the title
| } | ||
| } | ||
|
|
||
| func GetPDBMinAvailable(opts Options) (error, intstr.IntOrString) { |
Collaborator
There was a problem hiding this comment.
I think we could move this function inside newIngesterPodDisruptionBudget.
added 2 commits
October 21, 2025 12:32
Collaborator
JoaoBraveCoding
left a comment
JoaoBraveCoding
left a comment
There was a problem hiding this comment.
Reminder to:
- squash the commits to comply with the CLA signature
- make sure to
make testsbefore pushing some tests were failing
Collaborator
There was a problem hiding this comment.
You have to run make web-pre in order to generate the new documentation for this API change
| ErrSummaryAnnotationMissing = errors.New("rule requires annotation: summary") | ||
| // ErrDescriptionAnnotationMissing indicates that an alerting rule is missing the description annotation | ||
| ErrDescriptionAnnotationMissing = errors.New("rule requires annotation: description") | ||
| // ErrReplicationFactorTooHigh indicates that the lokistack's replication factor must always be less than the number of ingester replicas |
Collaborator
There was a problem hiding this comment.
Suggested change
| // ErrReplicationFactorTooHigh indicates that the lokistack's replication factor must always be less than the number of ingester replicas | |
| // ErrReplicationFactorTooHigh indicates that the configured Loki replication factor must always be less than the number of ingester replicas |
| ReasonZoneAwareEmptyLabel LokiStackConditionReason = "ReasonZoneAwareEmptyLabel" | ||
| // ReasonStorageNeedsSchemaUpdate when the object storage schema version is older than V13 | ||
| ReasonStorageNeedsSchemaUpdate LokiStackConditionReason = "StorageNeedsSchemaUpdate" | ||
| // ReasonInvalidReplicationFactor when the replication factor is equal to or more than the component replicas |
Collaborator
There was a problem hiding this comment.
Suggested change
| // ReasonInvalidReplicationFactor when the replication factor is equal to or more than the component replicas | |
| // ReasonInvalidReplicationFactor when the replication factor is equal to or bigger than the ingrester replicas |
| objects, err := manifests.BuildAll(opts) | ||
| if err != nil { | ||
| if errors.Is(err, lokiv1.ErrReplicationFactorTooHigh) { | ||
| return nil, &status.DegradedError{ |
Collaborator
There was a problem hiding this comment.
We can't return the status.DegradedErrror in operator/internal/manifests/ingester.go because of an import cycle, right? I wonder if we should just move the definition of status.DegradedError around to allow package manifests to use it.
cc @xperimental any thoughts?
Comment on lines
+299
to
313
| pdbMinAvailable := intstr.FromInt32(int32(1)) | ||
| switch opts.Stack.Size { | ||
| case lokiv1.SizeOneXPico, lokiv1.SizeOneXMedium: | ||
| // For these sizes, default Replication.Factor = 2 and Ingester.Replicas = 3 | ||
| if opts.Stack.Template.Ingester.Replicas <= opts.Stack.Replication.Factor { | ||
| return nil, lokiv1.ErrReplicationFactorTooHigh | ||
| } | ||
| pdbMinAvailable = intstr.FromInt32(opts.Stack.Replication.Factor) | ||
| case lokiv1.SizeOneXExtraSmall, lokiv1.SizeOneXSmall: | ||
| // For these sizes, default Replication.Factor = 2 and Ingester.Replicas = 2 | ||
| // Therefore set the pdbMinAvailable to 1 to keep 1 spare pod for rolling updates | ||
| if opts.Stack.Template.Ingester.Replicas > opts.Stack.Replication.Factor { | ||
| pdbMinAvailable = intstr.FromInt32(opts.Stack.Replication.Factor - 1) | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
In the current way we would always skip the validation for sizes lokiv1.SizeOneXExtraSmall, lokiv1.SizeOneXSmall this new way isolates it to just the default values scenario
Suggested change
| pdbMinAvailable := intstr.FromInt32(int32(1)) | |
| switch opts.Stack.Size { | |
| case lokiv1.SizeOneXPico, lokiv1.SizeOneXMedium: | |
| // For these sizes, default Replication.Factor = 2 and Ingester.Replicas = 3 | |
| if opts.Stack.Template.Ingester.Replicas <= opts.Stack.Replication.Factor { | |
| return nil, lokiv1.ErrReplicationFactorTooHigh | |
| } | |
| pdbMinAvailable = intstr.FromInt32(opts.Stack.Replication.Factor) | |
| case lokiv1.SizeOneXExtraSmall, lokiv1.SizeOneXSmall: | |
| // For these sizes, default Replication.Factor = 2 and Ingester.Replicas = 2 | |
| // Therefore set the pdbMinAvailable to 1 to keep 1 spare pod for rolling updates | |
| if opts.Stack.Template.Ingester.Replicas > opts.Stack.Replication.Factor { | |
| pdbMinAvailable = intstr.FromInt32(opts.Stack.Replication.Factor - 1) | |
| } | |
| } | |
| pdbMinAvailable := intstr.FromInt32(1) | |
| // Because by default both 1x.extra-small and 1x.small have replication | |
| // factor of 2 and ingester replicas of 2, they would always trip the error | |
| // condition so until we reevaluate the default values, we skip the check and | |
| // keep pdbMinAvailable at 1 to keep 1 spare pod for rolling updates. | |
| if (opts.Stack.Size != lokiv1.SizeOneXExtraSmall && opts.Stack.Size != lokiv1.SizeOneXSmall) || | |
| opts.Stack.Template.Ingester.Replicas != 2 || opts.Stack.Replication.Factor != 2 { | |
| if opts.Stack.Template.Ingester.Replicas <= opts.Stack.Replication.Factor { | |
| return nil, lokiv1.ErrReplicationFactorTooHigh | |
| } | |
| pdbMinAvailable = intstr.FromInt32(opts.Stack.Replication.Factor) | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 / why we need it:
Set the default value of the ingester's PodDisruptionBudget's minimum available pods to the replication factor
Which issue(s) this PR fixes:
Fixes LOG-6715
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR