fix: remove Backup owner field indexer from ScheduledBackup controller#9466
Closed
andrew-farries wants to merge 1 commit intocloudnative-pg:mainfrom
Closed
fix: remove Backup owner field indexer from ScheduledBackup controller#9466andrew-farries wants to merge 1 commit intocloudnative-pg:mainfrom
Backup owner field indexer from ScheduledBackup controller#9466andrew-farries wants to merge 1 commit intocloudnative-pg:mainfrom
Conversation
Contributor
|
❗ By default, the pull request is configured to backport to all release branches.
|
Member
|
/test feature_type=backup-restore, |
Contributor
|
@sxd, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/20308164589 |
Member
|
/test |
Contributor
|
@sxd, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/20308221186 |
94311f8 to
9442b07
Compare
4 tasks
Remove the indexer on Backup resources that allowed finding all Backups owned by a ScheduledBackup. The indexer never worked because it was checking for owners of kind `Backup` instead of `ScheduledBackup`. Even if it had correctly looked for owners of kind `ScheduledBackup`, it would only have worked for the cases where the `ScheduledBackup` was configured with: ``` backupOwnerReference: self ``` When `backupOwnerReference` is set to `none` (the default) or `cluster` this indexer would not have indexed anything. The Backup controller handles concurrency by itself by checking for running Backups before starting a new one, so rather than fixing this indexer we simply remove it. Signed-off-by: Andrew Farries <andrew.farries@xata.io>
armru
added a commit
that referenced
this pull request
Dec 18, 2025
…ed indexer The ScheduledBackup controller was using a broken field indexer on owner references to find child Backups for concurrency checking. The indexer checked for the wrong owner kind, making it non-functional. This fix replaces the owner reference-based indexer with a more robust approach: a field indexer on the ParentScheduledBackupLabel that is already set on all backups created by ScheduledBackup. This label is always present regardless of the backupOwnerReference configuration. Closes #9466 Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
9442b07 to
fceb696
Compare
Member
|
Given that this patch fixes the issue but also alter the behaviour (feat) I would propose to use the more conservative: #9489 approach |
mnencia
pushed a commit
that referenced
this pull request
Dec 22, 2025
…ed indexer The ScheduledBackup controller was using a broken field indexer on owner references to find child Backups for concurrency checking. The indexer checked for the wrong owner kind, making it non-functional. This fix replaces the owner reference-based indexer with a more robust approach: a field indexer on the ParentScheduledBackupLabel that is already set on all backups created by ScheduledBackup. This label is always present regardless of the backupOwnerReference configuration. Closes #9466 Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Member
Contributor
|
❗ By default, the pull request is configured to backport to all release branches.
|
leonardoce
pushed a commit
that referenced
this pull request
Dec 23, 2025
…ed indexer The ScheduledBackup controller was using a broken field indexer on owner references to find child Backups for concurrency checking. The indexer checked for the wrong owner kind, making it non-functional. This fix replaces the owner reference-based indexer with a more robust approach: a field indexer on the ParentScheduledBackupLabel that is already set on all backups created by ScheduledBackup. This label is always present regardless of the backupOwnerReference configuration. Closes #9466 Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
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
5 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.
Remove the indexer on
Backupresources that allowed finding allBackupsowned by aScheduledBackup.The indexer never worked because it was checking
Backupsfor owner references of kindBackupinstead ofScheduledBackup:cloudnative-pg/internal/controller/scheduledbackup_controller.go
Lines 350 to 352 in e2d6bd0
This check meant that the indexer never returned any results and so the
GetChildBackupsmethod always returned an empty slice.Even if it had correctly looked for owners of kind
ScheduledBackup, it would only have worked for the cases where theScheduledBackupwas configured with:When
backupOwnerReferencewas set tonone(the default) orclusterthis indexer would also not have returned anything.The intended purpose of the
GetChildBackupsmethod that used the indexer was to prevent concurrentBackupsfor aScheduledBackup. The check never worked and theBackupcontroller handles concurrency itself by checking for runningBackupsbefore starting a new one. So rather than fixing this indexer it is better to simply remove it.