Skip to content

Commit

Permalink
Update NEG Status update logic when there is conflict in NEG desc.
Browse files Browse the repository at this point in the history 
* When there is conflict in NEG description, if the conflict occurs
  within the same cluster, skip update since that CR is owned by a
  different syncer. Otherwise, set initialize condition to false.
* Always update negObjRef in updateInitStatus to make sure the
  information is up-to-date.
  • Loading branch information
@sawsa307
sawsa307 committed Jul 30, 2024
1 parent 199b90e commit d994735
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 39 deletions.
16 changes: 11 additions & 5 deletions pkg/neg/syncers/transaction.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
discovery "k8s.io/api/discovery/v1"
"k8s.io/ingress-gce/pkg/flags"
"k8s.io/ingress-gce/pkg/network"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/endpointslices"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -423,6 +424,7 @@ func (s *transactionSyncer) ensureNetworkEndpointGroups() error {

var errList []error
var negObjRefs []negv1beta1.NegObjectReference
var negCrConflicted, clusterIDConflicted bool
negsByLocation := make(map[string]int)
for _, zone := range zones {
var negObj negv1beta1.NegObjectReference
Expand All @@ -445,6 +447,8 @@ func (s *transactionSyncer) ensureNetworkEndpointGroups() error {
)
if err != nil {
errList = append(errList, err)
clusterIDConflicted = clusterIDConflicted || errors.Is(err, utils.ErrClusterIDMismatch)
negCrConflicted = negCrConflicted || errors.Is(err, ErrNEGNameInUse)
}

if s.svcNegClient != nil && err == nil {
Expand All @@ -453,7 +457,12 @@ func (s *transactionSyncer) ensureNetworkEndpointGroups() error {
}
}

s.updateInitStatus(negObjRefs, errList)
// Do not modify NEG Status if there is conflict within the same cluster
// because the CR is owned by a different syncer.
if !negCrConflicted || clusterIDConflicted {
s.updateInitStatus(negObjRefs, errList)
}

s.syncMetricsCollector.UpdateSyncerNegCount(s.NegSyncerKey, negsByLocation)
return utilerrors.NewAggregate(errList)
}
Expand Down Expand Up @@ -773,10 +782,7 @@ func (s *transactionSyncer) updateInitStatus(negObjRefs []negv1beta1.NegObjectRe
}

neg := origNeg.DeepCopy()

if len(negObjRefs) != 0 {
neg.Status.NetworkEndpointGroups = negObjRefs
}
neg.Status.NetworkEndpointGroups = negObjRefs

initializedCondition := getInitializedCondition(utilerrors.NewAggregate(errList))
finalCondition := ensureCondition(neg, initializedCondition)
Expand Down
84 changes: 53 additions & 31 deletions pkg/neg/syncers/transaction_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1042,12 +1042,13 @@ func TestTransactionSyncerWithNegCR(t *testing.T) {
testNegType := negtypes.VmIpPortEndpointType

testCases := []struct {
desc string
negExists bool
negDesc string
crStatusPopulated bool
customName bool
expectErr bool
desc string
negExists bool
negDesc string
crStatusPopulated bool
customName bool
expectErr bool
expectNoopOnNegStatus bool
}{
{
desc: "Neg does not exist",
Expand All @@ -1064,12 +1065,13 @@ func TestTransactionSyncerWithNegCR(t *testing.T) {
expectErr: false,
},
{
desc: "Neg exists, custom name, without neg description",
negExists: true,
negDesc: "",
crStatusPopulated: false,
customName: true,
expectErr: true,
desc: "Neg exists, custom name, without neg description",
negExists: true,
negDesc: "",
crStatusPopulated: false,
customName: true,
expectErr: true,
expectNoopOnNegStatus: true,
},
{
desc: "Neg exists, cr has with populated status, with correct neg description",
Expand Down Expand Up @@ -1103,7 +1105,7 @@ func TestTransactionSyncerWithNegCR(t *testing.T) {
expectErr: false,
},
{
desc: "Neg exists, with conflicting cluster id in neg description",
desc: "Neg exists, with mismatched cluster id in neg description",
negExists: true,
negDesc: utils.NegDescription{
ClusterUID: "cluster-2",
Expand All @@ -1115,40 +1117,43 @@ func TestTransactionSyncerWithNegCR(t *testing.T) {
expectErr: true,
},
{
desc: "Neg exists, with conflicting namespace in neg description",
desc: "Neg exists, with mismatched namespace in neg description",
negExists: true,
negDesc: utils.NegDescription{
ClusterUID: kubeSystemUID,
Namespace: "namespace-2",
ServiceName: testServiceName,
Port: "80",
}.String(),
crStatusPopulated: false,
expectErr: true,
crStatusPopulated: false,
expectErr: true,
expectNoopOnNegStatus: true,
},
{
desc: "Neg exists, with conflicting service in neg description",
desc: "Neg exists, with mismatched service in neg description",
negExists: true,
negDesc: utils.NegDescription{
ClusterUID: kubeSystemUID,
Namespace: testServiceNamespace,
ServiceName: "service-2",
Port: "80",
}.String(),
crStatusPopulated: false,
expectErr: true,
crStatusPopulated: false,
expectErr: true,
expectNoopOnNegStatus: true,
},
{
desc: "Neg exists, with conflicting port in neg description",
desc: "Neg exists, with mismatched port in neg description",
negExists: true,
negDesc: utils.NegDescription{
ClusterUID: kubeSystemUID,
Namespace: testServiceNamespace,
ServiceName: testServiceName,
Port: "81",
}.String(),
crStatusPopulated: false,
expectErr: true,
crStatusPopulated: false,
expectErr: true,
expectNoopOnNegStatus: true,
},
{
desc: "Neg exists, cr has populated status, but error during initialization",
Expand All @@ -1158,10 +1163,11 @@ func TestTransactionSyncerWithNegCR(t *testing.T) {
ClusterUID: kubeSystemUID,
Namespace: testServiceNamespace,
ServiceName: testServiceName,
Port: "81",
Port: "81", // Expected port to be 80
}.String(),
crStatusPopulated: true,
expectErr: true,
crStatusPopulated: true,
expectErr: true,
expectNoopOnNegStatus: true,
},
}

Expand Down Expand Up @@ -1207,7 +1213,8 @@ func TestTransactionSyncerWithNegCR(t *testing.T) {
err = syncer.ensureNetworkEndpointGroups()
if !tc.expectErr && err != nil {
t.Errorf("Expected no error, but got: %v", err)
} else if tc.expectErr && err == nil {
}
if tc.expectErr && err == nil {
t.Errorf("Expected error, but got none")
}

Expand All @@ -1216,7 +1223,7 @@ func TestTransactionSyncerWithNegCR(t *testing.T) {
t.Errorf("Failed to get NEG from neg client: %s", err)
}
ret, _ := fakeCloud.AggregatedListNetworkEndpointGroup(syncer.NegSyncerKey.GetAPIVersion(), klog.TODO())
if len(expectedNegRefs) == 0 && !tc.expectErr {
if !tc.expectErr {
expectedNegRefs = negObjectReferences(ret)
}
// if error occurs, expect that neg object references are not populated
Expand All @@ -1225,12 +1232,27 @@ func TestTransactionSyncerWithNegCR(t *testing.T) {
}

checkNegCR(t, negCR, creationTS, expectZones, expectedNegRefs, false, tc.expectErr)
if tc.expectErr {
// If status is already populated, expect no change even when error occurs
if tc.expectErr && tc.expectNoopOnNegStatus {
// If CR is populated, we should have initialized and synced condition
var expectedConditionLen int
if tc.crStatusPopulated {
expectedConditionLen = 2
}

if len(negCR.Status.Conditions) != expectedConditionLen {
t.Errorf("Expected no change in NEG CR, but got len(negCR.Status.Conditions) = %d", len(negCR.Status.Conditions))
}
if len(negCR.Status.NetworkEndpointGroups) != len(expectedNegRefs) {
t.Errorf("Expected no change in NEG CR, but got len(negCR.Status.NetworkEndpointGroups) = %d", len(negCR.Status.NetworkEndpointGroups))
}
}
if tc.expectErr && !tc.expectNoopOnNegStatus {
checkCondition(t, negCR.Status.Conditions, negv1beta1.Initialized, creationTS, corev1.ConditionFalse, true)
} else if tc.crStatusPopulated {
}
if !tc.expectErr && tc.crStatusPopulated {
checkCondition(t, negCR.Status.Conditions, negv1beta1.Initialized, creationTS, corev1.ConditionTrue, false)
} else {
}
if !tc.expectErr && !tc.crStatusPopulated {
checkCondition(t, negCR.Status.Conditions, negv1beta1.Initialized, creationTS, corev1.ConditionTrue, true)
}

Expand Down
7 changes: 5 additions & 2 deletions pkg/neg/syncers/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ const (
managedByEPSControllerValue = "endpointslice-controller.k8s.io"
)

var ErrNEGNameInUse = errors.New("NEG name is already in use")

// encodeEndpoint encodes ip and instance into a single string
func encodeEndpoint(ip, instance, port string) string {
return strings.Join([]string{ip, instance, port}, separator)
Expand Down Expand Up @@ -150,11 +152,12 @@ func ensureNetworkEndpointGroup(svcNamespace, svcName, negName, zone, negService
}
if customName && neg.Description == "" {
negLogger.Error(nil, "Found Neg with custom name but empty description")
return negv1beta1.NegObjectReference{}, fmt.Errorf("neg name %s is already in use, found a custom named neg with an empty description", negName)
return negv1beta1.NegObjectReference{}, fmt.Errorf("%w: found a custom named neg %s with an empty description", ErrNEGNameInUse, negName)
}
if matches, err := utils.VerifyDescription(expectedDesc, neg.Description, negName, zone); !matches {
negLogger.Error(err, "Neg Name is already in use")
return negv1beta1.NegObjectReference{}, fmt.Errorf("neg name %s is already in use, found conflicting description: %w", negName, err)
// Wrap returned error from VerifyDescription() since we need to check if error is ErrClusterIDMismatch.
return negv1beta1.NegObjectReference{}, fmt.Errorf("%w: found conflicting description in neg %s: %w", ErrNEGNameInUse, negName, err)
}

if networkEndpointType != negtypes.NonGCPPrivateEndpointType &&
Expand Down
12 changes: 11 additions & 1 deletion pkg/utils/negdescription.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,14 @@ package utils

import (
"encoding/json"
"errors"
"fmt"

"k8s.io/klog/v2"
)

var ErrClusterIDMismatch = errors.New("NEG has mismatched cluster ID")

// Description stores the description for a BackendService.
type NegDescription struct {
ClusterUID string `json:"cluster-uid,omitempty"`
Expand Down Expand Up @@ -61,7 +64,14 @@ func VerifyDescription(expectDesc NegDescription, descString, negName, zone stri
if err != nil {
klog.Warningf("Error unmarshalling Neg Description %s err:%s", negName, err)
} else {
if desc.ClusterUID != expectDesc.ClusterUID || desc.Namespace != expectDesc.Namespace || desc.ServiceName != expectDesc.ServiceName || desc.Port != expectDesc.Port {
// Wrap the error to determine if the NEG desc conflict is due to cluster ID mismatch.
// When there is mismatch in NEG description,
// if the conflict occurs within the same cluster, NEG status should not be updated.
// otherwise, NEG status should have initialized=False condition.
if desc.ClusterUID != expectDesc.ClusterUID {
return false, fmt.Errorf("%w: expected description of NEG object %q/%q to be %+v, but got %+v", ErrClusterIDMismatch, zone, negName, expectDesc, desc)
}
if desc.Namespace != expectDesc.Namespace || desc.ServiceName != expectDesc.ServiceName || desc.Port != expectDesc.Port {
return false, fmt.Errorf("expected description of NEG object %q/%q to be %+v, but got %+v", zone, negName, expectDesc, desc)
}
}
Expand Down

0 comments on commit d994735

Please sign in to comment.