Skip to content

Commit 74d71eb

Browse files
britaniarbritaniar
committed
address some comments
1 parent 884776e commit 74d71eb

File tree

2 files changed

+46
-29
lines changed

2 files changed

+46
-29
lines changed

‎pkg/controllers/workapplier/availability_tracker.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ import (
2323

2424
fleetnetworkingv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1"
2525
"go.goms.io/fleet-networking/pkg/common/objectmeta"
26+
2627
"go.goms.io/fleet/pkg/utils"
28+
"go.goms.io/fleet/pkg/utils/condition"
2729
"go.goms.io/fleet/pkg/utils/controller"
2830
)
2931

@@ -262,25 +264,23 @@ func trackServiceExportAvailability(curObj *unstructured.Unstructured) (Manifest
262264
}
263265

264266
// Check if ServiceExport is valid and up to date
267+
svcExportObj := klog.KObj(curObj)
265268
validCond := meta.FindStatusCondition(svcExport.Status.Conditions, string(fleetnetworkingv1alpha1.ServiceExportValid))
266-
if validCond == nil || svcExport.Generation != validCond.ObservedGeneration ||
267-
meta.IsStatusConditionFalse(svcExport.Status.Conditions, string(fleetnetworkingv1alpha1.ServiceExportValid)) {
268-
klog.V(2).InfoS("Still need to wait for ServiceExport to be available", "serviceExport", klog.KObj(curObj))
269+
if !condition.IsConditionStatusTrue(validCond, svcExport.Generation) {
270+
klog.V(2).InfoS("Still need to wait for ServiceExport to be valid", "serviceExport", svcExportObj, "validCondition", validCond)
269271
return ManifestProcessingAvailabilityResultTypeNotYetAvailable, nil
270272
}
271273

272-
weight := svcExport.Annotations[objectmeta.ServiceExportAnnotationWeight]
273-
if weight != "0" {
274+
if weight := svcExport.Annotations[objectmeta.ServiceExportAnnotationWeight]; weight != "0" {
274275
// Check conflict condition for non-zero weight
275276
conflictCond := meta.FindStatusCondition(svcExport.Status.Conditions, string(fleetnetworkingv1alpha1.ServiceExportConflict))
276-
if conflictCond == nil || svcExport.Generation != conflictCond.ObservedGeneration ||
277-
meta.IsStatusConditionTrue(svcExport.Status.Conditions, string(fleetnetworkingv1alpha1.ServiceExportConflict)) {
278-
klog.V(2).InfoS("Still need to wait for ServiceExport to be available", "serviceExport", klog.KObj(curObj))
277+
if condition.IsConditionStatusTrue(conflictCond, svcExport.Generation) {
278+
klog.V(2).InfoS("Still need to wait for ServiceExport to not have conflicts", "serviceExport", svcExportObj, "conflictCondition", conflictCond)
279279
return ManifestProcessingAvailabilityResultTypeNotYetAvailable, nil
280280
}
281281
}
282-
283-
klog.V(2).InfoS("ServiceExport is available", "serviceExport", klog.KObj(curObj))
282+
klog.V(2).InfoS("Skipping checking the conflict condition for the weight 0", "serviceExport", svcExportObj)
283+
klog.V(2).InfoS("ServiceExport is available", "serviceExport", svcExportObj)
284284
return ManifestProcessingAvailabilityResultTypeAvailable, nil
285285
}
286286

‎pkg/controllers/workapplier/availability_tracker_test.go‎

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828

2929
fleetnetworkingv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1"
3030
"go.goms.io/fleet-networking/pkg/common/objectmeta"
31+
3132
fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
3233
"go.goms.io/fleet/pkg/utils"
3334
"go.goms.io/fleet/pkg/utils/parallelizer"
@@ -988,25 +989,23 @@ func TestTrackInMemberClusterObjAvailabilityByGVR(t *testing.T) {
988989
func TestServiceExportAvailability(t *testing.T) {
989990
svcExportTemplate := &fleetnetworkingv1alpha1.ServiceExport{
990991
ObjectMeta: metav1.ObjectMeta{
991-
Name: "test-svcExport",
992-
Namespace: nsName,
993-
Annotations: map[string]string{
994-
objectmeta.ServiceExportAnnotationWeight: "0",
995-
},
996-
Generation: 3,
992+
Name: "test-svcExport",
993+
Namespace: nsName,
994+
Annotations: map[string]string{},
995+
Generation: 3,
997996
},
998997
}
999998

1000999
testCases := []struct {
10011000
name string
10021001
weight string
1003-
status *fleetnetworkingv1alpha1.ServiceExportStatus
1002+
status fleetnetworkingv1alpha1.ServiceExportStatus
10041003
wantManifestProcessingAvailabilityResultType ManifestProcessingAvailabilityResultType
10051004
}{
10061005
{
10071006
name: "available svcExport (annotation weight is 0)",
10081007
weight: "0",
1009-
status: &fleetnetworkingv1alpha1.ServiceExportStatus{
1008+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
10101009
Conditions: []metav1.Condition{
10111010
{
10121011
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
@@ -1021,7 +1020,7 @@ func TestServiceExportAvailability(t *testing.T) {
10211020
{
10221021
name: "unavailable svcExport (ServiceExportValid is false)",
10231022
weight: "0",
1024-
status: &fleetnetworkingv1alpha1.ServiceExportStatus{
1023+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
10251024
Conditions: []metav1.Condition{
10261025
{
10271026
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
@@ -1036,7 +1035,7 @@ func TestServiceExportAvailability(t *testing.T) {
10361035
{
10371036
name: "unavailable svcExport (different generation, annotation weight is 0)",
10381037
weight: "0",
1039-
status: &fleetnetworkingv1alpha1.ServiceExportStatus{
1038+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
10401039
Conditions: []metav1.Condition{
10411040
{
10421041
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
@@ -1051,7 +1050,7 @@ func TestServiceExportAvailability(t *testing.T) {
10511050
{
10521051
name: "available svcExport with no conflict (annotation weight is 1)",
10531052
weight: "1",
1054-
status: &fleetnetworkingv1alpha1.ServiceExportStatus{
1053+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
10551054
Conditions: []metav1.Condition{
10561055
{
10571056
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
@@ -1072,7 +1071,7 @@ func TestServiceExportAvailability(t *testing.T) {
10721071
{
10731072
name: "unavailable svcExport with conflict (annotation weight is 1)",
10741073
weight: "1",
1075-
status: &fleetnetworkingv1alpha1.ServiceExportStatus{
1074+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
10761075
Conditions: []metav1.Condition{
10771076
{
10781077
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
@@ -1093,12 +1092,13 @@ func TestServiceExportAvailability(t *testing.T) {
10931092
{
10941093
name: "unavailable invalid svcExport (annotation weight is 1)",
10951094
weight: "1",
1096-
status: &fleetnetworkingv1alpha1.ServiceExportStatus{
1095+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
10971096
Conditions: []metav1.Condition{
10981097
{
1099-
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
1100-
Status: metav1.ConditionFalse,
1101-
Reason: "ServiceIneligible",
1098+
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
1099+
Status: metav1.ConditionFalse,
1100+
Reason: "ServiceIneligible",
1101+
ObservedGeneration: 3,
11021102
},
11031103
},
11041104
},
@@ -1107,7 +1107,7 @@ func TestServiceExportAvailability(t *testing.T) {
11071107
{
11081108
name: "unavailable svcExport (different generation, annotation weight is 1)",
11091109
weight: "1",
1110-
status: &fleetnetworkingv1alpha1.ServiceExportStatus{
1110+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
11111111
Conditions: []metav1.Condition{
11121112
{
11131113
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
@@ -1125,13 +1125,30 @@ func TestServiceExportAvailability(t *testing.T) {
11251125
},
11261126
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
11271127
},
1128+
{
1129+
name: "unavailable svcExport (no annotation weight)",
1130+
weight: "",
1131+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
1132+
Conditions: []metav1.Condition{
1133+
{
1134+
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
1135+
Status: metav1.ConditionTrue,
1136+
Reason: "ServiceIsValid",
1137+
ObservedGeneration: 3,
1138+
},
1139+
},
1140+
},
1141+
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
1142+
},
11281143
}
11291144

11301145
for _, tc := range testCases {
11311146
t.Run(tc.name, func(t *testing.T) {
11321147
svcExport := svcExportTemplate.DeepCopy()
1133-
svcExport.Annotations[objectmeta.ServiceExportAnnotationWeight] = tc.weight
1134-
svcExport.Status = *tc.status
1148+
if tc.weight != "" {
1149+
svcExport.Annotations[objectmeta.ServiceExportAnnotationWeight] = tc.weight
1150+
}
1151+
svcExport.Status = tc.status
11351152
gotResTyp, err := trackServiceExportAvailability(toUnstructured(t, svcExport))
11361153
if err != nil {
11371154
t.Fatalf("trackServiceExportAvailability() = %v, want no error", err)

0 commit comments

Comments
 (0)