Remove the restriction that an ACNP referred ClusterGroup must be created before the ACNP #2478

Dyanngg · 2021-07-27T17:49:13Z

This is the second of two PRs that relaxes admission validations for ClusterGroup references. This PR removes the restriction that a ClusterGroup must exist before it can be referred to in Antrea ClusterNetworkPolicies. It also allows a ClusterGroup to be deleted even if it is still referred to by an ACNP.

This PR also refactors the way test resources are created in testSteps of antreapolicy_test.go. Since resource creation/deletion dependencies are no longer enforced, e2e testcases can simply use an ordered list of test resources specification to test different scenarios (i.e. ACNP is created before its referred CG and vice versa).

codecov-commenter · 2021-07-27T18:13:28Z

Codecov Report

Merging #2478 (9a44855) into main (1d148ad) will decrease coverage by 1.23%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main    #2478      +/-   ##
==========================================
- Coverage   60.58%   59.34%   -1.24%     
==========================================
  Files         286      287       +1     
  Lines       23096    23107      +11     
==========================================
- Hits        13993    13714     -279     
- Misses       7644     7956     +312     
+ Partials     1459     1437      -22

Flag	Coverage Δ
e2e-tests	`∅ <ø> (?)`
kind-e2e-tests	`45.37% <0.00%> (-2.26%)`	⬇️
unit-tests	`41.51% <33.33%> (+0.08%)`	⬆️

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

Impacted Files	Coverage Δ
...g/controller/networkpolicy/clusternetworkpolicy.go	`65.11% <ø> (-0.02%)`	⬇️
pkg/controller/networkpolicy/validate.go	`21.51% <ø> (+0.95%)`	⬆️
pkg/controller/networkpolicy/clustergroup.go	`67.59% <33.33%> (+1.85%)`	⬆️
...g/agent/apiserver/handlers/featuregates/handler.go	`0.00% <0.00%> (-82.36%)`	⬇️
...kg/apiserver/registry/system/supportbundle/rest.go	`19.54% <0.00%> (-55.18%)`	⬇️
pkg/support/dump_others.go	`0.00% <0.00%> (-51.73%)`	⬇️
pkg/support/dump.go	`8.13% <0.00%> (-49.60%)`	⬇️
...g/agent/apiserver/handlers/addressgroup/handler.go	`0.00% <0.00%> (-40.00%)`	⬇️
...agent/apiserver/handlers/appliedtogroup/handler.go	`0.00% <0.00%> (-40.00%)`	⬇️
pkg/apiserver/handlers/loglevel/handler.go	`0.00% <0.00%> (-38.47%)`	⬇️
... and 48 more

pkg/controller/networkpolicy/validate.go

abhiraut · 2021-07-30T00:43:22Z

pkg/controller/networkpolicy/clustergroup.go

+		if err != nil {
+			klog.Errorf("Unable to delete internal Group %s from store: %v", key, err)
+		}
+		n.triggerParentGroupSync(grp)


we have been triggering CNP updates from workers.. is it possible to do so in syncInternalGroup?

It will be very tricky to do so in syncInternalGroup, in case of deletion. If a group key is processed by syncInternalGroup, the controller would expect the internal group still exist in the internal group store, otherwise it won't do anything. So in this case, if we want to rely on syncInternalGroup for parent group syncs, we would need to delete the internal group from the internal store after syncInternalGroup. However, at the time of parent group sync we also want to ensure that the childGroup is deleted from internal store, so that we don't include outdated members. Since this process is async, it's hard for us to ensure that syncInternalGroup of the parent happens after the internal group deletion.

hmm im worried this comes in the path of the synchronous Delete handlers and may cause perf issue. maybe add a TODO comment here to explain why its done here now and perhaps we should look in to moving this to the workers. let me also dig deeper if we can have an alternative here..

you plan to add a TODO here? more important than ParentGroup my comment is on the triggerCNPUpdates; as that's the one i presume would be time consuming

Thanks for the reminder. Added TODO.

Dyanngg · 2021-08-06T23:25:22Z

/test-all

Dyanngg · 2021-08-10T20:42:03Z

/test-all

Dyanngg · 2021-08-11T23:41:08Z

rebased /test-all

pkg/controller/networkpolicy/clustergroup.go

Dyanngg · 2021-08-12T00:48:37Z

/test-all

antoninbas · 2021-08-23T21:46:03Z

Are we still planning to merge this for v1.3? If yes, @abhiraut could you rebase and @tnqn do you want to take a look?

abhiraut · 2021-08-23T21:55:40Z

yup. i will take care of this PR .. let us first merge the static ipblock PR #2577 and then I will rebase all conflicts together

…ated before the ACNP as well as the restriction on ClusterGroup cannot be deleted if still referred to by an ACNP. Signed-off-by: Yang Ding <dingyang@vmware.com>

Co-authored-by: abhiraut <rauta@vmware.com> Signed-off-by: abhiraut <rauta@vmware.com>

abhiraut · 2021-08-24T21:13:24Z

Are we still planning to merge this for v1.3? If yes, @abhiraut could you rebase and @tnqn do you want to take a look?

done

abhiraut · 2021-08-24T21:13:44Z

/test-all

tnqn · 2021-08-25T10:44:27Z

pkg/controller/networkpolicy/clustergroup.go

+	//  members. Since this process is async, there's no guarantee which happens first. The drawback is
+	//  that all ACNPs that refers to this CG directly will be synchronously processed once as part of
+	//  deletion handler, which could cause perf issue in scale.
+	c.triggerCNPUpdates(og)


The call is unnecessary.

If we want to rely on syncInternalGroup for ACNP syncs, the internal group needs to be deleted from the internal store after syncInternalGroup.

I don't think we need to defer deleting internal group to syncInternalGroup. triggerCNPUpdates actually just requires the group name. The association is stored in CNP store, instead of group store.

However, at the time of ACNP sync, we also want to ensure that the group is deleted from internal store already, so that we don't include outdated members. Since this process is async, there's no guarantee which happens first.

It doesn't matter whether the group is deleted from internal store already when processing ACNP. The only thing we need to ensure is triggering CNP update after syncInternalGroup, so that its data will be correct eventually.

Of course, we need to ensure c.triggerParentGroupSync() and c.triggerCNPUpdates() are called in syncInternalGroup regardless of the existence of the internal group. deferred calls can work. I don't think they really need to return error.

func (c *NetworkPolicyController) syncInternalGroup(key string) error { defer c.triggerCNPUpdates(key) defer c.triggerParentGroupSync(key) // Retrieve the internal Group corresponding to this key. ... }

tnqn · 2021-08-25T10:48:57Z

pkg/controller/networkpolicy/clustergroup.go

+		if err != nil {
+			klog.Errorf("Unable to delete internal Group %s from store: %v", key, err)
+		}
+		c.triggerParentGroupSync(grp)


Same as triggerCNPUpdates. Calling it in syncInternalGroup should be good enough. The method doesn't really require the whole group but just the group name.

abhiraut · 2021-08-25T19:44:32Z

thanks for the review Quan! let me evaluate and push a change based on that

Signed-off-by: abhiraut <rauta@vmware.com>

tnqn

LGTM, just one nit

tnqn · 2021-08-26T02:40:36Z

pkg/controller/networkpolicy/clustergroup.go

+	parentGroupObjs, err := c.internalGroupStore.GetByIndex(store.ChildGroupIndex, grp)
+	if err != nil {
+		klog.Errorf("Error retrieving parents of ClusterGroup %s: %v", grp, err)
+	}


nit: Not introduced by this PR, but it should return earlier upon error, just like triggerCNPUpdates, though the following processing is safe to run in this particular case.

Signed-off-by: abhiraut <rauta@vmware.com>

tnqn

LGTM

tnqn · 2021-08-26T09:46:51Z

/test-all

…ated before the ACNP (#2478) as well as the restriction on ClusterGroup cannot be deleted if still referred to by an ACNP. Co-authored-by: abhiraut <rauta@vmware.com> Signed-off-by: Yang Ding <dingyang@vmware.com> Signed-off-by: abhiraut <rauta@vmware.com> (cherry picked from commit d87dee1)

Dyanngg force-pushed the remove-acnp-cg-ref-restriction branch from 9320e68 to c1733da Compare July 27, 2021 22:34

Dyanngg changed the title ~~[WIP] Remove the restriction that an ACNP referred ClusterGroup must be created before the ACNP~~ Remove the restriction that an ACNP referred ClusterGroup must be created before the ACNP Jul 27, 2021

Dyanngg force-pushed the remove-acnp-cg-ref-restriction branch from c1733da to f1de0be Compare July 27, 2021 22:40

Dyanngg requested a review from abhiraut July 27, 2021 22:40

Dyanngg added this to the Antrea v1.3 release milestone Jul 27, 2021

Dyanngg added the area/network-policy/lifecycle Issues or PRs related to the network policy lifecycle. label Jul 27, 2021

Dyanngg force-pushed the remove-acnp-cg-ref-restriction branch from f1de0be to fd5faea Compare July 27, 2021 23:52

Dyanngg requested a review from edwardbadboy July 28, 2021 04:37

abhiraut reviewed Jul 30, 2021

View reviewed changes

Dyanngg force-pushed the remove-acnp-cg-ref-restriction branch from fd5faea to 6106fad Compare July 30, 2021 18:47

Dyanngg requested a review from abhiraut August 4, 2021 01:11

Dyanngg force-pushed the remove-acnp-cg-ref-restriction branch from 6106fad to 021c563 Compare August 4, 2021 01:19

abhiraut requested a review from tnqn August 6, 2021 20:26

Dyanngg force-pushed the remove-acnp-cg-ref-restriction branch from 021c563 to 40a106f Compare August 10, 2021 20:40

abhiraut previously approved these changes Aug 10, 2021

View reviewed changes

Dyanngg dismissed abhiraut’s stale review via b83346c August 11, 2021 23:40

Dyanngg force-pushed the remove-acnp-cg-ref-restriction branch from 40a106f to b83346c Compare August 11, 2021 23:40

Dyanngg requested a review from abhiraut August 11, 2021 23:41

abhiraut reviewed Aug 12, 2021

View reviewed changes

pkg/controller/networkpolicy/clustergroup.go Outdated Show resolved Hide resolved

Dyanngg force-pushed the remove-acnp-cg-ref-restriction branch from b83346c to 9fea7ce Compare August 12, 2021 00:48

abhiraut previously approved these changes Aug 12, 2021

View reviewed changes

abhiraut requested a review from GraysonWu August 17, 2021 18:28

Remove the restriction that an ACNP referred ClusterGroup must be cre…

be723fe

…ated before the ACNP as well as the restriction on ClusterGroup cannot be deleted if still referred to by an ACNP. Signed-off-by: Yang Ding <dingyang@vmware.com>

Resolve rebase conflicts

504f6b5

Co-authored-by: abhiraut <rauta@vmware.com> Signed-off-by: abhiraut <rauta@vmware.com>

abhiraut dismissed their stale review via 504f6b5 August 24, 2021 19:28

abhiraut force-pushed the remove-acnp-cg-ref-restriction branch from 9fea7ce to 504f6b5 Compare August 24, 2021 19:28

tnqn reviewed Aug 25, 2021

View reviewed changes

Address Quans comments

690c743

Signed-off-by: abhiraut <rauta@vmware.com>

tnqn previously approved these changes Aug 26, 2021

View reviewed changes

Return early on error

9a44855

Signed-off-by: abhiraut <rauta@vmware.com>

abhiraut dismissed tnqn’s stale review via 9a44855 August 26, 2021 03:45

abhiraut requested a review from tnqn August 26, 2021 03:46

tnqn approved these changes Aug 26, 2021

View reviewed changes

tnqn merged commit d87dee1 into antrea-io:main Aug 26, 2021

Remove the restriction that an ACNP referred ClusterGroup must be created before the ACNP #2478

Remove the restriction that an ACNP referred ClusterGroup must be created before the ACNP #2478

Dyanngg commented Jul 27, 2021 •

edited

codecov-commenter commented Jul 27, 2021 •

edited

abhiraut Jul 30, 2021

Dyanngg Jul 30, 2021

abhiraut Aug 4, 2021

abhiraut Aug 10, 2021 •

edited

Dyanngg Aug 10, 2021

Dyanngg commented Aug 6, 2021

Dyanngg commented Aug 10, 2021

Dyanngg commented Aug 11, 2021

Dyanngg commented Aug 12, 2021

antoninbas commented Aug 23, 2021

abhiraut commented Aug 23, 2021

abhiraut commented Aug 24, 2021

abhiraut commented Aug 24, 2021

tnqn Aug 25, 2021

tnqn Aug 25, 2021

abhiraut commented Aug 25, 2021

tnqn left a comment

tnqn Aug 26, 2021

abhiraut Aug 26, 2021

tnqn left a comment

tnqn commented Aug 26, 2021

Remove the restriction that an ACNP referred ClusterGroup must be created before the ACNP #2478

Remove the restriction that an ACNP referred ClusterGroup must be created before the ACNP #2478

Conversation

Dyanngg commented Jul 27, 2021 • edited

codecov-commenter commented Jul 27, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhiraut Aug 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg commented Aug 6, 2021

Dyanngg commented Aug 10, 2021

Dyanngg commented Aug 11, 2021

Dyanngg commented Aug 12, 2021

antoninbas commented Aug 23, 2021

abhiraut commented Aug 23, 2021

abhiraut commented Aug 24, 2021

abhiraut commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhiraut commented Aug 25, 2021

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 26, 2021

Dyanngg commented Jul 27, 2021 •

edited

codecov-commenter commented Jul 27, 2021 •

edited

abhiraut Aug 10, 2021 •

edited