Skip to content

Commit 6b43ca2

Browse files
britaniarbritaniar
committed
address some comments
1 parent 884776e commit 6b43ca2

File tree

4 files changed

+132
-32
lines changed

4 files changed

+132
-32
lines changed

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ require (
1919
github.com/spf13/pflag v1.0.5
2020
github.com/stretchr/testify v1.10.0
2121
github.com/wI2L/jsondiff v0.6.0
22-
go.goms.io/fleet-networking v0.3.3
22+
go.goms.io/fleet-networking v0.3.5
2323
go.uber.org/atomic v1.11.0
2424
go.uber.org/zap v1.27.0
2525
golang.org/x/exp v0.0.0-20241004190924-225e2abe05e6

‎go.sum‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -253,8 +253,8 @@ github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcY
253253
github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
254254
github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
255255
github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
256-
go.goms.io/fleet-networking v0.3.3 h1:5rwBntaUoLF+E1CzaWAEL4GdvLJPQorKhjgkbLlllPE=
257-
go.goms.io/fleet-networking v0.3.3/go.mod h1:Qgbi8M1fGaz/p5rtb6HJPmTDATWRnMt9HD1gz57WKUc=
256+
go.goms.io/fleet-networking v0.3.5 h1:nR0LFEbpu9cfBwYFVV8cGB8OARemIwtzaaFpRpp1GtQ=
257+
go.goms.io/fleet-networking v0.3.5/go.mod h1:zegH1iEZScjUWiGnXL67D/ZfWTM9DBUaSo1gPWaxLek=
258258
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.53.0 h1:4K4tsIXefpVJtvA/8srF4V4y0akAoPHkIslgAkjixJA=
259259
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.53.0/go.mod h1:jjdQuTGVsXV4vSs+CJ2qYDeDPf9yIJV23qlIzBm73Vg=
260260
go.opentelemetry.io/otel v1.31.0 h1:NsJcKPIW0D0H3NgzPDHmo0WW6SptzPdqg/L1zsIm2hY=

‎pkg/controllers/workapplier/availability_tracker.go‎

Lines changed: 15 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,28 @@ 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+
weight, err := objectmeta.ExtractWeightFromServiceExport(&svcExport)
275+
if err != nil {
276+
klog.V(2).InfoS("ServiceExport has invalid weight", "serviceExport", svcExportObj, "error", err)
277+
return ManifestProcessingAvailabilityResultTypeFailed, err
278+
}
279+
if weight != 0 {
274280
// Check conflict condition for non-zero weight
275281
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))
282+
if !condition.IsConditionStatusFalse(conflictCond, svcExport.Generation) {
283+
klog.V(2).InfoS("Still need to wait for ServiceExport to not have conflicts", "serviceExport", svcExportObj, "conflictCondition", conflictCond)
279284
return ManifestProcessingAvailabilityResultTypeNotYetAvailable, nil
280285
}
281286
}
282-
283-
klog.V(2).InfoS("ServiceExport is available", "serviceExport", klog.KObj(curObj))
287+
klog.V(2).InfoS("Skipping checking the conflict condition for the weight 0", "serviceExport", svcExportObj)
288+
klog.V(2).InfoS("ServiceExport is available", "serviceExport", svcExportObj)
284289
return ManifestProcessingAvailabilityResultTypeAvailable, nil
285290
}
286291

‎pkg/controllers/workapplier/availability_tracker_test.go‎

Lines changed: 114 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,64 @@ 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{
1111+
Conditions: []metav1.Condition{
1112+
{
1113+
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
1114+
Status: metav1.ConditionTrue,
1115+
Reason: "ServiceIsValid",
1116+
ObservedGeneration: 3,
1117+
},
1118+
{
1119+
Type: string(fleetnetworkingv1alpha1.ServiceExportConflict),
1120+
Status: metav1.ConditionTrue,
1121+
Reason: "ConflictFound",
1122+
ObservedGeneration: 2,
1123+
},
1124+
},
1125+
},
1126+
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
1127+
},
1128+
{
1129+
name: "unavailable svcExport (no annotation weight, no conflict condition)",
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+
},
1143+
{
1144+
name: "available svcExport (no annotation weight)",
1145+
weight: "",
1146+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
1147+
Conditions: []metav1.Condition{
1148+
{
1149+
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
1150+
Status: metav1.ConditionTrue,
1151+
Reason: "ServiceIsValid",
1152+
ObservedGeneration: 3,
1153+
},
1154+
{
1155+
Type: string(fleetnetworkingv1alpha1.ServiceExportConflict),
1156+
Status: metav1.ConditionFalse,
1157+
Reason: "NoConflictFound",
1158+
ObservedGeneration: 3,
1159+
},
1160+
},
1161+
},
1162+
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeAvailable,
1163+
},
1164+
{
1165+
name: "unavailable svcExport (no annotation weight with conflict)",
1166+
weight: "",
1167+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
11111168
Conditions: []metav1.Condition{
11121169
{
11131170
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
@@ -1119,19 +1176,57 @@ func TestServiceExportAvailability(t *testing.T) {
11191176
Type: string(fleetnetworkingv1alpha1.ServiceExportConflict),
11201177
Status: metav1.ConditionTrue,
11211178
Reason: "ConflictFound",
1179+
ObservedGeneration: 3,
1180+
},
1181+
},
1182+
},
1183+
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
1184+
},
1185+
{
1186+
name: "unavailable svcExport (no annotation weight, different generation)",
1187+
weight: "",
1188+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
1189+
Conditions: []metav1.Condition{
1190+
{
1191+
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
1192+
Status: metav1.ConditionTrue,
1193+
Reason: "ServiceIsValid",
1194+
ObservedGeneration: 3,
1195+
},
1196+
{
1197+
Type: string(fleetnetworkingv1alpha1.ServiceExportConflict),
1198+
Status: metav1.ConditionFalse,
1199+
Reason: "NoConflictFound",
11221200
ObservedGeneration: 2,
11231201
},
11241202
},
11251203
},
11261204
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
11271205
},
1206+
{
1207+
name: "unavailable svcExport (no annotation weight, invalid service export)",
1208+
weight: "",
1209+
status: fleetnetworkingv1alpha1.ServiceExportStatus{
1210+
Conditions: []metav1.Condition{
1211+
{
1212+
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
1213+
Status: metav1.ConditionFalse,
1214+
Reason: "ServiceIsNotValid",
1215+
ObservedGeneration: 3,
1216+
},
1217+
},
1218+
},
1219+
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
1220+
},
11281221
}
11291222

11301223
for _, tc := range testCases {
11311224
t.Run(tc.name, func(t *testing.T) {
11321225
svcExport := svcExportTemplate.DeepCopy()
1133-
svcExport.Annotations[objectmeta.ServiceExportAnnotationWeight] = tc.weight
1134-
svcExport.Status = *tc.status
1226+
if tc.weight != "" {
1227+
svcExport.Annotations[objectmeta.ServiceExportAnnotationWeight] = tc.weight
1228+
}
1229+
svcExport.Status = tc.status
11351230
gotResTyp, err := trackServiceExportAvailability(toUnstructured(t, svcExport))
11361231
if err != nil {
11371232
t.Fatalf("trackServiceExportAvailability() = %v, want no error", err)

0 commit comments

Comments
 (0)