K8SPSMDB-1488 | minio configuration changes are not synced with PBM#2189
K8SPSMDB-1488 | minio configuration changes are not synced with PBM#2189mayankshah1607 merged 16 commits intomainfrom
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
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
please add description |
There was a problem hiding this comment.
Pull request overview
This PR refactors the isResyncNeeded function to fix an issue where Minio configuration changes were not properly synced with PBM (Percona Backup for MongoDB). The refactoring replaces explicit field-by-field comparisons with cleaner switch-case logic that delegates to Equal methods provided by the storage package.
Changes:
- Simplified the
isResyncNeededfunction by replacing verbose field comparisons with Equal method calls for all storage types - Added support for Minio storage type configuration change detection
- Added import for
github.com/percona/percona-backup-mongodb/pbm/storagepackage
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
mayankshah1607
force-pushed
the
K8SPSMDB-1488-refactor
branch
from
85233ce to
020475f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 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 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/psmdb/backup/pbm.go:359
- The ForcePathStyle field is missing from GetPBMStorageMinioConfig, but it's checked in isResyncNeeded (line 237 of pbm.go) and exists in the BackupStorageMinioSpec struct. This field should be added to the Minio config initialization, similar to how it's set for S3 on line 408. Without this, changes to ForcePathStyle won't be properly synced to PBM even though the resync check will detect them.
Minio: &mio.Config{
Region: stg.Minio.Region,
Endpoint: stg.Minio.EndpointURL,
Bucket: stg.Minio.Bucket,
Prefix: stg.Minio.Prefix,
InsecureSkipTLSVerify: stg.Minio.InsecureSkipTLSVerify,
DebugTrace: stg.Minio.DebugTrace,
PartSize: stg.Minio.PartSize,
Secure: stg.Minio.Secure,
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if currentCfg.Storage.S3 != nil && newCfg.Storage.S3 != nil { | ||
| if currentCfg.Storage.S3.Bucket != newCfg.Storage.S3.Bucket { | ||
| return true | ||
| } | ||
|
|
||
| if currentCfg.Storage.S3.Region != newCfg.Storage.S3.Region { | ||
| return true | ||
| } | ||
|
|
||
| if currentCfg.Storage.S3.EndpointURL != newCfg.Storage.S3.EndpointURL { | ||
| return true | ||
| } | ||
|
|
||
| if currentCfg.Storage.S3.Prefix != newCfg.Storage.S3.Prefix { | ||
| return true | ||
| } | ||
|
|
||
| if currentCfg.Storage.S3.Credentials.AccessKeyID != newCfg.Storage.S3.Credentials.AccessKeyID { | ||
| if currentCfg.Storage.S3.Bucket != newCfg.Storage.S3.Bucket || | ||
| currentCfg.Storage.S3.Region != newCfg.Storage.S3.Region || | ||
| currentCfg.Storage.S3.EndpointURL != newCfg.Storage.S3.EndpointURL || | ||
| currentCfg.Storage.S3.Prefix != newCfg.Storage.S3.Prefix || | ||
| currentCfg.Storage.S3.Credentials.AccessKeyID != newCfg.Storage.S3.Credentials.AccessKeyID || | ||
| currentCfg.Storage.S3.Credentials.SecretAccessKey != newCfg.Storage.S3.Credentials.SecretAccessKey { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
The S3 configuration check is missing the ForcePathStyle field comparison. This field is being set in GetPBMStorageS3Config (line 408 of pbm.go) and is a pointer type (*bool) in the spec. It should be checked using ptr.Equal like it is for Minio on line 237, otherwise changes to S3's ForcePathStyle setting won't trigger a resync.
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return true | ||
| } | ||
|
|
||
| if currentCfg.Storage.S3.ForcePathStyle != newCfg.Storage.S3.ForcePathStyle { |
There was a problem hiding this comment.
Inconsistent handling of ForcePathStyle comparison. S3's ForcePathStyle is a pointer type (*bool) in the API spec, but is being compared directly as a bool here, while MinIO's ForcePathStyle (also *bool) is correctly compared using ptr.Equal on line 279. This should use ptr.Equal for pointer comparison to handle nil values correctly.
| if currentCfg.Storage.S3.ForcePathStyle != newCfg.Storage.S3.ForcePathStyle { | |
| if !ptr.Equal(currentCfg.Storage.S3.ForcePathStyle, newCfg.Storage.S3.ForcePathStyle) { |
| { | ||
| "minio: nothing changed", | ||
| &config.Config{ | ||
| Storage: config.StorageConf{ | ||
| Type: storage.Minio, | ||
| Minio: &mio.Config{ | ||
| Bucket: "operator-testing", | ||
| Region: "us-east-1", | ||
| Endpoint: "operator-testing.com", | ||
| Secure: true, | ||
| InsecureSkipTLSVerify: false, | ||
| }, | ||
| }, | ||
| }, | ||
| &config.Config{ | ||
| Storage: config.StorageConf{ | ||
| Type: storage.Minio, | ||
| Minio: &mio.Config{ | ||
| Bucket: "operator-testing", | ||
| Region: "us-east-1", | ||
| Endpoint: "operator-testing.com", | ||
| Secure: true, | ||
| InsecureSkipTLSVerify: false, | ||
| }, | ||
| }, | ||
| }, | ||
| false, | ||
| }, |
There was a problem hiding this comment.
Missing test coverage for MinIO ForcePathStyle changes. The isResyncNeeded function checks for ForcePathStyle changes in MinIO configuration (line 279 in pbm.go), but there's no corresponding test case to verify this behavior. Consider adding a test case similar to the ones for other MinIO configuration changes.
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
commit: f255ff2 |
CHANGE DESCRIPTION
Problem:
Changing Minio configuration does not get synced with PBM.
Cause:
The isResyncNeeded helper does not take Minio configuration into account
Solution:
Update
isResyncNeededto check Minio configurationCHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability