Skip to content

feat: add svc export trackability #1061

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 6 commits into from
Mar 13, 2025
Merged

feat: add svc export trackability #1061

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c47b06c
add service export health check
britaniar Mar 5, 2025
5073c45
address comments
britaniar Mar 6, 2025
884776e
minor fix
britaniar Mar 6, 2025
6b43ca2
address some comments
britaniar Mar 7, 2025
72bbcc5
Merge branch 'main' into svcExportTrackability
ryanzhang-oss Mar 11, 2025
732108e
address comments, add test cases and update test
britaniar Mar 12, 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
11 changes: 7 additions & 4 deletions pkg/controllers/workapplier/availability_tracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,11 +270,12 @@ func trackServiceExportAvailability(curObj *unstructured.Unstructured) (Manifest
klog.V(2).InfoS("Still need to wait for ServiceExport to be valid", "serviceExport", svcExportObj, "validCondition", validCond)
return ManifestProcessingAvailabilityResultTypeNotYetAvailable, nil
}

// Validate annotation weight. Updating the annotation won't change the object generation,
Copy link
Contributor

@zhiying-lin zhiying-lin Mar 13, 2025

Choose a reason for hiding this comment

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

missing period.

// so the current status is not reliable and need to validate the annotation again here
weight, err := objectmeta.ExtractWeightFromServiceExport(&svcExport)
if err != nil {
klog.V(2).InfoS("ServiceExport has invalid weight", "serviceExport", svcExportObj, "error", err)
return ManifestProcessingAvailabilityResultTypeFailed, err
klog.Errorf(err.Error(), "ServiceExport has invalid weight", "serviceExport", svcExportObj)
Copy link
Contributor

@zhiying-lin zhiying-lin Mar 13, 2025

Choose a reason for hiding this comment

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

klog.Errorf(err, "ServiceExport has invalid weight", "serviceExport", svcExportObj)

you don't need to call the err.Error(). the klog will print out the error message automatically.

return ManifestProcessingAvailabilityResultTypeNotYetAvailable, err
Copy link
Contributor

@zhiying-lin zhiying-lin Mar 13, 2025

Choose a reason for hiding this comment

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

we should not return the error here. no need to retry.
Sorry, it was missed before :(

}
if weight != 0 {
// Check conflict condition for non-zero weight
Expand All @@ -283,8 +284,10 @@ func trackServiceExportAvailability(curObj *unstructured.Unstructured) (Manifest
klog.V(2).InfoS("Still need to wait for ServiceExport to not have conflicts", "serviceExport", svcExportObj, "conflictCondition", conflictCond)
return ManifestProcessingAvailabilityResultTypeNotYetAvailable, nil
}
} else {
klog.V(2).InfoS("Skipping checking the conflict condition for the weight 0", "serviceExport", svcExportObj)
}
klog.V(2).InfoS("Skipping checking the conflict condition for the weight 0", "serviceExport", svcExportObj)

klog.V(2).InfoS("ServiceExport is available", "serviceExport", svcExportObj)
return ManifestProcessingAvailabilityResultTypeAvailable, nil
}
Expand Down
56 changes: 55 additions & 1 deletion pkg/controllers/workapplier/availability_tracker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package workapplier

import (
"context"
"fmt"
"testing"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -1001,6 +1002,7 @@ func TestServiceExportAvailability(t *testing.T) {
weight string
status fleetnetworkingv1alpha1.ServiceExportStatus
wantManifestProcessingAvailabilityResultType ManifestProcessingAvailabilityResultType
err error
}{
{
name: "available svcExport (annotation weight is 0)",
Expand All @@ -1016,6 +1018,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeAvailable,
err: nil,
},
{
name: "unavailable svcExport (ServiceExportValid is false)",
Expand All @@ -1031,6 +1034,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: nil,
},
{
name: "unavailable svcExport (different generation, annotation weight is 0)",
Expand All @@ -1046,6 +1050,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: nil,
},
{
name: "available svcExport with no conflict (annotation weight is 1)",
Expand All @@ -1067,6 +1072,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeAvailable,
err: nil,
},
{
name: "unavailable svcExport with conflict (annotation weight is 1)",
Expand All @@ -1088,6 +1094,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: nil,
},
{
name: "unavailable invalid svcExport (annotation weight is 1)",
Expand All @@ -1103,6 +1110,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: nil,
},
{
name: "unavailable svcExport (different generation, annotation weight is 1)",
Expand All @@ -1124,6 +1132,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: nil,
},
{
name: "unavailable svcExport (no annotation weight, no conflict condition)",
Expand All @@ -1139,6 +1148,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: nil,
},
{
name: "available svcExport (no annotation weight)",
Expand All @@ -1160,6 +1170,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeAvailable,
err: nil,
},
{
name: "unavailable svcExport (no annotation weight with conflict)",
Expand All @@ -1181,6 +1192,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: nil,
},
{
name: "unavailable svcExport (no annotation weight, different generation)",
Expand All @@ -1202,6 +1214,7 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: nil,
},
{
name: "unavailable svcExport (no annotation weight, invalid service export)",
Expand All @@ -1217,6 +1230,39 @@ func TestServiceExportAvailability(t *testing.T) {
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: nil,
},
{
name: "unavailable svcExport (invalid weight)",
weight: "a",
status: fleetnetworkingv1alpha1.ServiceExportStatus{
Conditions: []metav1.Condition{
{
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
Status: metav1.ConditionTrue,
Reason: "ServiceIsValid",
ObservedGeneration: 3,
},
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: fmt.Errorf("the weight annotation is not a valid integer: a"),
},
{
name: "unavailable svcExport (out of range weight)",
weight: "1002",
status: fleetnetworkingv1alpha1.ServiceExportStatus{
Conditions: []metav1.Condition{
{
Type: string(fleetnetworkingv1alpha1.ServiceExportValid),
Status: metav1.ConditionTrue,
Reason: "ServiceIsValid",
ObservedGeneration: 3,
},
},
},
wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
err: fmt.Errorf("the weight annotation is not in the range [0, 1000]: 1002"),
},
}

Expand All @@ -1228,9 +1274,17 @@ func TestServiceExportAvailability(t *testing.T) {
}
svcExport.Status = tc.status
gotResTyp, err := trackServiceExportAvailability(toUnstructured(t, svcExport))

// Check for errors
if err != nil {
t.Fatalf("trackServiceExportAvailability() = %v, want no error", err)
if tc.err == nil || err.Error() != tc.err.Error() {
t.Fatalf("trackServiceExportAvailability() = %v, want %v", err, tc.err)
}
} else if tc.err != nil {
t.Fatalf("trackServiceExportAvailability() = nil, want error: %v", tc.err)
}

// Check the result type
if gotResTyp != tc.wantManifestProcessingAvailabilityResultType {
t.Errorf("manifestProcessingAvailabilityResultType = %v, want %v", gotResTyp, tc.wantManifestProcessingAvailabilityResultType)
}
Expand Down
Loading