Skip to content

Commit 08ce18d

Browse files
britaniarbritaniar
committed
compare workStatus directly
1 parent 9795f08 commit 08ce18d

File tree

3 files changed

+211
-99
lines changed

3 files changed

+211
-99
lines changed

‎pkg/controllers/workgenerator/controller.go‎

Lines changed: 32 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import (
1515
"strings"
1616
"time"
1717

18+
"github.com/google/go-cmp/cmp"
19+
"github.com/google/go-cmp/cmp/cmpopts"
1820
"go.uber.org/atomic"
1921
"golang.org/x/sync/errgroup"
2022
corev1 "k8s.io/api/core/v1"
@@ -1148,51 +1150,39 @@ func (r *Reconciler) SetupWithManager(mgr controllerruntime.Manager) error {
11481150
"Failed to process an update event for work object")
11491151
return
11501152
}
1151-
oldAppliedCondition := meta.FindStatusCondition(oldWork.Status.Conditions, fleetv1beta1.WorkConditionTypeApplied)
1152-
newAppliedCondition := meta.FindStatusCondition(newWork.Status.Conditions, fleetv1beta1.WorkConditionTypeApplied)
1153-
oldAvailableCondition := meta.FindStatusCondition(oldWork.Status.Conditions, fleetv1beta1.WorkConditionTypeAvailable)
1154-
newAvailableCondition := meta.FindStatusCondition(newWork.Status.Conditions, fleetv1beta1.WorkConditionTypeAvailable)
11551153

1156-
// we try to filter out events, we only need to handle the updated event if the applied or available condition flip between true and false
1157-
// or the failed/diffed/drifted placements are changed.
1158-
if condition.EqualCondition(oldAppliedCondition, newAppliedCondition) && condition.EqualCondition(oldAvailableCondition, newAvailableCondition) {
1159-
oldDriftedPlacements := extractDriftedResourcePlacementsFromWork(oldWork)
1160-
newDriftedPlacements := extractDriftedResourcePlacementsFromWork(newWork)
1161-
driftsEqual := utils.IsDriftedResourcePlacementsEqual(oldDriftedPlacements, newDriftedPlacements)
1162-
if condition.IsConditionStatusFalse(newAppliedCondition, newWork.Generation) || condition.IsConditionStatusFalse(newAvailableCondition, newWork.Generation) {
1163-
diffsEqual := true
1164-
if condition.IsConditionStatusFalse(newAppliedCondition, newWork.Generation) {
1165-
oldDiffedPlacements := extractDiffedResourcePlacementsFromWork(oldWork)
1166-
newDiffedPlacements := extractDiffedResourcePlacementsFromWork(newWork)
1167-
diffsEqual = utils.IsDiffedResourcePlacementsEqual(oldDiffedPlacements, newDiffedPlacements)
1168-
}
1169-
// we need to compare the failed placement if the work is not applied or available
1170-
oldFailedPlacements := extractFailedResourcePlacementsFromWork(oldWork)
1171-
newFailedPlacements := extractFailedResourcePlacementsFromWork(newWork)
1172-
if driftsEqual && diffsEqual && utils.IsFailedResourcePlacementsEqual(oldFailedPlacements, newFailedPlacements) {
1173-
klog.V(2).InfoS("The placement lists didn't change on failed work, no need to reconcile", "oldWork", klog.KObj(oldWork), "newWork", klog.KObj(newWork))
1174-
return
1175-
}
1176-
} else {
1177-
oldResourceSnapshot := oldWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel]
1178-
newResourceSnapshot := newWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel]
1179-
if oldResourceSnapshot == "" || newResourceSnapshot == "" {
1180-
klog.ErrorS(controller.NewUnexpectedBehaviorError(errors.New("found an invalid work without parent-resource-snapshot-index")),
1181-
"Could not find the parent resource snapshot index label", "oldWork", klog.KObj(oldWork), "oldResourceSnapshotLabelValue", oldResourceSnapshot,
1182-
"newWork", klog.KObj(newWork), "newResourceSnapshotLabelValue", newResourceSnapshot)
1183-
return
1184-
}
1185-
// There is an edge case that, the work spec is the same but from different resourceSnapshots.
1186-
// WorkGenerator will update the work because of the label changes, but the generation is the same.
1187-
// When the normal update happens, the controller will set the applied condition as false and wait
1188-
// until the work condition has been changed.
1189-
// In this edge case, we need to requeue the binding to update the binding status.
1190-
if oldResourceSnapshot == newResourceSnapshot && driftsEqual {
1191-
klog.V(2).InfoS("The work applied or available condition stayed as true, no need to reconcile", "oldWork", klog.KObj(oldWork), "newWork", klog.KObj(newWork))
1192-
return
1193-
}
1154+
lessFuncCondition := func(a, b metav1.Condition) bool {
1155+
return a.Type < b.Type
1156+
}
1157+
workStatusCmpOptions := cmp.Options{
1158+
cmpopts.SortSlices(lessFuncCondition),
1159+
cmpopts.SortSlices(utils.LessFuncResourceIdentifier),
1160+
cmpopts.SortSlices(utils.LessFuncPatchDetail),
1161+
utils.IgnoreConditionLTTAndMessageFields,
1162+
cmpopts.EquateEmpty(),
1163+
}
1164+
if diff := cmp.Diff(oldWork.Status, newWork.Status, workStatusCmpOptions); diff != "" {
1165+
klog.V(2).InfoS("Work status has been changed", "oldWork", klog.KObj(oldWork), "newWork", klog.KObj(newWork))
1166+
} else {
1167+
oldResourceSnapshot := oldWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel]
1168+
newResourceSnapshot := newWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel]
1169+
if oldResourceSnapshot == "" || newResourceSnapshot == "" {
1170+
klog.ErrorS(controller.NewUnexpectedBehaviorError(errors.New("found an invalid work without parent-resource-snapshot-index")),
1171+
"Could not find the parent resource snapshot index label", "oldWork", klog.KObj(oldWork), "oldResourceSnapshotLabelValue", oldResourceSnapshot,
1172+
"newWork", klog.KObj(newWork), "newResourceSnapshotLabelValue", newResourceSnapshot)
1173+
return
1174+
}
1175+
// There is an edge case that, the work spec is the same but from different resourceSnapshots.
1176+
// WorkGenerator will update the work because of the label changes, but the generation is the same.
1177+
// When the normal update happens, the controller will set the applied condition as false and wait
1178+
// until the work condition has been changed.
1179+
// In this edge case, we need to requeue the binding to update the binding status.
1180+
if oldResourceSnapshot == newResourceSnapshot {
1181+
klog.V(2).InfoS("The work applied or available condition stayed as true, no need to reconcile", "oldWork", klog.KObj(oldWork), "newWork", klog.KObj(newWork))
1182+
return
11941183
}
11951184
}
1185+
11961186
// We need to update the binding status in this case
11971187
klog.V(2).InfoS("Received a work update event that we need to handle", "work", klog.KObj(newWork), "parentBindingName", parentBindingName)
11981188
queue.Add(reconcile.Request{NamespacedName: types.NamespacedName{

‎pkg/controllers/workgenerator/controller_integration_test.go‎

Lines changed: 168 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,7 @@ var _ = Describe("Test Work Generator Controller", func() {
451451
// mark one of the failed manifests as available but no change in the overall condition
452452
markOneManifestAvailable(&work, false)
453453
// check the binding status that it should be applied but not available and with one failed placement
454-
verifyBindStatusNotAvailableWithOnePlacement(binding, false, true)
454+
verifyBindStatusNotAvailableWithOnePlacement(binding, false, true, false)
455455
// mark the work available directly
456456
markWorkAvailable(&work)
457457
// check the binding status that it should be marked as available true eventually
@@ -470,7 +470,8 @@ var _ = Describe("Test Work Generator Controller", func() {
470470
// mark one of the failed manifests as available but no change in the overall condition
471471
markOneManifestAvailable(&work, false)
472472
// check the binding status that it should be applied but not available and with one failed placement
473-
verifyBindStatusNotAvailableWithOnePlacement(binding, false, true)
473+
// placement list should be changed
474+
verifyBindStatusNotAvailableWithOnePlacement(binding, false, true, false)
474475
// mark the work available directly
475476
markWorkAvailable(&work)
476477
// check the binding status that it should be marked as available true eventually
@@ -486,10 +487,21 @@ var _ = Describe("Test Work Generator Controller", func() {
486487
markWorkAsAppliedButNotAvailableWithFailedManifest(&work, true)
487488
// check the binding status that it should be applied but not available and with two failed placement and 2 drifted placement
488489
verifyBindStatusNotAvailableWithTwoPlacements(binding, false, true, true)
490+
// mark one of the failed manifests as available and no drift placements but no change in the overall condition
491+
markOneManifestAvailable(&work, true)
492+
// check the binding status that it should be applied but not available and with one failed placement and one drifted placement
493+
// placement list should be changed
494+
verifyBindStatusNotAvailableWithOnePlacement(binding, false, true, true)
489495
// mark the work available directly
490496
markWorkAvailable(&work)
491-
// check the binding status that it should be marked as available true eventually with 2 drift placements
492-
verifyBindStatusAvail(binding, false, true)
497+
// check the binding status that it should be marked as available true eventually with one drift placement
498+
verifyBindStatusAvailableWithOnePlacement(binding, false)
499+
// mark the work with no drift placements
500+
markWorkWithNoDrift(&work)
501+
// check the binding status that it should be marked as available true eventually with no drift placement
502+
// placement list should be changed
503+
verifyBindStatusAvail(binding, false, false)
504+
493505
})
494506

495507
It("Should continue to update the binding status even if the master resource snapshot is deleted after the work is synced", func() {
@@ -2073,7 +2085,7 @@ func verifyBindStatusNotAvailableWithTwoPlacements(binding *placementv1beta1.Clu
20732085
}, timeout, interval).Should(BeEmpty(), fmt.Sprintf("binding(%s) mismatch (-want +got)", binding.Name))
20742086
}
20752087

2076-
func verifyBindStatusNotAvailableWithOnePlacement(binding *placementv1beta1.ClusterResourceBinding, hasOverride, hasFailedPlacement bool) {
2088+
func verifyBindStatusNotAvailableWithOnePlacement(binding *placementv1beta1.ClusterResourceBinding, hasOverride, hasFailedPlacement, hasDriftedPlacement bool) {
20772089
Eventually(func() string {
20782090
Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed())
20792091
overrideReason := condition.OverrideNotSpecifiedReason
@@ -2133,6 +2145,96 @@ func verifyBindStatusNotAvailableWithOnePlacement(binding *placementv1beta1.Clus
21332145
},
21342146
}
21352147
}
2148+
2149+
if hasDriftedPlacement {
2150+
wantStatus.DriftedPlacements = []placementv1beta1.DriftedResourcePlacement{
2151+
{
2152+
ResourceIdentifier: placementv1beta1.ResourceIdentifier{
2153+
Group: "",
2154+
Version: "v1",
2155+
Kind: "Service",
2156+
Name: "svc-name",
2157+
Namespace: "svc-namespace",
2158+
},
2159+
ObservationTime: metav1.Time{Time: specificTime},
2160+
FirstDriftedObservedTime: metav1.Time{Time: specificTime},
2161+
TargetClusterObservedGeneration: 1,
2162+
ObservedDrifts: []placementv1beta1.PatchDetail{
2163+
{
2164+
Path: "/spec/ports/1/containerPort",
2165+
ValueInHub: "80",
2166+
ValueInMember: "90",
2167+
},
2168+
},
2169+
},
2170+
}
2171+
}
2172+
return cmp.Diff(wantStatus, binding.Status, cmpConditionOption)
2173+
}, timeout, interval).Should(BeEmpty(), fmt.Sprintf("binding(%s) mismatch (-want +got)", binding.Name))
2174+
}
2175+
2176+
func verifyBindStatusAvailableWithOnePlacement(binding *placementv1beta1.ClusterResourceBinding, hasOverride bool) {
2177+
Eventually(func() string {
2178+
Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed())
2179+
overrideReason := condition.OverrideNotSpecifiedReason
2180+
if hasOverride {
2181+
overrideReason = condition.OverriddenSucceededReason
2182+
}
2183+
wantStatus := placementv1beta1.ResourceBindingStatus{
2184+
Conditions: []metav1.Condition{
2185+
{
2186+
Type: string(placementv1beta1.ResourceBindingRolloutStarted),
2187+
Status: metav1.ConditionTrue,
2188+
Reason: condition.RolloutStartedReason,
2189+
ObservedGeneration: binding.GetGeneration(),
2190+
},
2191+
{
2192+
Type: string(placementv1beta1.ResourceBindingOverridden),
2193+
Status: metav1.ConditionTrue,
2194+
Reason: overrideReason,
2195+
ObservedGeneration: binding.GetGeneration(),
2196+
},
2197+
{
2198+
Type: string(placementv1beta1.ResourceBindingWorkSynchronized),
2199+
Status: metav1.ConditionTrue,
2200+
Reason: condition.AllWorkSyncedReason,
2201+
ObservedGeneration: binding.GetGeneration(),
2202+
},
2203+
{
2204+
Type: string(placementv1beta1.ResourceBindingApplied),
2205+
Status: metav1.ConditionTrue,
2206+
Reason: condition.AllWorkAppliedReason,
2207+
ObservedGeneration: binding.GetGeneration(),
2208+
},
2209+
{
2210+
Type: string(placementv1beta1.ResourceBindingAvailable),
2211+
Status: metav1.ConditionTrue,
2212+
Reason: condition.AllWorkAvailableReason,
2213+
ObservedGeneration: binding.GetGeneration(),
2214+
},
2215+
},
2216+
DriftedPlacements: []placementv1beta1.DriftedResourcePlacement{
2217+
{
2218+
ResourceIdentifier: placementv1beta1.ResourceIdentifier{
2219+
Group: "",
2220+
Version: "v1",
2221+
Kind: "Service",
2222+
Name: "svc-name",
2223+
Namespace: "svc-namespace",
2224+
},
2225+
ObservationTime: metav1.Time{Time: specificTime},
2226+
FirstDriftedObservedTime: metav1.Time{Time: specificTime},
2227+
TargetClusterObservedGeneration: 1,
2228+
ObservedDrifts: []placementv1beta1.PatchDetail{
2229+
{
2230+
Path: "/spec/ports/1/containerPort",
2231+
ValueInHub: "80",
2232+
ValueInMember: "90",
2233+
},
2234+
},
2235+
},
2236+
},
2237+
}
21362238
return cmp.Diff(wantStatus, binding.Status, cmpConditionOption)
21372239
}, timeout, interval).Should(BeEmpty(), fmt.Sprintf("binding(%s) mismatch (-want +got)", binding.Name))
21382240
}
@@ -2232,6 +2334,67 @@ func markWorkAvailable(work *placementv1beta1.Work) {
22322334
By(fmt.Sprintf("resource work `%s` is marked as available", work.Name))
22332335
}
22342336

2337+
func markWorkWithNoDrift(work *placementv1beta1.Work) {
2338+
work.Status.ManifestConditions = []placementv1beta1.ManifestCondition{
2339+
{
2340+
Identifier: placementv1beta1.WorkResourceIdentifier{
2341+
Ordinal: 0,
2342+
Group: "",
2343+
Version: "v1",
2344+
Kind: "ConfigMap",
2345+
Name: "config-name",
2346+
Namespace: "config-namespace",
2347+
},
2348+
Conditions: []metav1.Condition{
2349+
{
2350+
Type: placementv1beta1.WorkConditionTypeApplied,
2351+
Status: metav1.ConditionTrue,
2352+
Reason: "fakeAppliedManifest",
2353+
Message: "fake apply manifest",
2354+
LastTransitionTime: metav1.Now(),
2355+
},
2356+
{
2357+
Type: placementv1beta1.WorkConditionTypeAvailable,
2358+
Status: metav1.ConditionTrue,
2359+
Reason: "fakeAvailableManifest",
2360+
Message: "fake available manifest",
2361+
LastTransitionTime: metav1.Now(),
2362+
},
2363+
},
2364+
DriftDetails: nil,
2365+
},
2366+
{
2367+
Identifier: placementv1beta1.WorkResourceIdentifier{
2368+
Ordinal: 1,
2369+
Group: "",
2370+
Version: "v1",
2371+
Kind: "Service",
2372+
Name: "svc-name",
2373+
Namespace: "svc-namespace",
2374+
},
2375+
Conditions: []metav1.Condition{
2376+
{
2377+
Type: placementv1beta1.WorkConditionTypeApplied,
2378+
Status: metav1.ConditionTrue,
2379+
Reason: "fakeAppliedManifest",
2380+
Message: "fake apply manifest",
2381+
LastTransitionTime: metav1.Now(),
2382+
},
2383+
{
2384+
Type: placementv1beta1.WorkConditionTypeAvailable,
2385+
Status: metav1.ConditionTrue,
2386+
Reason: "fakeAvailableManifest",
2387+
Message: "fake available manifest",
2388+
LastTransitionTime: metav1.Now(),
2389+
},
2390+
},
2391+
DriftDetails: nil,
2392+
},
2393+
}
2394+
Expect(k8sClient.Status().Update(ctx, work)).Should(Succeed())
2395+
By(fmt.Sprintf("resource work `%s` is marked with no drift", work.Name))
2396+
}
2397+
22352398
// markWorkWithFailedToApplyAndNotAvailable marks the work as not applied with failedPlacement
22362399
func markWorkWithFailedToApplyAndNotAvailable(work *placementv1beta1.Work, hasDiffedDetails, hasDriftedDetails bool) {
22372400
meta.SetStatusCondition(&work.Status.Conditions, metav1.Condition{

‎pkg/utils/common.go‎

Lines changed: 11 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,17 @@ var LessFuncResourceIdentifier = func(a, b placementv1beta1.ResourceIdentifier)
516516
return aStr < bStr
517517
}
518518

519+
// LessFuncPatchDetail is a less function for sorting patch details
520+
var LessFuncPatchDetail = func(a, b placementv1beta1.PatchDetail) bool {
521+
if a.Path != b.Path {
522+
return a.Path < b.Path
523+
}
524+
if a.ValueInMember != b.ValueInMember {
525+
return a.ValueInMember < b.ValueInMember
526+
}
527+
return a.ValueInHub < b.ValueInHub
528+
}
529+
519530
// LessFuncFailedResourcePlacements is a less function for sorting failed resource placements
520531
var LessFuncFailedResourcePlacements = func(a, b placementv1beta1.FailedResourcePlacement) bool {
521532
var aStr, bStr string
@@ -573,32 +584,6 @@ var LessFuncDriftedResourcePlacements = func(a, b placementv1beta1.DriftedResour
573584
return aStr < bStr
574585
}
575586

576-
func IsDriftedResourcePlacementsEqual(oldDriftedResourcePlacements, newDriftedResourcePlacements []placementv1beta1.DriftedResourcePlacement) bool {
577-
if len(oldDriftedResourcePlacements) != len(newDriftedResourcePlacements) {
578-
return false
579-
}
580-
sort.Slice(oldDriftedResourcePlacements, func(i, j int) bool {
581-
return LessFuncDriftedResourcePlacements(oldDriftedResourcePlacements[i], oldDriftedResourcePlacements[j])
582-
})
583-
sort.Slice(newDriftedResourcePlacements, func(i, j int) bool {
584-
return LessFuncDriftedResourcePlacements(newDriftedResourcePlacements[i], newDriftedResourcePlacements[j])
585-
})
586-
for i := range oldDriftedResourcePlacements {
587-
oldDriftedResourcePlacement := oldDriftedResourcePlacements[i]
588-
newDriftedResourcePlacement := newDriftedResourcePlacements[i]
589-
if !equality.Semantic.DeepEqual(oldDriftedResourcePlacement.ResourceIdentifier, newDriftedResourcePlacement.ResourceIdentifier) {
590-
return false
591-
}
592-
if &oldDriftedResourcePlacement.TargetClusterObservedGeneration != &newDriftedResourcePlacement.TargetClusterObservedGeneration {
593-
return false
594-
}
595-
if &oldDriftedResourcePlacement.ObservationTime != &newDriftedResourcePlacement.ObservationTime {
596-
return false
597-
}
598-
}
599-
return true
600-
}
601-
602587
// LessFuncDiffedResourcePlacements is a less function for sorting drifted resource placements
603588
var LessFuncDiffedResourcePlacements = func(a, b placementv1beta1.DiffedResourcePlacement) bool {
604589
var aStr, bStr string
@@ -616,32 +601,6 @@ var LessFuncDiffedResourcePlacements = func(a, b placementv1beta1.DiffedResource
616601
return aStr < bStr
617602
}
618603

619-
func IsDiffedResourcePlacementsEqual(oldDiffedResourcePlacements, newDiffedResourcePlacements []placementv1beta1.DiffedResourcePlacement) bool {
620-
if len(oldDiffedResourcePlacements) != len(newDiffedResourcePlacements) {
621-
return false
622-
}
623-
sort.Slice(oldDiffedResourcePlacements, func(i, j int) bool {
624-
return LessFuncDiffedResourcePlacements(oldDiffedResourcePlacements[i], oldDiffedResourcePlacements[j])
625-
})
626-
sort.Slice(newDiffedResourcePlacements, func(i, j int) bool {
627-
return LessFuncDiffedResourcePlacements(newDiffedResourcePlacements[i], newDiffedResourcePlacements[j])
628-
})
629-
for i := range oldDiffedResourcePlacements {
630-
oldDiffedResourcePlacement := oldDiffedResourcePlacements[i]
631-
newDiffedResourcePlacement := newDiffedResourcePlacements[i]
632-
if !equality.Semantic.DeepEqual(oldDiffedResourcePlacement.ResourceIdentifier, newDiffedResourcePlacement.ResourceIdentifier) {
633-
return false
634-
}
635-
if &oldDiffedResourcePlacement.TargetClusterObservedGeneration != &newDiffedResourcePlacement.TargetClusterObservedGeneration {
636-
return false
637-
}
638-
if &oldDiffedResourcePlacement.ObservationTime != &newDiffedResourcePlacement.ObservationTime {
639-
return false
640-
}
641-
}
642-
return true
643-
}
644-
645604
// IsFleetAnnotationPresent returns true if a key with fleet prefix is present in the annotations map.
646605
func IsFleetAnnotationPresent(annotations map[string]string) bool {
647606
for k := range annotations {

0 commit comments

Comments
 (0)