Skip to content

Commit

Permalink
Remove Services.Get() from syncNegStatusAnnotation()
Browse files Browse the repository at this point in the history
  • Loading branch information
gauravkghildiyal authored and sawsa307 committed Sep 2, 2023
1 parent d17ab66 commit bcfebd7
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 5 deletions.
15 changes: 11 additions & 4 deletions pkg/neg/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -683,19 +683,26 @@ func (c *Controller) syncNegStatusAnnotation(namespace, name string, portMap neg
if err != nil {
return err
}
coreClient := c.client.CoreV1()
service, err := coreClient.Services(namespace).Get(context.TODO(), name, metav1.GetOptions{})
obj, exists, err := c.serviceLister.GetByKey(getServiceKey(namespace, name).Key())
if err != nil {
return err
}
if !exists {
// Service no longer exists so doesn't require any update.
return nil
}
service, ok := obj.(*apiv1.Service)
if !ok {
return fmt.Errorf("cannot convert obj to Service; obj=%T", obj)
}

// Remove NEG Status Annotation when no NEG is needed
if len(portMap) == 0 {
if _, ok := service.Annotations[annotations.NEGStatusKey]; ok {
newSvcObjectMeta := service.ObjectMeta.DeepCopy()
delete(newSvcObjectMeta.Annotations, annotations.NEGStatusKey)
klog.V(2).Infof("Removing NEG status annotation from service: %s/%s", namespace, name)
return patch.PatchServiceObjectMetadata(coreClient, service, *newSvcObjectMeta)
return patch.PatchServiceObjectMetadata(c.client.CoreV1(), service, *newSvcObjectMeta)
}
// service doesn't have the expose NEG annotation and doesn't need update
return nil
Expand All @@ -717,7 +724,7 @@ func (c *Controller) syncNegStatusAnnotation(namespace, name string, portMap neg
}
newSvcObjectMeta.Annotations[annotations.NEGStatusKey] = annotation
klog.V(2).Infof("Updating NEG visibility annotation %q on service %s/%s.", annotation, namespace, name)
return patch.PatchServiceObjectMetadata(coreClient, service, *newSvcObjectMeta)
return patch.PatchServiceObjectMetadata(c.client.CoreV1(), service, *newSvcObjectMeta)
}

// syncDestinationRuleNegStatusAnnotation syncs the destinationrule related neg status annotation
Expand Down
10 changes: 9 additions & 1 deletion pkg/neg/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ func TestSyncNegAnnotation(t *testing.T) {
controller := newTestController(fake.NewSimpleClientset())
defer controller.stop()
svcClient := controller.client.CoreV1().Services(testServiceNamespace)
newTestService(controller, false, []int32{})
svc := newTestService(controller, false, []int32{})
namespace := testServiceNamespace
name := testServiceName
namer := controller.namer
Expand Down Expand Up @@ -732,6 +732,7 @@ func TestSyncNegAnnotation(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
controller.serviceLister.Add(svc)
controller.syncNegStatusAnnotation(namespace, name, tc.previousPortMap)
svc, _ := svcClient.Get(context.TODO(), name, metav1.GetOptions{})

Expand All @@ -741,6 +742,10 @@ func TestSyncNegAnnotation(t *testing.T) {
}
validateServiceStateAnnotation(t, svc, oldSvcPorts, controller.namer)

// Mimic any Update calls to the API Server by manually updating the
// informer cache.
controller.serviceLister.Update(svc)

controller.syncNegStatusAnnotation(namespace, name, tc.portMap)
svc, _ = svcClient.Get(context.TODO(), name, metav1.GetOptions{})

Expand All @@ -749,6 +754,9 @@ func TestSyncNegAnnotation(t *testing.T) {
svcPorts = append(svcPorts, port.ServicePort)
}
validateServiceStateAnnotation(t, svc, svcPorts, controller.namer)

// Reset state of controller for the next test run.
controller.serviceLister.Delete(svc)
})
}
}
Expand Down

0 comments on commit bcfebd7

Please sign in to comment.