refactor: make sure operator upgrade smooth #187
Conversation
WalkthroughThe changes introduced in this pull request primarily focus on enhancing the management of persistent volume claims (PVCs) based on file storage types within the GreptimeDB controllers. Key modifications include the introduction of a new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (11)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
tests/e2e/greptimedbstandalone/test_basic_standalone.go (1)
90-90: Approved: Storage type specification enhances test precision.The change to include
common.FileStorageTypeDatanodeas an argument in theh.GetPVCscall aligns with the PR objectives and improves the specificity of the PVC retrieval in the test.Consider adding a brief comment explaining why we're specifically checking for datanode PVCs here. This would enhance the test's readability and maintainability. For example:
// Verify that the datanode PVC is retained after standalone deletion dataPVCs, err := h.GetPVCs(ctx, testStandalone.Namespace, testStandalone.Name, greptimev1alpha1.StandaloneKind, common.FileStorageTypeDatanode)tests/e2e/greptimedbcluster/test_basic_cluster.go (1)
90-90: Approved: Enhanced PVC retrieval with specific storage type.The addition of
common.FileStorageTypeDatanodeas an argument toh.GetPVCsaligns well with the PR objectives. It makes the PVC retrieval more specific and consistent with the newFileStorageTypeparameter introduced in this PR.Consider adding a brief comment explaining the significance of using
common.FileStorageTypeDatanodehere. This would enhance code readability and make the intent clearer for future maintainers. For example:// Retrieve PVCs specifically for datanode storage type datanodePVCs, err := h.GetPVCs(ctx, testCluster.Namespace, testCluster.Name, greptimev1alpha1.DatanodeComponentKind, common.FileStorageTypeDatanode)tests/e2e/greptimedbcluster/test_cluster_enable_remote_wal.go (1)
90-90: LGTM! Consider enhancing error handling.The change from
niltocommon.FileStorageTypeDatanodein theGetPVCscall aligns well with the PR objective of adding aFileStorageTypeparameter. This modification improves the specificity of PVC retrieval for datanodes, which is beneficial for storage management during upgrades.Consider wrapping the
GetPVCscall in a separate function that includes more detailed error handling. This could provide more context if the PVC retrieval fails, making debugging easier. For example:func getDatanodePVCs(ctx context.Context, h *helper.Helper, cluster *greptimev1alpha1.GreptimeDBCluster) ([]corev1.PersistentVolumeClaim, error) { pvcs, err := h.GetPVCs(ctx, cluster.Namespace, cluster.Name, greptimev1alpha1.DatanodeComponentKind, common.FileStorageTypeDatanode) if err != nil { return nil, fmt.Errorf("failed to get datanode PVCs: %w", err) } return pvcs, nil }Then use this function in your test:
datanodePVCs, err := getDatanodePVCs(ctx, h, testCluster) Expect(err).NotTo(HaveOccurred(), "failed to get datanode PVCs")This approach would make the test more readable and provide more detailed error information if needed.
tests/e2e/greptimedbcluster/test_cluster_standalone_wal.go (1)
96-96: LGTM: Updated PVC retrieval for WALThe change from
common.WALFileStorageLabelstocommon.FileStorageTypeWALis consistent with the previous modification and aligns with the PR objectives.Consider extracting the
FileStorageTypeWALconstant to a variable at the beginning of the function for improved readability:func TestClusterStandaloneWAL(ctx context.Context, h *helper.Helper) { const ( testCRFile = "./testdata/resources/cluster/standalone-wal/cluster.yaml" testSQLFile = "./testdata/sql/cluster/partition.sql" + walStorageType = common.FileStorageTypeWAL ) // ... (rest of the function) Eventually(func() error { - walPVCs, err := h.GetPVCs(ctx, testCluster.Namespace, testCluster.Name, greptimev1alpha1.DatanodeComponentKind, common.FileStorageTypeWAL) + walPVCs, err := h.GetPVCs(ctx, testCluster.Namespace, testCluster.Name, greptimev1alpha1.DatanodeComponentKind, walStorageType) if err != nil { return err } // ... (rest of the function) }, helper.DefaultTimeout, time.Second).ShouldNot(HaveOccurred()) }This change would make the code more readable and easier to maintain.
controllers/greptimedbstandalone/controller.go (3)
122-126: LGTM: Proper handling of default valuesThe changes appropriately create a deep copy of the original object and set default values. The error handling for
SetDefaultsis good.Consider wrapping the error returned from
SetDefaultsto provide more context:if err = standalone.SetDefaults(); err != nil { - r.Recorder.Event(standalone, corev1.EventTypeWarning, "SetDefaultValuesFailed", fmt.Sprintf("Set default values failed: %v", err)) - return ctrl.Result{}, err + wrappedErr := fmt.Errorf("failed to set default values: %w", err) + r.Recorder.Event(standalone, corev1.EventTypeWarning, "SetDefaultValuesFailed", wrappedErr.Error()) + return ctrl.Result{}, wrappedErr }This change would provide more context in the error message, making debugging easier.
128-138: LGTM: Proper handling of spec updates and loggingThe changes correctly compare the original and updated specs, update the cluster state when necessary, and log the creation of a new standalone at the appropriate time.
Consider adding a retry mechanism for the update operation to handle potential conflicts:
if !cmp.Equal(originalObject.Spec, standalone.Spec) { - if err = r.Update(ctx, standalone); err != nil { - r.Recorder.Event(standalone, corev1.EventTypeWarning, "UpdateStandaloneFailed", fmt.Sprintf("Update standalone failed: %v", err)) - return ctrl.Result{}, err - } + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Get(ctx, types.NamespacedName{Name: standalone.Name, Namespace: standalone.Namespace}, standalone); err != nil { + return err + } + standalone.Spec = originalObject.Spec + return r.Update(ctx, standalone) + }) + if err != nil { + r.Recorder.Event(standalone, corev1.EventTypeWarning, "UpdateStandaloneFailed", fmt.Sprintf("Update standalone failed: %v", err)) + return ctrl.Result{}, err + } }This change would make the update operation more resilient to conflicts that may occur in a concurrent environment.
122-138: Overall assessment: Improved standalone object handlingThe changes in this file enhance the
Reconcilefunction by:
- Properly setting default values for the standalone object.
- Efficiently detecting and applying updates to the standalone spec.
- Improving the timing of logging for new standalone creation.
These modifications contribute to more robust and consistent handling of the GreptimeDBStandalone custom resource. The use of deep copying and comparison ensures that updates are applied only when necessary, potentially reducing unnecessary API calls to the Kubernetes cluster.
Consider implementing a unit test for the
Reconcilefunction to verify the new behavior, especially around setting defaults and updating the standalone object. This would help ensure the correctness of these critical operations and make the code more maintainable in the long run.controllers/greptimedbcluster/deployers/datanode.go (1)
237-240: Improved storage deletion with FileStorageTypeThe
deleteStoragemethod has been refactored to useFileStorageType, which is consistent with the changes in theCleanUpmethod. The use ofGetPVCsfunction simplifies PVC retrieval.Consider adding more detailed error logging when PVC deletion fails. For example:
for _, pvc := range claims { klog.Infof("Deleting datanode PVC: %s", pvc.Name) if err := d.Delete(ctx, &pvc); err != nil { + klog.Errorf("Failed to delete PVC %s: %v", pvc.Name, err) return err } }Also applies to: 245-249
controllers/greptimedbstandalone/deployer.go (2)
154-155: Enhance log message to include the storage typeThe current log message in
deleteStorageis generic and doesn't specify which storage type is being deleted. For better clarity in logs, consider including thefsTypein the message.Suggested change:
- klog.Infof("Deleting standalone storage...") + klog.Infof("Deleting standalone storage of type: %s", fsType)
Line range hint
162-166: Handle errors when deleting PVCs to avoid partial deletionsIf an error occurs while deleting a PVC, the function returns immediately, which might leave subsequent PVCs undeleted. To ensure all PVCs are attempted for deletion, consider logging the error and continuing the loop or aggregating errors.
Suggested modification:
var deleteErrors []error for _, pvc := range claims { klog.Infof("Deleting standalone PVC: %s", pvc.Name) if err := d.Delete(ctx, &pvc); err != nil { klog.Errorf("Failed to delete PVC %s: %v", pvc.Name, err) deleteErrors = append(deleteErrors, err) } } if len(deleteErrors) > 0 { return fmt.Errorf("failed to delete one or more PVCs") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
- controllers/common/common.go (3 hunks)
- controllers/greptimedbcluster/controller.go (2 hunks)
- controllers/greptimedbcluster/deployers/datanode.go (3 hunks)
- controllers/greptimedbstandalone/controller.go (2 hunks)
- controllers/greptimedbstandalone/deployer.go (3 hunks)
- go.mod (1 hunks)
- tests/e2e/greptimedbcluster/test_basic_cluster.go (1 hunks)
- tests/e2e/greptimedbcluster/test_cluster_enable_flow.go (1 hunks)
- tests/e2e/greptimedbcluster/test_cluster_enable_remote_wal.go (1 hunks)
- tests/e2e/greptimedbcluster/test_cluster_standalone_wal.go (1 hunks)
- tests/e2e/greptimedbstandalone/test_basic_standalone.go (1 hunks)
- tests/e2e/helper/helper.go (1 hunks)
🔇 Additional comments not posted (19)
tests/e2e/greptimedbcluster/test_basic_cluster.go (1)
Line range hint
1-101: Summary: Improved specificity in PVC retrieval for datanode storage.The change made to the
TestBasicClusterfunction enhances the test by specifying the storage type when retrieving Persistent Volume Claims (PVCs) for datanodes. This modification aligns well with the PR objectives of refactoring the operator for smoother upgrades and introducing theFileStorageTypeparameter.The test's overall structure and logic remain intact, maintaining its effectiveness in verifying the basic cluster functionality while now being more precise in its PVC management. This change contributes to a more robust and specific testing process for the GreptimeDB operator.
tests/e2e/greptimedbcluster/test_cluster_enable_flow.go (1)
90-90: LGTM: Improved PVC retrieval specificityThe addition of
common.FileStorageTypeDatanodeas a parameter toh.GetPVCsaligns well with the PR objectives. This change enhances the precision of PVC retrieval by specifically targeting datanode storage, which is consistent with the introduction of the newFileStorageTypeparameter mentioned in the PR summary.This modification improves the test's ability to accurately verify PVC retention for datanodes after cluster deletion, without altering the overall logic of the test.
tests/e2e/greptimedbcluster/test_cluster_standalone_wal.go (2)
90-96: Overall assessment: Improved PVC retrieval consistencyThe changes in this file successfully implement the new
FileStorageTypeparameter for both datanode and WAL PVC retrievals. This update enhances code clarity and maintains consistency with the PR objectives. The modifications contribute to a more streamlined upgrade process for the operator by standardizing how different storage types are handled.
90-90: LGTM: Updated PVC retrieval for datanodesThe change from
common.DatanodeFileStorageLabelstocommon.FileStorageTypeDatanodealigns with the PR objectives and improves clarity in specifying the storage type.Let's verify the consistency of this change across the codebase:
✅ Verification successful
Verified: Consistent usage of
FileStorageTypeDatanodeacross the codebaseThe update successfully replaces
DatanodeFileStorageLabelswithFileStorageTypeDatanode, ensuring consistency throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of DatanodeFileStorageLabels and ensure FileStorageTypeDatanode is used consistently. # Test 1: Search for any remaining usage of DatanodeFileStorageLabels echo "Searching for any remaining usage of DatanodeFileStorageLabels:" rg --type go "DatanodeFileStorageLabels" # Test 2: Verify consistent usage of FileStorageTypeDatanode echo "Verifying consistent usage of FileStorageTypeDatanode:" rg --type go "FileStorageTypeDatanode"Length of output: 2106
go.mod (1)
7-7: Dependency update looks good, but verify its impact.The update of
github.com/google/go-cmpfrom v0.5.9 to v0.6.0 is a minor version change, which typically includes new features and bug fixes without breaking changes. This change also moves the dependency from indirect to direct, suggesting intentional use in the project.To ensure this update doesn't introduce any unexpected behavior, please:
- Review the changelog for
go-cmpv0.6.0 to understand the changes.- Run the project's test suite to verify that the update doesn't break any existing functionality.
- Check if any new features in v0.6.0 could be beneficial for the project.
tests/e2e/helper/helper.go (1)
125-126: Approve changes with verification suggestions.The refactoring of
GetPVCsfunction aligns well with the PR objectives. The implementation is now more concise and leverages a common utility function, which should improve maintainability and consistency across the codebase.However, this change has some potential impacts:
- Any existing calls to this method using
additionalLabelswill need to be updated to provide the appropriateFileStorageType.- The behavior of PVC retrieval might have changed, depending on how
common.GetPVCshandles filtering compared to the previous implementation.To ensure the changes don't introduce regressions:
- Update all calls to this method in the e2e tests to use the new signature.
- Verify that all e2e tests using this method still pass with the new implementation.
- Check if there are any edge cases in the e2e tests that relied on custom label filtering, and adjust the tests if necessary.
You can use the following script to find all occurrences of
GetPVCsin the e2e test files:✅ Verification successful
Verification Successful.
All calls to the
GetPVCsmethod in the e2e tests have been updated to use the newFileStorageTypeparameter. No instances of the oldadditionalLabelsparameter remain, ensuring consistency and reducing potential errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all occurrences of GetPVCs in e2e test files rg --type go 'GetPVCs' tests/e2eLength of output: 1646
controllers/greptimedbstandalone/controller.go (1)
24-24: LGTM: Import addition for object comparisonThe addition of the
github.com/google/go-cmp/cmpimport is appropriate for the new functionality in theReconcilefunction, where object comparison is performed.controllers/greptimedbcluster/deployers/datanode.go (3)
85-85: Improved storage cleanup with FileStorageType constantsThe changes in the
CleanUpmethod enhance code clarity and maintainability by usingFileStorageTypeconstants instead of label maps. This aligns well with the PR objectives of streamlining the upgrade process.Also applies to: 91-91, 97-97
473-473: Consistent use of FileStorageType in PVC generationThe
generatePVCsmethod has been updated to useFileStorageTypeconstants, which is consistent with the changes in other parts of the file. This improves code consistency and readability.Also applies to: 478-478, 483-483
Line range hint
1-624: Overall improvement in storage management and code consistencyThe changes introduced in this file significantly enhance the storage management process by consistently using
FileStorageTypeconstants. This aligns well with the PR objectives of streamlining the upgrade process and improving maintainability. The modifications are consistent throughout the file, from theCleanUpmethod to PVC generation, resulting in more readable and maintainable code.controllers/common/common.go (4)
31-34: Definition ofFileStorageTypeLabelKeyis appropriateThe constant
FileStorageTypeLabelKeyis correctly defined and well-documented. It provides a clear key for PVC labels indicating the type of file storage.
36-41: Proper declaration ofFileStorageTypeand its constantsThe type
FileStorageTypeand its associated constants (FileStorageTypeDatanode,FileStorageTypeWAL,FileStorageTypeCache) are appropriately defined, enhancing code clarity and maintainability.
175-189: Validation of label assignment inFileStorageToPVCThe function
FileStorageToPVCcorrectly assigns labels based on theFileStorageType. SettinglabelstonilforFileStorageTypeDatanodeis appropriate due to limitations with modifying PVC labels in StatefulSets.
209-259: Verify thatGetPVCscorrectly filters PVCs based onFileStorageTypelabelsThe
GetPVCsfunction constructs label selectors to retrieve PVCs according to theFileStorageType. The use ofMatchExpressionsandMatchLabelsseems appropriate. However, to ensure that PVCs are correctly retrieved, please verify that PVCs have the expected labels in the cluster.Run the following script to list PVCs and their labels:
Check the output to ensure that the labels align with the expectations, confirming that the
GetPVCsfunction will accurately filter the PVCs based on their storage type.controllers/greptimedbstandalone/deployer.go (2)
322-334: Confirm PVCs are generated correctly for each storage typeIn the
generatePVCsmethod, PVCs are created for different storage types usingcommon.FileStorageToPVC. Ensure that this function correctly handles eachFileStorageTypeand that the resulting PVCs have the appropriate configurations.To verify, review the PVC definitions generated for each storage type:
#!/bin/bash # Description: Display generated PVC YAML for inspection. # Test: Output PVC definitions. Expect: Correct configurations per storage type. kubectl get pvc -n <namespace> -o yamlReplace
<namespace>with the appropriate namespace to inspect the PVCs.
100-105: Verify all calls todeleteStorageuse the new signatureThe
deleteStoragefunction now acceptsfsType common.FileStorageTypeinstead ofadditionalLabels map[string]string. Please ensure that all invocations ofdeleteStoragehave been updated accordingly to prevent any potential issues.Run the following script to check for any remaining calls using the old signature:
Also applies to: 106-110, 112-116
controllers/greptimedbcluster/controller.go (3)
24-24: Adding"github.com/google/go-cmp/cmp"for deep comparison is appropriateThe inclusion of the
cmppackage allows for accurate comparison of complex structs likeSpec. This is suitable for detecting changes after setting default values.
142-146: Use ofDeepCopyandSetDefaultsCreating a deep copy of the cluster before applying defaults is a good practice to preserve the original state for comparison. Error handling is properly implemented.
Line range hint
156-162: Validation of cluster creation detection logicBy moving the check for cluster creation after setting defaults and potential updates, you ensure that the cluster status reflects the most accurate state. The logic is sound.
zyy17
force-pushed
the
refactor/legacy-upgrade
branch
from
9fc590e
to
2ec86f8
Compare
What's changed
FileStorageTypeand don't need to add labels for datanode storage because of the statefulset doesn't support to modify PVC labels;Summary by CodeRabbit
Release Notes
New Features
Improvements