From c2d3e54551e5cd84660b212b9f5bea2fdb64a43a Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Mon, 10 Feb 2020 21:47:07 +0800 Subject: [PATCH] EndpointSliceTracker should track updated resource version During EndpointSlice reconcilation, EndpointSliceTracker is supposed to track expected EndpointSlice resource versions so that external changes to them can be detected. But it actually tracked the stale resource version and resulted in every Service was handled twice as it always received an EndpointSlice update with a different resource version but was actually created/updated by itself during the first processing. --- pkg/controller/endpointslice/reconciler.go | 8 ++++---- pkg/controller/endpointslice/reconciler_test.go | 15 +++++++++++++++ pkg/controller/endpointslice/utils_test.go | 6 ++++++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/pkg/controller/endpointslice/reconciler.go b/pkg/controller/endpointslice/reconciler.go index 0d2ff70e011..4adc7073a3f 100644 --- a/pkg/controller/endpointslice/reconciler.go +++ b/pkg/controller/endpointslice/reconciler.go @@ -206,7 +206,7 @@ func (r *reconciler) finalize( for _, endpointSlice := range slicesToCreate { addTriggerTimeAnnotation(endpointSlice, triggerTime) - _, err := r.client.DiscoveryV1beta1().EndpointSlices(service.Namespace).Create(context.TODO(), endpointSlice, metav1.CreateOptions{}) + createdSlice, err := r.client.DiscoveryV1beta1().EndpointSlices(service.Namespace).Create(context.TODO(), endpointSlice, metav1.CreateOptions{}) if err != nil { // If the namespace is terminating, creates will continue to fail. Simply drop the item. if errors.HasStatusCause(err, corev1.NamespaceTerminatingCause) { @@ -214,18 +214,18 @@ func (r *reconciler) finalize( } errs = append(errs, fmt.Errorf("Error creating EndpointSlice for Service %s/%s: %v", service.Namespace, service.Name, err)) } else { - r.endpointSliceTracker.Update(endpointSlice) + r.endpointSliceTracker.Update(createdSlice) metrics.EndpointSliceChanges.WithLabelValues("create").Inc() } } for _, endpointSlice := range slicesToUpdate { addTriggerTimeAnnotation(endpointSlice, triggerTime) - _, err := r.client.DiscoveryV1beta1().EndpointSlices(service.Namespace).Update(context.TODO(), endpointSlice, metav1.UpdateOptions{}) + updatedSlice, err := r.client.DiscoveryV1beta1().EndpointSlices(service.Namespace).Update(context.TODO(), endpointSlice, metav1.UpdateOptions{}) if err != nil { errs = append(errs, fmt.Errorf("Error updating %s EndpointSlice for Service %s/%s: %v", endpointSlice.Name, service.Namespace, service.Name, err)) } else { - r.endpointSliceTracker.Update(endpointSlice) + r.endpointSliceTracker.Update(updatedSlice) metrics.EndpointSliceChanges.WithLabelValues("update").Inc() } } diff --git a/pkg/controller/endpointslice/reconciler_test.go b/pkg/controller/endpointslice/reconciler_test.go index f305ca2478e..4308df209b7 100644 --- a/pkg/controller/endpointslice/reconciler_test.go +++ b/pkg/controller/endpointslice/reconciler_test.go @@ -62,6 +62,7 @@ func TestReconcileEmpty(t *testing.T) { assert.Equal(t, svc.Name, slices[0].Labels[discovery.LabelServiceName]) assert.EqualValues(t, []discovery.EndpointPort{}, slices[0].Ports) assert.EqualValues(t, []discovery.Endpoint{}, slices[0].Endpoints) + expectTrackedResourceVersion(t, r.endpointSliceTracker, &slices[0], "100") expectMetrics(t, expectedMetrics{desiredSlices: 1, actualSlices: 1, desiredEndpoints: 0, addedPerSync: 0, removedPerSync: 0, numCreated: 1, numUpdated: 0, numDeleted: 0}) } @@ -190,6 +191,8 @@ func TestReconcile1Pod(t *testing.T) { t.Errorf("Expected endpoint: %+v, got: %+v", testCase.expectedEndpoint, endpoint) } + expectTrackedResourceVersion(t, r.endpointSliceTracker, &slice, "100") + expectMetrics(t, expectedMetrics{desiredSlices: 1, actualSlices: 1, desiredEndpoints: 1, addedPerSync: 1, removedPerSync: 0, numCreated: 1, numUpdated: 0, numDeleted: 0}) }) } @@ -221,6 +224,7 @@ func TestReconcile1EndpointSlice(t *testing.T) { assert.Equal(t, svc.Name, slices[0].Labels[discovery.LabelServiceName]) assert.EqualValues(t, []discovery.EndpointPort{}, slices[0].Ports) assert.EqualValues(t, []discovery.Endpoint{}, slices[0].Endpoints) + expectTrackedResourceVersion(t, r.endpointSliceTracker, &slices[0], "200") expectMetrics(t, expectedMetrics{desiredSlices: 1, actualSlices: 1, desiredEndpoints: 0, addedPerSync: 0, removedPerSync: 0, numCreated: 0, numUpdated: 1, numDeleted: 0}) } @@ -821,6 +825,17 @@ func expectActions(t *testing.T, actions []k8stesting.Action, num int, verb, res } } +func expectTrackedResourceVersion(t *testing.T, tracker *endpointSliceTracker, slice *discovery.EndpointSlice, expectedRV string) { + rrv := tracker.relatedResourceVersions(slice) + rv, tracked := rrv[slice.Name] + if !tracked { + t.Fatalf("Expected EndpointSlice %s to be tracked", slice.Name) + } + if rv != expectedRV { + t.Errorf("Expected ResourceVersion of %s to be %s, got %s", slice.Name, expectedRV, rv) + } +} + func portsAndAddressTypeEqual(slice1, slice2 discovery.EndpointSlice) bool { return apiequality.Semantic.DeepEqual(slice1.Ports, slice2.Ports) && apiequality.Semantic.DeepEqual(slice1.AddressType, slice2.AddressType) } diff --git a/pkg/controller/endpointslice/utils_test.go b/pkg/controller/endpointslice/utils_test.go index 4d9b19e815e..a038f1d8b3b 100644 --- a/pkg/controller/endpointslice/utils_test.go +++ b/pkg/controller/endpointslice/utils_test.go @@ -401,9 +401,15 @@ func newClientset() *fake.Clientset { endpointSlice.ObjectMeta.Name = fmt.Sprintf("%s-%s", endpointSlice.ObjectMeta.GenerateName, rand.String(8)) endpointSlice.ObjectMeta.GenerateName = "" } + endpointSlice.ObjectMeta.ResourceVersion = "100" return false, endpointSlice, nil })) + client.PrependReactor("update", "endpointslices", k8stesting.ReactionFunc(func(action k8stesting.Action) (bool, runtime.Object, error) { + endpointSlice := action.(k8stesting.CreateAction).GetObject().(*discovery.EndpointSlice) + endpointSlice.ObjectMeta.ResourceVersion = "200" + return false, endpointSlice, nil + })) return client }