Skip to content

fix: bug fixes in rollout controller and work-generator #379

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

Merged
ryanzhang-oss merged 3 commits into from
Dec 18, 2025
+180 −1
Merged

fix: bug fixes in rollout controller and work-generator #379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3065b39
fix: bug fixes in rollout controller and work-generator
zhiying-lin Dec 15, 2025
ca29c4b
address comments
zhiying-lin Dec 16, 2025
47fb1dd
fix the comment
zhiying-lin Dec 16, 2025
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
4 changes: 3 additions & 1 deletion pkg/controllers/rollout/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req runtime.Request) (runtim
}

// find the master resourceSnapshot.
masterResourceSnapshot, err := controller.FetchLatestMasterResourceSnapshot(ctx, r.UncachedReader, placementKey)
// Use the cached client so that rollout controller and work-generator have the same view of the
// resourceSnapshots in order to reduce the possibility of missing resourceSnapshots in work-generator.
masterResourceSnapshot, err := controller.FetchLatestMasterResourceSnapshot(ctx, r.Client, placementKey)
Copy link
Contributor

@ryanzhang-oss ryanzhang-oss Dec 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this actually fixed the unexpected bug which was caused by an uncached read failure but we thought it was from cache.

if err != nil {
klog.ErrorS(err, "Failed to find the masterResourceSnapshot for the placement",
"placement", placementObjRef)
Expand Down
7 changes: 7 additions & 0 deletions pkg/controllers/workgenerator/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,13 @@ func (r *Reconciler) syncApplyStrategy(

// areAllWorkSynced checks if all the works are synced with the resource binding.
func areAllWorkSynced(existingWorks map[string]*fleetv1beta1.Work, resourceBinding fleetv1beta1.BindingObj, _, _ string) bool {
// If there is no existing work, they are not synced.
// Even for the case where the resource snapshot has no selected resources,
// there should be one work created for the empty resource list.
if len(existingWorks) == 0 {
return false
}
Comment on lines +677 to +681
Copy link
Contributor

@ryanzhang-oss ryanzhang-oss Dec 16, 2025

Choose a reason for hiding this comment

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

looks like this function is only called when the resource snapshot is missing.

Copy link
Contributor

@ryanzhang-oss ryanzhang-oss Dec 16, 2025

Choose a reason for hiding this comment

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

IIRC, we only create an empty work for eveloped cases. I was thinking of removing it

Copy link
Collaborator Author

@zhiying-lin zhiying-lin Dec 16, 2025

Choose a reason for hiding this comment

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

looks like this function is only called when the resource snapshot is missing.

yeah, it's possible when rollout controller updates the binding using the resourceSnapshot while it is deleted when work-generator queries this snapshot.

IIRC, we only create an empty work for eveloped cases. I was thinking of removing it

I validated it in my fleet.
We'll create the empty work even for the normal case,

 kubectl get work crp-empty-work -n fleet-member-aks-member-5 -o yaml
apiVersion: placement.kubernetes-fleet.io/v1
kind: Work
metadata:
  annotations:
    kubernetes-fleet.io/parent-cluster-resource-override-snapshot-hash: 74234e98afe7498fb5daf1f36ac2d78acc339464f950703b8c019892f982b90b
    kubernetes-fleet.io/parent-resource-override-snapshot-hash: 74234e98afe7498fb5daf1f36ac2d78acc339464f950703b8c019892f982b90b
    kubernetes-fleet.io/parent-resource-snapshot-name: crp-empty-0-snapshot
  creationTimestamp: "2025-12-16T08:04:43Z"
  finalizers:
  - kubernetes-fleet.io/work-cleanup
  generation: 1
  labels:
    kubernetes-fleet.io/parent-CRP: crp-empty
    kubernetes-fleet.io/parent-resource-binding: crp-empty-aks-member-5-30b4685b
    kubernetes-fleet.io/parent-resource-snapshot-index: "0"
  name: crp-empty-work
  namespace: fleet-member-aks-member-5
  resourceVersion: "440189780"
  uid: ce9960fc-f4e0-4b3f-b46c-846e0fb9c8ea
spec:
  applyStrategy:
    comparisonOption: PartialComparison
    type: ClientSideApply
    whenToApply: Always
    whenToTakeOver: Always
  workload: {}
status:
  conditions:
  - lastTransitionTime: "2025-12-16T08:04:43Z"
    message: All the specified manifests have been applied
    observedGeneration: 1
    reason: AllManifestsApplied
    status: "True"
    type: Applied
  - lastTransitionTime: "2025-12-16T08:04:43Z"
    message: All of the applied manifests are available
    observedGeneration: 1
    reason: AllManifestsAvailable
    status: "True"
    type: Available

yeah, we was thinking of removing this behavior few times, but we have to specially handle this case in multiple controllers. prefer to keep it internally.

The original complains was that it's not obvious from the CRP condition when selecting nothing. We can improve the external user experience/messages separately.


// TODO: check resourceOverrideSnapshotHash and clusterResourceOverrideSnapshotHash after all the work has the ParentResourceOverrideSnapshotHashAnnotation and ParentClusterResourceOverrideSnapshotHashAnnotation
resourceSnapshotName := resourceBinding.GetBindingSpec().ResourceSnapshotName
for _, work := range existingWorks {
Expand Down
170 changes: 170 additions & 0 deletions pkg/controllers/workgenerator/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3751,6 +3751,176 @@ func TestSyncApplyStrategy(t *testing.T) {
}
}

func TestAreAllWorkSynced(t *testing.T) {
tests := map[string]struct {
existingWorks map[string]*fleetv1beta1.Work
resourceBinding fleetv1beta1.BindingObj
want bool
}{
"returns false when no existing works": {
existingWorks: map[string]*fleetv1beta1.Work{},
resourceBinding: &fleetv1beta1.ClusterResourceBinding{
Spec: fleetv1beta1.ResourceBindingSpec{
ResourceSnapshotName: "snapshot-1",
},
},
want: false,
},
"returns true when all works are synced with annotation": {
existingWorks: map[string]*fleetv1beta1.Work{
"work1": {
ObjectMeta: metav1.ObjectMeta{
Name: "work1",
Annotations: map[string]string{
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1",
},
},
},
"work2": {
ObjectMeta: metav1.ObjectMeta{
Name: "work2",
Annotations: map[string]string{
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1",
},
},
},
},
resourceBinding: &fleetv1beta1.ClusterResourceBinding{
Spec: fleetv1beta1.ResourceBindingSpec{
ResourceSnapshotName: "snapshot-1",
},
},
want: true,
},
"returns false when works have different snapshot names": {
existingWorks: map[string]*fleetv1beta1.Work{
"work1": {
ObjectMeta: metav1.ObjectMeta{
Name: "work1",
Annotations: map[string]string{
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1",
},
},
},
"work2": {
ObjectMeta: metav1.ObjectMeta{
Name: "work2",
Annotations: map[string]string{
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-2",
},
},
},
},
resourceBinding: &fleetv1beta1.ClusterResourceBinding{
Spec: fleetv1beta1.ResourceBindingSpec{
ResourceSnapshotName: "snapshot-1",
},
},
want: false,
},
"returns true when works are synced via label construction (fallback)": {
existingWorks: map[string]*fleetv1beta1.Work{
"work1": {
ObjectMeta: metav1.ObjectMeta{
Name: "work1",
Labels: map[string]string{
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
},
},
},
},
resourceBinding: &fleetv1beta1.ClusterResourceBinding{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
fleetv1beta1.PlacementTrackingLabel: "test-placement",
},
},
Spec: fleetv1beta1.ResourceBindingSpec{
ResourceSnapshotName: "test-placement-1-snapshot",
},
},
want: true,
},
"returns false when label construction fallback fails": {
existingWorks: map[string]*fleetv1beta1.Work{
"work1": {
ObjectMeta: metav1.ObjectMeta{
Name: "work1",
Labels: map[string]string{
fleetv1beta1.ParentResourceSnapshotIndexLabel: "2",
},
},
},
},
resourceBinding: &fleetv1beta1.ClusterResourceBinding{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
fleetv1beta1.PlacementTrackingLabel: "test-placement",
},
},
Spec: fleetv1beta1.ResourceBindingSpec{
ResourceSnapshotName: "test-placement-1-snapshot",
},
},
want: false,
},
"returns false when works have no annotation and invalid label": {
existingWorks: map[string]*fleetv1beta1.Work{
"work1": {
ObjectMeta: metav1.ObjectMeta{
Name: "work1",
Labels: map[string]string{
fleetv1beta1.ParentResourceSnapshotIndexLabel: "invalid",
},
},
},
},
resourceBinding: &fleetv1beta1.ClusterResourceBinding{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
fleetv1beta1.PlacementTrackingLabel: "test-placement",
},
},
Spec: fleetv1beta1.ResourceBindingSpec{
ResourceSnapshotName: "test-placement-1-snapshot",
},
},
want: false,
},
"returns true for ResourceBinding (namespaced) with annotation": {
existingWorks: map[string]*fleetv1beta1.Work{
"work1": {
ObjectMeta: metav1.ObjectMeta{
Name: "work1",
Annotations: map[string]string{
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "test-snapshot-1",
},
},
},
},
resourceBinding: &fleetv1beta1.ResourceBinding{
ObjectMeta: metav1.ObjectMeta{
Name: "test-binding",
Namespace: "test-namespace",
},
Spec: fleetv1beta1.ResourceBindingSpec{
ResourceSnapshotName: "test-snapshot-1",
},
},
want: true,
},
}

for name, tt := range tests {
t.Run(name, func(t *testing.T) {
got := areAllWorkSynced(tt.existingWorks, tt.resourceBinding, "", "")
if got != tt.want {
t.Errorf("areAllWorkSynced() = %v, want %v", got, tt.want)
}
})
}
}

func TestShouldIgnoreWork(t *testing.T) {
tests := map[string]struct {
enqueueCRP bool
Expand Down
Loading