diff --git a/pkg/controller/service/BUILD b/pkg/controller/service/BUILD index 883d5037667..4654d37151b 100644 --- a/pkg/controller/service/BUILD +++ b/pkg/controller/service/BUILD @@ -57,6 +57,7 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/client-go/informers:go_default_library", diff --git a/pkg/controller/service/patch.go b/pkg/controller/service/patch.go index 1364317d182..da714e133ca 100644 --- a/pkg/controller/service/patch.go +++ b/pkg/controller/service/patch.go @@ -37,11 +37,7 @@ func patch(c v1core.CoreV1Interface, oldSvc *v1.Service, newSvc *v1.Service) (*v return nil, err } - updatedSvc, err := c.Services(oldSvc.Namespace).Patch(oldSvc.Name, types.StrategicMergePatchType, patchBytes, "status") - if err != nil { - return nil, fmt.Errorf("failed to patch %q for svc %s/%s: %v", patchBytes, oldSvc.Namespace, oldSvc.Name, err) - } - return updatedSvc, nil + return c.Services(oldSvc.Namespace).Patch(oldSvc.Name, types.StrategicMergePatchType, patchBytes, "status") } func getPatchBytes(oldSvc *v1.Service, newSvc *v1.Service) ([]byte, error) { diff --git a/pkg/controller/service/service_controller_test.go b/pkg/controller/service/service_controller_test.go index c578879be82..63ff39b4dab 100644 --- a/pkg/controller/service/service_controller_test.go +++ b/pkg/controller/service/service_controller_test.go @@ -28,6 +28,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/informers" @@ -615,35 +616,61 @@ func TestProcessServiceCreateOrUpdate(t *testing.T) { } -// TestConflictWhenProcessServiceCreateOrUpdate tests if processServiceCreateOrUpdate will -// retry creating the load balancer when the update operation returns a conflict -// error. -func TestConflictWhenProcessServiceCreateOrUpdate(t *testing.T) { - svcName := "conflict-lb" - svc := newService(svcName, types.UID("123"), v1.ServiceTypeLoadBalancer) - controller, _, client := newController() - client.PrependReactor("update", "services", func(action core.Action) (bool, runtime.Object, error) { - update := action.(core.UpdateAction) - return true, update.GetObject(), apierrors.NewConflict(action.GetResource().GroupResource(), svcName, errors.New("Object changed")) - }) +// TestProcessServiceCreateOrUpdateK8sError tests processServiceCreateOrUpdate +// with various kubernetes errors. +func TestProcessServiceCreateOrUpdateK8sError(t *testing.T) { + svcName := "svc-k8s-err" + conflictErr := apierrors.NewConflict(schema.GroupResource{}, svcName, errors.New("Object conflict")) + notFoundErr := apierrors.NewNotFound(schema.GroupResource{}, svcName) - if err := controller.processServiceCreateOrUpdate(svc, svcName); err == nil { - t.Fatalf("controller.processServiceCreateOrUpdate() = nil, want error") + testCases := []struct { + desc string + k8sErr error + expectErr error + }{ + { + desc: "conflict error", + k8sErr: conflictErr, + expectErr: fmt.Errorf("failed to update load balancer status: %v", conflictErr), + }, + { + desc: "not found error", + k8sErr: notFoundErr, + expectErr: nil, + }, } - errMsg := "Error syncing load balancer" - if gotEvent := func() bool { - events := controller.eventRecorder.(*record.FakeRecorder).Events - for len(events) > 0 { - e := <-events - if strings.Contains(e, errMsg) { - return true + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + svc := newService(svcName, types.UID("123"), v1.ServiceTypeLoadBalancer) + controller, _, client := newController() + client.PrependReactor("patch", "services", func(action core.Action) (bool, runtime.Object, error) { + return true, nil, tc.k8sErr + }) + + if err := controller.processServiceCreateOrUpdate(svc, svcName); !reflect.DeepEqual(err, tc.expectErr) { + t.Fatalf("processServiceCreateOrUpdate() = %v, want %v", err, tc.expectErr) } - } - return false - }(); !gotEvent { - t.Errorf("controller.processServiceCreateOrUpdate() = can't find sync error event, want event contains %q", errMsg) + if tc.expectErr == nil { + return + } + + errMsg := "Error syncing load balancer" + if gotEvent := func() bool { + events := controller.eventRecorder.(*record.FakeRecorder).Events + for len(events) > 0 { + e := <-events + if strings.Contains(e, errMsg) { + return true + } + } + return false + }(); !gotEvent { + t.Errorf("processServiceCreateOrUpdate() = can't find sync error event, want event contains %q", errMsg) + } + }) } + } func TestSyncService(t *testing.T) {