Copilot commented on code in PR #13877:
URL: https://github.com/apache/cloudstack/pull/13877#discussion_r3801450205
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -974,6 +974,10 @@ public Pair<List<Backup>, Integer> listBackups(final
ListBackupsCmd cmd) {
sb.and("name", sb.entity().getName(), SearchCriteria.Op.EQ);
sb.and("zoneId", sb.entity().getZoneId(), SearchCriteria.Op.EQ);
sb.and("backupOfferingId", sb.entity().getBackupOfferingId(),
SearchCriteria.Op.EQ);
+ // Tombstoned chain backups (Status.Hidden) are never shown to users;
they exist only so the
+ // incremental chain GC can sweep them once their last descendant is
deleted.
+ sb.and("statusNeq", sb.entity().getStatus(), SearchCriteria.Op.NEQ);
+ sb.and("backupStatus", sb.entity().getStatus(), SearchCriteria.Op.EQ);
Review Comment:
`listBackups` adds a mandatory `backupStatus = ?` criterion
(`sb.and("backupStatus", ...)`) but this method never sets the `backupStatus`
parameter and the API command has no status filter. This can cause the search
to fail or return zero results for all callers. If the intent is only to hide
tombstoned chain members, keep the `statusNeq != Hidden` clause and drop the
unused `backupStatus` filter.
##########
test/integration/smoke/test_backup_recovery_nas.py:
##########
@@ -56,13 +56,17 @@ def setUpClass(cls):
# Check backup configuration values, set them to enable the nas
provider
backup_enabled_cfg = Configurations.list(cls.api_client,
name='backup.framework.enabled')
backup_provider_cfg = Configurations.list(cls.api_client,
name='backup.framework.provider.plugin')
+ incremental_backup_enabled_cfg = Configurations.list(cls.api_client,
name='nas.backup.incremental.enabled')
cls.backup_enabled = backup_enabled_cfg[0].value
cls.backup_provider = backup_provider_cfg[0].value
+ cls.incremental_backup_enabled =
incremental_backup_enabled_cfg[0].value
Review Comment:
`nas.backup.incremental.enabled` is a zone-scoped ConfigKey, but the test
reads it without `zoneid`. If the environment has a zone override, this will
read the wrong value and can make the suite flaky. Pass `zoneid=cls.zone.id`
like other backup tests do.
This issue also appears in the following locations of the same file:
- line 66
- line 99
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
