Conversation
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
mayankshah1607
requested review from
egegunes,
gkech,
hors,
nmarukovich,
oksana-grishchenko and
pooknull
as code owners
| true, | ||
| }, | ||
| { | ||
| "s3: endpointUrl changed", |
There was a problem hiding this comment.
IsSameStorage does not compare endpoint URL for S3 and Azure, probably because we don't expect them to change (this is not the case for Minio)
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where changing Minio configuration leads to constant storage resync reconciliations. The solution delegates the storage comparison logic to PBM's IsSameStorage method, which is what PBM itself uses to detect when resyncs are needed.
Changes:
- Simplified
isResyncNeededfunction to use PBM'sIsSameStoragemethod instead of manual field comparisons - Removed manual field-by-field comparison logic for all storage types (S3, Minio, GCS, Azure, Filesystem)
- Removed unused
k8s.io/utils/ptrimport - Removed test cases for endpoint URL changes in S3 and Azure that are no longer relevant with the new comparison logic
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/controller/perconaservermongodb/pbm.go | Replaced manual storage comparison with call to IsSameStorage method and removed unused import |
| pkg/controller/perconaservermongodb/pbm_test.go | Removed test cases for S3 and Azure endpoint URL changes that are no longer needed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| true, | ||
| }, | ||
| // TODO: uncomment this when we have PBM 2.13.0 |
There was a problem hiding this comment.
The TODO comment refers to "PBM 2.13.0" but should verify this is the correct version. Consider adding a link to the relevant PBM issue or PR that will include the EndpointURL in the IsSameStorage check for better context and tracking.
| // TODO: uncomment this when we have PBM 2.13.0 | |
| // TODO: uncomment this when PBM includes EndpointURL in IsSameStorage check |
| }, | ||
| true, | ||
| }, | ||
| // TODO: uncomment this when we have PBM 2.13.0 |
There was a problem hiding this comment.
The TODO comment refers to "PBM 2.13.0" but should verify this is the correct version. Consider adding a link to the relevant PBM issue or PR that will include the EndpointURL in the IsSameStorage check for better context and tracking.
| // TODO: uncomment this when we have PBM 2.13.0 | |
| // TODO: uncomment this when PBM includes Azure EndpointURL in the IsSameStorage check |
| }, | ||
| // TODO: uncomment this when we have PBM 2.13.0 | ||
| // { | ||
| // "azure: endpointUrl changed", |
There was a problem hiding this comment.
IMO instead of commenting out we can do
if tt.skip {
t.Skip()
}
The mean reason for not commenting out for me is to protect the test from a refactor and rename a type or field with the code silently becoming stale. Skipped tests still compile, so we catch changes.
Skipped tests are also visible to the test output.
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
CHANGE DESCRIPTION
Problem:
Changing minio config leads to constant storage resync reconciliations
Solution:
Use
IsSameStoragesince that's what PBM uses to detect resyncs - https://github.com/percona/percona-backup-mongodb/blob/968f87a3b1763abe700e4a6dedd168672019aa18/cmd/pbm/config.go#L163-L165CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability