Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support skip syncing matrixonecluster cr #562

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

loveRhythm1990
Copy link
Contributor

@loveRhythm1990 loveRhythm1990 commented Oct 23, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Fixes # https://github.com/matrixorigin/MO-Cloud/issues/4074

What this PR does / why we need it:

some MatrixoneCluster CRs should skip sync for some reason, like debug or migrate MatrixoneCluster CR subresource management to other controllers. if matrixonecluster cr has annotation matrixorigin.io/skip-sync, operator controller will skip sync cr

Special notes for your reviewer:

Not Available

Additional documentation (e.g. design docs, usage docs, etc.):

Not Available


PR Type

enhancement


Description

  • Introduced a new annotation matrixorigin.io/skip-sync to allow skipping the synchronization of MatrixOneCluster custom resources.
  • Updated the Observe and Finalize methods in the MatrixOneClusterActor to check for the skip-sync annotation and bypass operations if present.
  • Added logging to provide feedback when synchronization or finalization is skipped due to the annotation.

Changes walkthrough 📝

Relevant files
Enhancement
controller.go
Add support for skipping sync of MatrixOneCluster CR         

pkg/controllers/mocluster/controller.go

  • Added a new annotation matrixorigin.io/skip-sync to control skipping
    sync.
  • Implemented logic to skip synchronization if the annotation is
    present.
  • Added logging to indicate when synchronization is skipped.
  • Updated finalization logic to respect the skip-sync annotation.
  • +9/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Race Condition
    The new skip-sync functionality might introduce a race condition if the annotation is added or removed while the controller is processing the resource. Consider implementing a mechanism to handle concurrent modifications.

    Error Handling
    The new skip-sync logic in the Observe and Finalize methods doesn't handle potential errors that might occur when accessing annotations. Consider adding error checking for robustness.

    Logging Improvement
    The logging messages for skipped sync and finalize operations could be more informative. Consider including more context, such as the reason for skipping or the value of the annotation.

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Extract repeated logic into a separate function to improve code organization and maintainability

    Consider extracting the skip-sync check into a separate function to improve code
    reusability and readability, as it's used in both Observe and Finalize methods.

    pkg/controllers/mocluster/controller.go [64-67]

    -if ctx.Obj.Annotations[annSkipSync] != "" {
    -    ctx.Log.Info(fmt.Sprintf("skip sync matrixonecluster %v", client.ObjectKeyFromObject(ctx.Obj)))
    +func shouldSkipSync(ctx *recon.Context[*v1alpha1.MatrixOneCluster]) bool {
    +    if ctx.Obj.Annotations[annSkipSync] != "" {
    +        ctx.Log.Info(fmt.Sprintf("skip sync matrixonecluster %v", client.ObjectKeyFromObject(ctx.Obj)))
    +        return true
    +    }
    +    return false
    +}
    +
    +// In Observe method
    +if shouldSkipSync(ctx) {
         return nil, nil
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Extracting the skip-sync check into a separate function enhances code reusability and readability, as the logic is used in both the Observe and Finalize methods. This change improves maintainability by centralizing the logic.

    8
    Best practice
    Use structured logging for improved log readability and consistency

    Consider using a structured logging approach instead of fmt.Sprintf for better log
    parsing and consistency across the codebase.

    pkg/controllers/mocluster/controller.go [65]

    -ctx.Log.Info(fmt.Sprintf("skip sync matrixonecluster %v", client.ObjectKeyFromObject(ctx.Obj)))
    +ctx.Log.Info("Skipping sync for matrixonecluster", "object", client.ObjectKeyFromObject(ctx.Obj))
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Structured logging improves log readability and consistency across the codebase, making it easier to parse logs. This suggestion enhances the logging practice without altering the core functionality.

    7
    Use a constant for annotation value comparison to improve code maintainability and reduce errors

    Consider using a constant for the "true" value when checking the skip-sync
    annotation. This improves maintainability and reduces the risk of typos.

    pkg/controllers/mocluster/controller.go [64]

    -if ctx.Obj.Annotations[annSkipSync] != "" {
    +const skipSyncValue = "true"
    +if ctx.Obj.Annotations[annSkipSync] == skipSyncValue {
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Using a constant for the annotation value comparison can improve maintainability and reduce the risk of typos. However, the current implementation checks for a non-empty string, which might be intentional to allow any non-empty value to trigger the skip. The suggestion is valid but may not align with the intended logic.

    5

    💡 Need additional feedback ? start a PR chat

    Copy link
    Contributor

    @aylei aylei left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    lgtm

    @aylei aylei merged commit 124d07b into matrixorigin:main Oct 24, 2024
    3 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants