-
Notifications
You must be signed in to change notification settings - Fork 77
fix: guard PatchCRVersion and use retry logic #520
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
base: master
Are you sure you want to change the base?
fix: guard PatchCRVersion and use retry logic #520
Conversation
Signed-off-by: Anant Singh Rathore <[email protected]>
Summary of ChangesHello @anant-shipit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and reliability of MeshSync CR version updates during startup. By introducing a guard mechanism to prevent patching non-existent Custom Resources and adopting standard Kubernetes retry logic, the system avoids unnecessary errors in environments where MeshSync might not be fully deployed. The changes also align the CR patching process with modern Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses the issue of startup errors when the MeshSync CR is not present by making the PatchCRVersion function more resilient. The refactoring to use controller-runtime's RetryOnConflict and client is a great improvement that aligns with standard Kubernetes operator patterns. My review includes a couple of suggestions to further improve the code by removing a redundant API call and enhancing context propagation, which will improve efficiency and robustness.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Anant Singh Rathore <[email protected]>
n2h9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a compilation error
https://github.com/meshery/meshsync/actions/runs/20621952052/job/59606921695?pr=520
pkg/lib/meshsync/meshsync.go:1: : # github.com/meshery/meshsync/pkg/lib/meshsync
Error: pkg/lib/meshsync/meshsync.go:76:50: not enough arguments in call to config.PatchCRVersion
have (*rest.Config)
want ("context".Context, *rest.Config) (typecheck)
Description:
Fixes #422
Problem
The
PatchCRVersionfunction previously attempted to update the MeshSync CR version during startup without verifying if the CRD existed. This resulted in false positive errors when running in environments where MeshSync is not installed. Additionally, the update logic used custom retry loops rather than standard Kubernetes patterns.Research & Proposed Fix
I have researched the Kubebuilder/Operator SDK patterns and updated the implementation plan as requested.
Findings:
PatchCRVersionruns during the imperative startup sequence (outside the reconcile loop), it requires self-contained resilience logic.Proposed Fix:
I have updated the logic to use Controller Runtime patterns combined with MeshKit standards.
IsNotFoundto fix Old MeshSync CRD not handled #422.RetryOnConflict(standard k8s utility) instead of a custom loop.MeshKitfor structured logging and error codes.Implementation Details
IsNotFoundto skip patching if the MeshSync CR is missing (clean state).controller-runtimeclient withMergeFromto ensure idempotent updates.internal/configimports to align with the project-wide move togithub.com/meshery/meshkit.Signed-off-by:
[Anant Singh Rathore] <[[email protected]]>