operator pdb: fix the remaining issues #7013
Conversation
karmada-bot
added
the
kind/cleanup
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
karmada-bot
added
the
size/L
Summary of ChangesHello @zhzhuang-zju, 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 focuses on refining the operator's handling of PodDisruptionBudgets (PDBs) by streamlining the creation of owner references for Deployments and StatefulSets. It enhances efficiency by eliminating redundant API calls to retrieve resource UIDs and improves code clarity through the introduction of standardized GVK constants and general import cleanup across various control plane components. These changes contribute to a more robust and maintainable PDB management system. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the creation of PodDisruptionBudgets (PDBs) for various components, simplifying the code by avoiding an extra API call to fetch the owner object. The changes in CreateOrUpdateDeployment and CreateOrUpdateStatefulSet to return the created/updated object are central to this improvement.
While the overall direction is good, I've found a couple of issues in operator/pkg/util/apiclient/idempotency.go that should be addressed:
CreateOrUpdateDeploymentis missing aGetcall beforeUpdate, which will likely cause update conflicts.CreateOrUpdateStatefulSethas a variable shadowing issue and returns an incorrect error value.
I've provided detailed comments and suggestions to fix these issues. Once these are addressed, the PR will be a solid improvement.
| func CreateOrUpdateDeployment(client clientset.Interface, deployment *appsv1.Deployment) (*appsv1.Deployment, error) { | ||
| var latest *appsv1.Deployment | ||
| var err error | ||
| latest, err = client.AppsV1().Deployments(deployment.GetNamespace()).Create(context.TODO(), deployment, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| if !apierrors.IsAlreadyExists(err) { | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| _, err := client.AppsV1().Deployments(deployment.GetNamespace()).Update(context.TODO(), deployment, metav1.UpdateOptions{}) | ||
| latest, err = client.AppsV1().Deployments(deployment.GetNamespace()).Update(context.TODO(), deployment, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| klog.V(5).InfoS("Successfully created or updated deployment", "deployment", deployment.GetName()) | ||
| return nil | ||
| return latest, err | ||
| } |
There was a problem hiding this comment.
The update path in this function is likely to fail with a conflict error. When updating a resource, the ResourceVersion of the object must be set to the current version in the cluster to avoid conflicts. The deployment object here is the desired state and does not have the ResourceVersion of the existing object.
To fix this, you should Get the existing deployment before calling Update to retrieve its ResourceVersion, similar to the pattern used in CreateOrUpdateStatefulSet.
func CreateOrUpdateDeployment(client clientset.Interface, deployment *appsv1.Deployment) (*appsv1.Deployment, error) {
var latest *appsv1.Deployment
var err error
latest, err = client.AppsV1().Deployments(deployment.GetNamespace()).Create(context.TODO(), deployment, metav1.CreateOptions{})
if err != nil {
if !apierrors.IsAlreadyExists(err) {
return nil, err
}
older, getErr := client.AppsV1().Deployments(deployment.GetNamespace()).Get(context.TODO(), deployment.GetName(), metav1.GetOptions{})
if getErr != nil {
return nil, getErr
}
deployment.ResourceVersion = older.ResourceVersion
latest, err = client.AppsV1().Deployments(deployment.GetNamespace()).Update(context.TODO(), deployment, metav1.UpdateOptions{})
if err != nil {
return nil, err
}
}
klog.V(5).InfoS("Successfully created or updated deployment", "deployment", deployment.GetName())
return latest, err
}| func CreateOrUpdateStatefulSet(client clientset.Interface, statefulSet *appsv1.StatefulSet) (*appsv1.StatefulSet, error) { | ||
| var latest *appsv1.StatefulSet | ||
| var err error | ||
| latest, err = client.AppsV1().StatefulSets(statefulSet.GetNamespace()).Create(context.TODO(), statefulSet, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| if !apierrors.IsAlreadyExists(err) { | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| older, err := client.AppsV1().StatefulSets(statefulSet.GetNamespace()).Get(context.TODO(), statefulSet.GetName(), metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| statefulSet.ResourceVersion = older.ResourceVersion | ||
| _, err = client.AppsV1().StatefulSets(statefulSet.GetNamespace()).Update(context.TODO(), statefulSet, metav1.UpdateOptions{}) | ||
| latest, err = client.AppsV1().StatefulSets(statefulSet.GetNamespace()).Update(context.TODO(), statefulSet, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| klog.V(5).InfoS("Successfully created or updated statefulset", "statefulset", statefulSet.GetName()) | ||
| return nil | ||
| return latest, nil | ||
| } |
There was a problem hiding this comment.
This function has a variable shadowing issue and an incorrect return value that should be addressed.
- At line 262,
older, err := ...shadows theerrvariable from the outer scope. This can lead to unexpected behavior as the error from theCreateoperation is not correctly handled in the update path. - At line 275, the function returns
nilfor the error, which is inconsistent withCreateOrUpdateDeploymentand can mask an error from a successful update path.
To fix these issues and improve consistency, the function should be refactored to handle errors properly without shadowing and return the final error state.
func CreateOrUpdateStatefulSet(client clientset.Interface, statefulSet *appsv1.StatefulSet) (*appsv1.StatefulSet, error) {
var latest *appsv1.StatefulSet
var err error
latest, err = client.AppsV1().StatefulSets(statefulSet.GetNamespace()).Create(context.TODO(), statefulSet, metav1.CreateOptions{})
if err != nil {
if !apierrors.IsAlreadyExists(err) {
return nil, err
}
var older *appsv1.StatefulSet
older, err = client.AppsV1().StatefulSets(statefulSet.GetNamespace()).Get(context.TODO(), statefulSet.GetName(), metav1.GetOptions{})
if err != nil {
return nil, err
}
statefulSet.ResourceVersion = older.ResourceVersion
latest, err = client.AppsV1().StatefulSets(statefulSet.GetNamespace()).Update(context.TODO(), statefulSet, metav1.UpdateOptions{})
if err != nil {
return nil, err
}
}
klog.V(5).InfoS("Successfully created or updated statefulset", "statefulset", statefulSet.GetName())
return latest, err
}There was a problem hiding this comment.
At line 262, older, err := ... shadows the err variable from the outer scope. This can lead to unexpected behavior as the error from the Create operation is not correctly handled in the update path.
Could you elaborate on the specific issues with shadowed declarations? I don’t really see a problem here—err appears to be properly handled in all cases.
There was a problem hiding this comment.
Thanks for asking for clarification! I understand why it might seem like the err variable is handled correctly in all cases, but there's a subtle yet important issue with variable shadowing in Go that can lead to unexpected behavior.
When you have the line older, err := client.AppsV1().StatefulSets(...).Get(...) at line 262, the := operator declares a new err variable within that if block's scope. This new err variable shadows the err variable declared in the outer function scope.
Here's the problem:
- Initial
Createcall: Theerrvariable in the outer scope holds the result of theCreateoperation. If it's anAlreadyExistserror, the code proceeds into theif err != nilblock. Getcall within theifblock:older, err := client.AppsV1().StatefulSets(...).Get(...)declares a newerrvariable. If thisGetcall fails, this newerrvariable will hold that error, and the function correctly returnsnil, err(referring to the newerr).Updatecall within theifblock:_, err = client.AppsV1().StatefulSets(...).Update(...)then assigns the result of theUpdateoperation to this new, shadowederrvariable. If thisUpdatecall fails, this newerrvariable will hold that error, and the function correctly returnsnil, err(referring to the newerr).
The critical flaw occurs if the Update call succeeds, but the Create call originally failed with AlreadyExists:
- The outer
errisapierrors.IsAlreadyExists(err). The code enters theifblock. - The
Getcall succeeds. - The
Updatecall succeeds. The newerrvariable (declared at line 262) is nownil. - The function then executes
return latest, nil(referring to the new,nilerrfrom the inner scope).
This means that even though the original Create operation failed (with AlreadyExists), and the Update operation successfully handled the existing resource, the function's return value for err would be nil. This masks the fact that the resource wasn't created but rather updated, which can be misleading for callers expecting a nil error only on successful creation or update without prior existence issues.
My suggested change addresses this by ensuring that the err variable is consistently the same variable throughout the function, so its final state accurately reflects the overall outcome of the CreateOrUpdate logic. By changing older, err := ... to older, err = ... (and similarly for the latest, err = ... assignment), we avoid introducing a new err variable and ensure that the final return latest, err correctly propagates any error that occurred at any stage, or nil if everything was successful.
There was a problem hiding this comment.
This masks the fact that the resource wasn't created but rather updated, which can be misleading for callers expecting a nil error only on successful creation or update without prior existence issues.
You've misunderstood one key point: the purpose of this function is CreateOrUpdate. If the create fails but the subsequent update succeeds, the operation is considered successful from the caller's perspective. This function is specifically designed to abstract away whether the resource already exists or not.
Signed-off-by: zhzhuang-zju <[email protected]>
zhzhuang-zju
force-pushed
the
operator-pdb
branch
from
46a6ae6 to
b1ac00e
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7013 +/- ##
==========================================
- Coverage 46.65% 46.61% -0.05%
==========================================
Files 699 699
Lines 48163 48158 -5
==========================================
- Hits 22469 22447 -22
- Misses 23999 24022 +23
+ Partials 1695 1689 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@zhzhuang-zju Please rebase this PR as the E2E tests have been merged #7010. |
|
Sure |
4 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Fix left comments in #6933
Which issue(s) this PR fixes:
Part of #6282
Special notes for your reviewer:
Does this PR introduce a user-facing change?: