Skip to content

fix: set expectation before creating target to prevent race condition #31

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.

Sign up for GitHub

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

Closed
AnnaYue wants to merge 1 commit into from
+18 −2
Closed

fix: set expectation before creating target to prevent race condition #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions synccontrols/sync_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,13 +582,19 @@ func (r *RealSyncControl) Scale(ctx context.Context, xsetObject api.XSetObject,
return fmt.Errorf("fail to create PVCs for target %s: %w", target.GetName(), err)
}
}
// set expectation BEFORE creating target to prevent race condition
// where multiple reconciles try to create the same target
if err := r.cacheExpectations.ExpectCreation(clientutil.ObjectKeyString(xsetObject), r.targetGVK, target.GetNamespace(), target.GetName()); err != nil {
return fmt.Errorf("fail to set creation expectation for target %s: %w", target.GetName(), err)
}
newTarget := target.DeepCopyObject().(client.Object)
logger.Info("try to create Target with revision of "+r.xsetGVK.Kind, "revision", revision.GetName())
if target, err = r.xControl.CreateTarget(ctx, newTarget); err != nil {
// delete expectation if create failed
r.cacheExpectations.DeleteExpectations(clientutil.ObjectKeyString(xsetObject))
Comment on lines +593 to +594
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

When target creation fails, DeleteExpectations is called with only the xset object key, which will delete ALL expectations for that xset. However, if some targets were successfully created before this one failed (since SlowStartBatch returns after first error), those targets' expectations will also be deleted. This can cause the reconciliation logic to not correctly track when those successfully created targets are added to the cache. The fix should delete only the specific target's expectation that failed, not all expectations for the xset.

Suggested change
// delete expectation if create failed
r.cacheExpectations.DeleteExpectations(clientutil.ObjectKeyString(xsetObject))
// delete expectation for this specific target if create failed
r.cacheExpectations.DeleteExpectations(clientutil.ObjectKeyString(xsetObject), r.targetGVK, target.GetNamespace(), target.GetName())

Copilot uses AI. Check for mistakes.
return err
}
// add an expectation for this target creation, before next reconciling
return r.cacheExpectations.ExpectCreation(clientutil.ObjectKeyString(xsetObject), r.targetGVK, target.GetNamespace(), target.GetName())
return nil
})
if needUpdateContext.Load() {
logger.Info("try to update ResourceContext for XSet after scaling out", "Context", syncContext.OwnedIds)
Expand Down
10 changes: 10 additions & 0 deletions xcontrol/target_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ import (
"fmt"
"reflect"

apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"kusionstack.io/kube-utils/controller/mixin"
refmanagerutil "kusionstack.io/kube-utils/controller/refmanager"
"sigs.k8s.io/controller-runtime/pkg/cache"
Expand Down Expand Up @@ -110,6 +112,14 @@ func (r *targetControl) GetFilteredTargets(ctx context.Context, selector *metav1

func (r *targetControl) CreateTarget(ctx context.Context, target client.Object) (client.Object, error) {
if err := r.client.Create(ctx, target); err != nil {
// If the target already exists, get and return the existing one
if apierrors.IsAlreadyExists(err) {
existingTarget := target.DeepCopyObject().(client.Object)
if getErr := r.client.Get(ctx, types.NamespacedName{Namespace: target.GetNamespace(), Name: target.GetName()}, existingTarget); getErr != nil {
return nil, fmt.Errorf("failed to create target and failed to get existing target: %w, %w", err, getErr)
}
return existingTarget, nil
}
return nil, fmt.Errorf("failed to create target: %w", err)
}
return target, nil
Expand Down
Loading