Update Pod annotations for Service type change #1936

chauhanshubham · 2021-03-03T12:13:05Z

includes a UT for the same.
Fixes #1910

codecov-io · 2021-03-03T12:59:35Z

Codecov Report

Merging #1936 (347cfa1) into main (5752f96) will increase coverage by 0.48%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1936      +/-   ##
==========================================
+ Coverage   61.57%   62.06%   +0.48%     
==========================================
  Files         265      265              
  Lines       19770    19774       +4     
==========================================
+ Hits        12174    12272      +98     
+ Misses       6333     6233     -100     
- Partials     1263     1269       +6

Flag	Coverage Δ
e2e-tests	`22.57% <0.00%> (?)`
kind-e2e-tests	`52.82% <0.00%> (-0.05%)`	⬇️
unit-tests	`41.68% <0.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/nodeportlocal/k8s/npl_controller.go	`60.15% <0.00%> (-0.98%)`	⬇️
pkg/agent/stats/collector.go	`94.61% <0.00%> (-3.85%)`	⬇️
pkg/apiserver/storage/ram/watch.go	`90.38% <0.00%> (-3.85%)`	⬇️
pkg/agent/openflow/client.go	`60.37% <0.00%> (+0.53%)`	⬆️
...ntroller/networkpolicy/networkpolicy_controller.go	`71.10% <0.00%> (+0.97%)`	⬆️
pkg/agent/agent.go	`48.92% <0.00%> (+1.19%)`	⬆️
pkg/ovs/openflow/ofctrl_builder.go	`56.46% <0.00%> (+1.26%)`	⬆️
pkg/ovs/ovsconfig/ovs_client.go	`51.73% <0.00%> (+2.24%)`	⬆️
pkg/agent/openflow/pipeline.go	`73.22% <0.00%> (+3.10%)`	⬆️
pkg/agent/openflow/cookie/allocator.go	`67.74% <0.00%> (+3.22%)`	⬆️
... and 1 more

antoninbas

out-of-curiosity, do you think it would make sense to log a warning in the Agent's NPL controller when processing a NodePort Service which has the NPL enabled annotation?

antoninbas · 2021-03-03T20:03:25Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

-		podKeys = oldPodSet.Difference(newPodSet).Union(newPodSet.Difference(oldPodSet))
+		oldPodSet = sets.NewString(c.getPodsFromService(oldSvc)...)
+		newPodSet = sets.NewString(c.getPodsFromService(newSvc)...)
+		podKeys = podKeys.Union(oldPodSet.Difference(newPodSet).Union(newPodSet.Difference(oldPodSet)))


could we write our own SymmetricDifference method instead of doing oldPodSet.Difference(newPodSet).Union(newPodSet.Difference(oldPodSet))? It will be more readable and it may also be more efficient since we would not be building intermediate sets.

+1, maybe the difference is minor here, but I found using Union when calculating the union of many sets affected the performance a lot (in particular, the TestInitXLargeScaleWithOneNamespace test) as it always makes a new set and copies all items when merging each set. I added a function to improve it in #1938. Maybe we could have a separate PR to add SymmetricDifference in same place and replace all such usage.

antoninbas · 2021-03-03T20:05:32Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

+	var oldPodSet, newPodSet sets.String
+	if oldSvc.Spec.Type != newSvc.Spec.Type {
+		newPodSet = sets.NewString(c.getPodsFromService(newSvc)...)
+		podKeys = podKeys.Union(newPodSet)
+	}
+
+	if !reflect.DeepEqual(oldSvc.Spec.Selector, newSvc.Spec.Selector) {
 		// Disjunctive union of Pods from both Service sets.
-		oldPodSet := sets.NewString(c.getPodsFromService(oldSvc)...)
-		newPodSet := sets.NewString(c.getPodsFromService(newSvc)...)
-		podKeys = oldPodSet.Difference(newPodSet).Union(newPodSet.Difference(oldPodSet))
+		oldPodSet = sets.NewString(c.getPodsFromService(oldSvc)...)
+		newPodSet = sets.NewString(c.getPodsFromService(newSvc)...)
+		podKeys = podKeys.Union(oldPodSet.Difference(newPodSet).Union(newPodSet.Difference(oldPodSet)))


It seems that this code does not really need to execute in case of an annotation change (oldSvcAnnotation != newSvcAnnotation), should it be in an else block?

If I look at this edge case:

oldSvcAnnotation is true

newSvcAnnotation is false

oldSvc.Spec.Type != newSvc.Spec.Type

oldSvc.Spec.Selector != newSvc.Spec.Selector
You actually end up enqueuing both old Pods and new Pods, when you only need to enqueue old Pods.

jianjuns · 2021-03-03T22:23:38Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

+
+	var oldPodSet, newPodSet sets.String
+	if oldSvc.Spec.Type != newSvc.Spec.Type {
+		newPodSet = sets.NewString(c.getPodsFromService(newSvc)...)


What Service types we care about? In all the Service type change case, we always use the newPodSet?

If nplAnnotation changes from false to true, the newPortSet is already counted in, and we can avoid getting the set twice?

jianjuns · 2021-03-08T18:40:51Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

 	if oldSvc.Spec.Type != newSvc.Spec.Type {
-		newPodSet = sets.NewString(c.getPodsFromService(newSvc)...)
+		if newPodSet != nil {


Again, what Service types we care about? In all the Service type change case, we should always process the newPodSet?

We essentially care about transitions from non-nodeport -> nodeport and vice-versa.
In cases of Service Type updates, I anticipate no change in the set of Pods (considering selectors to be identical at this moment), so we can just check the newPodSet for this, since old one must be the same.

In cases of Service Type updates, I anticipate no change in the set of Pods (considering selectors to be identical at this moment), so we can just check the newPodSet for this, since old one must be the same.

Can you clarify this? I don't think there is any guarantee that the Service Type update will be the only change in the Service spec. The selector may change as well. Even if the user first updated the Service Type, then updated the Selector separately, these 2 updates may be combined and the update handler may be called a single time.

that is correct, the selector may change as well in the same update, in which case it should come in the following conditional, where it compares the selectors in the spec.

antoninbas

I believe the logic is still not correct. With the latest code it seems that if only the annotation changes and nothing else, we do not process any Pod, which doesn't seem right.

I suggest the following logic, which I find easier to read and hopefully should be correct:

if newSvcAnnotation == "true" && newSvc.Spec.Type == corev1.ServiceTypeNodePort {
  klog.Warningf("Service %s is of type NodePort and cannot be used for NodePortLocal, the '%s' annotation will have no effect", newSvc.Name, NPLEnabledAnnotationKey)
}

var oldPodSet, newPodSet sets.String
sameSelector := reflect.DeepEqual(oldSvc.Spec.Selector, newSvc.Spec.Selector)
oldPodSet = sets.NewString(c.getPodsFromService(oldSvc)...)
if sameSelector {
    newPodSet = oldPodSet
} else {
    newPodSet = sets.NewString(c.getPodsFromService(newSvc)...)
}

podKeys := sets.String{}
oldNPLEnabled = oldSvcAnnotation == "true" && oldSvc.Spec.Type != corev1.ServiceTypeNodePort
newNPLEnabled = newSvcAnnotation == "true" && newSvc.Spec.Type != corev1.ServiceTypeNodePort
switch {
  case oldNPLEnabled && newNPLEnabled && !sameSelector:
      podKeys = utilsets.SymmetricDifference(oldPodSet, newPodSet)
  case oldNPLEnabled && !newNPLEnabled:
      podKeys = oldPodSet
   case newNPLEnabled && !oldNPLEnabled:
      podKeys = newPodSet
}

What do you think? It should take care of Jianjun's comment as well.

Also please rebase the PR and resolve conflicts.

jianjuns · 2021-03-11T02:05:58Z

@antoninbas : but could we avoid calling getPodsFromService() when not needed, as it is expensive?

antoninbas · 2021-03-11T02:07:47Z

@antoninbas : but could we avoid calling getPodsFromService() when not needed, as it is expensive?

sure, that can be moved to the individual case statements as needed

chauhanshubham · 2021-03-15T17:14:56Z

I believe the logic is still not correct. With the latest code it seems that if only the annotation changes and nothing else, we do not process any Pod, which doesn't seem right.

Missed the podKeys initialization while checking for annotation changes, the latest commit takes care of that. We can look at refactoring the function, if it looks unreadable right now, but I feel it has a logical sequencing to it that separates out various Service key/value comparisons.

jianjuns · 2021-03-15T20:24:09Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

-	} else if !reflect.DeepEqual(oldSvc.Spec.Selector, newSvc.Spec.Selector) {
+	}
+
+	if oldSvc.Spec.Type != newSvc.Spec.Type {


Should we check one of old or new type is NodePort here, if we just care about changing to NodePort or changing from NodePort?

chauhanshubham · 2021-03-15T21:13:49Z

I think in cases where Service annotation or Service type changes, the Pod set is not affected, so maybe we can just rely on the Pods from the new Service. The Pod sets will only differ when the selector changes.
I tried simplifying the code using this. What do you guys think?
Also a few additions to the Unit tests. The tests now cover changes in Service annotations, type, selectors (invalid to valid and vice-versa).

chauhanshubham · 2021-03-15T21:34:36Z

Also maybe this

if !reflect.DeepEqual(oldSvc.Spec.Selector, newSvc.Spec.Selector) {
	// Disjunctive union of Pods from both Service sets.
	oldPodSet := sets.NewString(c.getPodsFromService(oldSvc)...)
	newPodSet := sets.NewString(c.getPodsFromService(newSvc)...)
	podKeys = podKeys.Union(utilsets.SymmetricDifference(oldPodSet, newPodSet))
}

can be simplified further to

if !reflect.DeepEqual(oldSvc.Spec.Selector, newSvc.Spec.Selector) {
	// Disjunctive union of Pods from both Service sets.
	oldPodSet := sets.NewString(c.getPodsFromService(oldSvc)...)
	if podKeys == nil {
		podKeys = sets.NewString(c.getPodsFromService(newSvc)...)
	}
	podKeys = utilsets.SymmetricDifference(oldPodSet, newPodSet)
}

Which essentially means that
podkeys = podkeys union (oldPodSet symmetric-difference newPodSet)
is similar to
podkeys = podkeys symmetric-difference oldPodSet
since, prior to the selector comparison we are ensuring that podkeys can be either nil or equal to newPodSet.

This is not part of the PR yet, I can add it in the next commit, if this sounds right to you all.

EDIT: I think symmetricDifference is still required

jianjuns · 2021-03-15T22:23:49Z

The proposal looks good to me. I am fine not to optimize Service type change further, as it is rather uncommon.

tnqn · 2021-03-18T15:51:55Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

+		if podKeys == nil {
+			podKeys = sets.NewString(c.getPodsFromService(newSvc)...)
+		}
+		podKeys = utilsets.SymmetricDifference(oldPodSet, podKeys)


If annotation/Type is updated as well as Selector, don't it need to process the pods in the intersection?
For example: update a service's annotation from true to false and its selector from app=foo to tier=frontend, shouldn't all pods matching app=foo be processed?

antoninbas · 2021-04-06T02:04:22Z

@chauhanshubham could we try to merge this for the v1.0 release (Wednesday)?

includes a UT for the same. Fixes antrea-io#1910

jianjuns

LGTM. Thanks for addressing the comments!

antoninbas

LGTM

antoninbas · 2021-04-06T22:29:06Z

/test-all

Fixes antrea-io#1910

Fixes #1910

vmwclabot added the cla-not-required label Mar 3, 2021

chauhanshubham requested review from monotosh-avi, jianjuns, tnqn and antoninbas March 3, 2021 12:13

antoninbas reviewed Mar 3, 2021

View reviewed changes

jianjuns reviewed Mar 3, 2021

View reviewed changes

jianjuns reviewed Mar 8, 2021

View reviewed changes

antoninbas reviewed Mar 10, 2021

View reviewed changes

chauhanshubham force-pushed the svctype-npl branch 2 times, most recently from 71d7068 to f894784 Compare March 15, 2021 07:12

jianjuns reviewed Mar 15, 2021

View reviewed changes

tnqn reviewed Mar 18, 2021

View reviewed changes

antoninbas added this to the Antrea v1.0 release milestone Mar 30, 2021

shubhamavi added 6 commits April 6, 2021 18:08

Update Pod annotations for Service type change,

0f7c97d

includes a UT for the same. Fixes antrea-io#1910

addressing review comments, avoid re-computing sets

b0a370a

initialize podKeys on annotation change, nil check fix

ed8dc69

review comments, simplify Service enqueue conditionals, UT additions

941eb69

simplify podKey set computation for Service selector update

a051ef2

addressing review comments

347cfa1

chauhanshubham force-pushed the svctype-npl branch from f5c8575 to 347cfa1 Compare April 6, 2021 12:38

jianjuns approved these changes Apr 6, 2021

View reviewed changes

antoninbas approved these changes Apr 6, 2021

View reviewed changes

tnqn approved these changes Apr 7, 2021

View reviewed changes

tnqn merged commit b73ab99 into antrea-io:main Apr 7, 2021

antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Apr 30, 2021

Update Pod annotations for Service type change (antrea-io#1936)

b7b8429

Fixes antrea-io#1910

antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Apr 30, 2021

Update Pod annotations for Service type change (antrea-io#1936)

ae40d4f

Fixes antrea-io#1910

antoninbas pushed a commit that referenced this pull request Apr 30, 2021

Update Pod annotations for Service type change (#1936)

a88ae7a

Fixes #1910

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Pod annotations for Service type change #1936

Update Pod annotations for Service type change #1936

chauhanshubham commented Mar 3, 2021

codecov-io commented Mar 3, 2021 •

edited

Loading

antoninbas left a comment

antoninbas Mar 3, 2021

tnqn Mar 4, 2021

antoninbas Mar 3, 2021

jianjuns Mar 3, 2021

jianjuns Mar 3, 2021

jianjuns Mar 8, 2021

chauhanshubham Mar 8, 2021

antoninbas Mar 8, 2021

chauhanshubham Mar 9, 2021

antoninbas left a comment

jianjuns commented Mar 11, 2021

antoninbas commented Mar 11, 2021

chauhanshubham commented Mar 15, 2021

jianjuns Mar 15, 2021

chauhanshubham commented Mar 15, 2021

chauhanshubham commented Mar 15, 2021 •

edited

Loading

jianjuns commented Mar 15, 2021

tnqn Mar 18, 2021

antoninbas commented Apr 6, 2021

jianjuns left a comment

antoninbas left a comment

antoninbas commented Apr 6, 2021

Update Pod annotations for Service type change #1936

Update Pod annotations for Service type change #1936

Conversation

chauhanshubham commented Mar 3, 2021

codecov-io commented Mar 3, 2021 • edited Loading

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

jianjuns commented Mar 11, 2021

antoninbas commented Mar 11, 2021

chauhanshubham commented Mar 15, 2021

Choose a reason for hiding this comment

chauhanshubham commented Mar 15, 2021

chauhanshubham commented Mar 15, 2021 • edited Loading

jianjuns commented Mar 15, 2021

Choose a reason for hiding this comment

antoninbas commented Apr 6, 2021

jianjuns left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Apr 6, 2021

codecov-io commented Mar 3, 2021 •

edited

Loading

chauhanshubham commented Mar 15, 2021 •

edited

Loading