feat(migration): enhance skipMigration logic and update README for clarity#326
feat(migration): enhance skipMigration logic and update README for clarity#326anushkaaaaaaaa wants to merge 1 commit into
Conversation
…arity Signed-off-by: Anushka Sharan <anushkasharan05@gmail.com>
anushkaaaaaaaa
force-pushed
the
fix/auto-skip-migration-323
branch
from
05bf78a to
ada4ff3
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds automatic detection of previously migrated reports to prevent redundant migrations when the reports-server is upgraded or restarted. The server now checks the database on startup for reports containing the kyverno.reports-server.io/served-by: reports-server annotation and skips migration if found, while maintaining the --skipmigration flag for manual override.
Changes:
- Added
shouldSkipMigration()function to automatically detect if reports with the reports-server annotation already exist in the database - Updated migration logic to use automatic detection in addition to the manual flag
- Fixed spelling errors in migration error messages (changed "mirgrate" to "migrate")
- Updated documentation to reflect the new automatic detection behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| pkg/server/migration.go | Adds automatic migration skip detection logic and fixes spelling errors in error messages |
| pkg/app/opts/options.go | Updates flag description to mention automatic detection |
| charts/reports-server/README.md | Updates configuration documentation to mention automatic detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| klog.Info("found existing reports in database with reports-server annotation, skipping migration") | ||
| return true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| polrs, err := c.Store.PolicyReports().List(ctx, "") | ||
| if err == nil && len(polrs) > 0 { | ||
| for _, r := range polrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { | ||
| klog.Info("found existing reports in database with reports-server annotation, skipping migration") | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if c.APIServices.StoreEphemeralReports { | ||
| cephrs, err := c.Store.ClusterEphemeralReports().List(ctx) | ||
| if err == nil && len(cephrs) > 0 { | ||
| for _, r := range cephrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { | ||
| klog.Info("found existing reports in database with reports-server annotation, skipping migration") | ||
| return true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ephrs, err := c.Store.EphemeralReports().List(ctx, "") | ||
| if err == nil && len(ephrs) > 0 { | ||
| for _, r := range ephrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { | ||
| klog.Info("found existing reports in database with reports-server annotation, skipping migration") |
There was a problem hiding this comment.
The same log message "found existing reports in database with reports-server annotation, skipping migration" is printed on lines 32, 42, 54, and 64. This will result in duplicate log entries if reports exist in multiple categories. Consider logging once at the end of the function or using different messages for each report type to make debugging easier.
| cpolrs, err := c.Store.ClusterPolicyReports().List(ctx) | ||
| if err == nil && len(cpolrs) > 0 { | ||
| for _, r := range cpolrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { |
There was a problem hiding this comment.
The code accesses r.Annotations without checking if it's nil first. This could cause a nil pointer dereference if a report exists without annotations. The pattern used elsewhere in the codebase (e.g., watch.go lines 38, 74, 110, 146) includes a nil check before accessing the Annotations map. Add a nil check: if r.Annotations != nil && r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue
| polrs, err := c.Store.PolicyReports().List(ctx, "") | ||
| if err == nil && len(polrs) > 0 { | ||
| for _, r := range polrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { |
There was a problem hiding this comment.
The code accesses r.Annotations without checking if it's nil first. This could cause a nil pointer dereference if a report exists without annotations. The pattern used elsewhere in the codebase (e.g., watch.go lines 38, 74, 110, 146) includes a nil check before accessing the Annotations map. Add a nil check: if r.Annotations != nil && r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue
| polrs, err := c.Store.PolicyReports().List(ctx, "") | ||
| if err == nil && len(polrs) > 0 { | ||
| for _, r := range polrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { | ||
| klog.Info("found existing reports in database with reports-server annotation, skipping migration") | ||
| return true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Errors from database List operations are silently ignored. If the database is temporarily unavailable or there's a connection issue, the function will return false and proceed with migration, potentially causing duplicate data or migration failures. Consider logging the error or returning an error to the caller to allow proper error handling. Other database operations in the migration function return errors (e.g., line 90, 120).
| cephrs, err := c.Store.ClusterEphemeralReports().List(ctx) | ||
| if err == nil && len(cephrs) > 0 { | ||
| for _, r := range cephrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { |
There was a problem hiding this comment.
The code accesses r.Annotations without checking if it's nil first. This could cause a nil pointer dereference if a report exists without annotations. The pattern used elsewhere in the codebase (e.g., watch.go lines 38, 74, 110, 146) includes a nil check before accessing the Annotations map. Add a nil check: if r.Annotations != nil && r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue
| ephrs, err := c.Store.EphemeralReports().List(ctx, "") | ||
| if err == nil && len(ephrs) > 0 { | ||
| for _, r := range ephrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { |
There was a problem hiding this comment.
The code accesses r.Annotations without checking if it's nil first. This could cause a nil pointer dereference if a report exists without annotations. The pattern used elsewhere in the codebase (e.g., watch.go lines 38, 74, 110, 146) includes a nil check before accessing the Annotations map. Add a nil check: if r.Annotations != nil && r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue
| cpolrs, err := c.Store.ClusterPolicyReports().List(ctx) | ||
| if err == nil && len(cpolrs) > 0 { | ||
| for _, r := range cpolrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { | ||
| klog.Info("found existing reports in database with reports-server annotation, skipping migration") | ||
| return true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Errors from database List operations are silently ignored. If the database is temporarily unavailable or there's a connection issue, the function will return false and proceed with migration, potentially causing duplicate data or migration failures. Consider logging the error or returning an error to the caller to allow proper error handling. Other database operations in the migration function return errors (e.g., line 90, 120).
| cephrs, err := c.Store.ClusterEphemeralReports().List(ctx) | ||
| if err == nil && len(cephrs) > 0 { | ||
| for _, r := range cephrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { | ||
| klog.Info("found existing reports in database with reports-server annotation, skipping migration") | ||
| return true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Errors from database List operations are silently ignored. If the database is temporarily unavailable or there's a connection issue, the function will return false and proceed with migration, potentially causing duplicate data or migration failures. Consider logging the error or returning an error to the caller to allow proper error handling. Other database operations in the migration function return errors (e.g., line 153, 183).
| ephrs, err := c.Store.EphemeralReports().List(ctx, "") | ||
| if err == nil && len(ephrs) > 0 { | ||
| for _, r := range ephrs { | ||
| if r.Annotations[api.ServedByReportsServerAnnotation] == api.ServedByReportsServerValue { | ||
| klog.Info("found existing reports in database with reports-server annotation, skipping migration") | ||
| return true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Errors from database List operations are silently ignored. If the database is temporarily unavailable or there's a connection issue, the function will return false and proceed with migration, potentially causing duplicate data or migration failures. Consider logging the error or returning an error to the caller to allow proper error handling. Other database operations in the migration function return errors (e.g., line 153, 183).
|
@anushkaaaaaaaa |
Fixes
Added automatic detection to check if reports already exist in the database with the reports-server annotation. When detected, migration is skipped without requiring user intervention.
The server now queries the database on startup and looks for the
kyverno.reports-server.io/served-by: reports-serverannotation on existing reports. If found, it means a previous instance already migrated the data so we skip it. Keeps the--skipmigrationflag for manual override if needed.Changes
shouldSkipMigration()helper that checks database for reports with the annotation