Adds retry mechanism for Network Policy #2809

trozet · 2022-02-08T20:45:39Z

Leverages the same mechanism used by pods, except it also handles
retrying deletion.

Signed-off-by: Tim Rozet trozet@redhat.com

go-controller/pkg/ovn/ovn.go

trozet · 2022-02-08T22:44:11Z

Unrelated flake?

[Fail] OVN Namespace Operations on startup [It] creates an address set for existing nodes when the host network traffic namespace is created

trozet · 2022-02-08T22:44:17Z

/retest

trozet · 2022-02-08T22:53:46Z

Another unrelated flake

[Fail] Informer Event Handler Tests [It] adds existing pod and processes an update event
/home/runner/work/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/informer/informer_test.go:276

trozet · 2022-02-08T22:53:53Z

/retest

coveralls · 2022-02-08T23:10:59Z

Coverage increased (+0.07%) to 50.785% when pulling 25669a4 on trozet:retry_network_policy_on_fail into 5cd51e4 on ovn-org:master.

trozet · 2022-02-09T14:44:49Z

Looks like 1 test case failed in dualstack:
should enforce policy to allow traffic only from a pod in a different namespace based on PodSelector and NamespaceSelector

I'll test it out manually to make sure it was just a flake.

trozet · 2022-02-09T15:31:14Z

test case passes locally for me, and no obvious errors in the CI log

go-controller/pkg/ovn/namespace.go

go-controller/pkg/ovn/policy_retry.go

go-controller/pkg/ovn/policy.go

go-controller/pkg/factory/factory.go

go-controller/pkg/ovn/policy_retry.go

go-controller/pkg/ovn/ovn.go

tssurya · 2022-02-10T14:34:41Z

go-controller/pkg/ovn/ovn.go

-				oc.addNetworkPolicy(newPolicy)
+				oc.checkAndSkipRetryPolicy(oldPolicy)
+				if err := oc.deleteNetworkPolicy(oldPolicy, nil); err != nil {
+					oc.initRetryPolicyWithDelete(oldPolicy, nil)


this is a bit complicated. specially since we have initRetryPolicyWithDelete and initRetryPolicy.... need to go back and forth between functions to see what they do...should we add a comment to this part to explain what we do so that 4 months from now we are good?

self-note-attached-for-when-I-come-back-to-this-PR-in-the-future

Nice diagram! Initially I made just a single "initRetryPolicy(old, new)" type of of function, but then I thought that was more confusing when reading the code in other places. Also, it made things weird about implying what a nil old, or new object means.

yeah for me it was confusing going back and forth between pod_retry code and ovn handler code but in the end I think I grasped the logic, its genius with all this skip and unskip :)

@dcbw idea not me :)

I don't think I want any of this to be called genius...

go-controller/pkg/ovn/policy.go

tssurya · 2022-02-10T22:01:19Z

go-controller/pkg/ovn/policy.go

+		np = foundNp
+	}
+
+	if err := oc.destroyNetworkPolicy(np, nsInfo); err != nil {


you can reach here with nsInfo==nil and np!=nil case, I think your nested if's broke the logic of the older code. Is that on purpose?

the intention with this change was to be able to delete without nsInfo, because it is possible that the namespace could be gone by the time we want to delete a network policy. Here is a case where that may happen:

// Now do nsinfo operations to set the policy nsInfo, nsUnlock, err = oc.ensureNamespaceLocked(policy.Namespace, false, nil) if err != nil { // rollback network policy if err := oc.deleteNetworkPolicy(policy, np); err != nil { // rollback failed, add to retry to cleanup oc.addDeleteToRetryPolicy(policy, np) } return fmt.Errorf("unable to ensure namespace for network policy: %s, namespace: %s, error: %v", policy.Name, policy.Namespace, err) }

go-controller/pkg/ovn/policy_retry.go

tssurya · 2022-02-10T22:15:36Z

go-controller/pkg/ovn/policy_retry.go

+			klog.Infof("Network Policy Retry: %s retry network policy setup", namespacedName)
+
+			// check if we need to delete anything
+			if npEntry.oldPolicy != nil {


so essentially this is your trigger right?

if oldPolicy is set, you know its a delete retry versus

if newpolicy is set (policyToCreate to be more precise), we know its a add retry

if both are set then? you make sure delete is done first and don't retry add huh based on L55's continue?

we don't really care what the state is here, we just know if a retry entry has a delete we should remove it first, if that fails we should not try to add the new thing. They may conflict.

go-controller/pkg/ovn/policy_retry.go

tssurya · 2022-02-11T16:40:26Z

go-controller/pkg/ovn/policy_retry.go

+					continue
+				}
+				// successfully cleaned up old policy, remove it from the retry cache
+				npEntry.newPolicy = nil


we should probably just delete the entry from cache entirely at this point right?

yeah I guess this wasn't really necessary. We remove from the cache a few lines later, I don't think it hurts to have it though.

go-controller/pkg/ovn/policy_test.go

Leverages the same mechanism used by pods, except it also handles retrying deletion. Signed-off-by: Tim Rozet <trozet@redhat.com>

dcbw · 2022-02-11T21:55:10Z

I think it LGTM now. Trying a downstream run openshift/ovn-kubernetes#953 just for kicks.

Update: downstream seems OK.

tssurya

/lgtm
we can do the clean ups in later prs...

tssurya · 2022-02-14T09:30:04Z

go-controller/pkg/ovn/policy_retry.go

+					npEntry.timeStamp = time.Now()
+					continue
+				}
+				// successfully cleaned up old policy, remove it from the retry cache


nit:

Suggested change

// successfully cleaned up old policy, remove it from the retry cache

// successfully created new policy, remove it from retry cache.

tssurya · 2022-02-14T09:31:51Z

I think it LGTM now. Trying a downstream run openshift/ovn-kubernetes#953 just for kicks.

Update: downstream seems OK.

yeah, the thing with these retries is, I doubt we have any real time e2e's where the creation and/or deletion fails and we keep retrying...

Updating the ACL logging by checking the namespace was added to the network policy handler as part of: ovn-org#2809 However, the policy was not added to the namespace before updating the logging levels. Therefore the current policy would not be processed during logging level updates in this path. Reported-by: Andrew Stoycos <astoycos@redhat.com> Signed-off-by: Tim Rozet <trozet@redhat.com>

Updating the ACL logging by checking the namespace was added to the network policy handler as part of: ovn-org/ovn-kubernetes#2809 However, the policy was not added to the namespace before updating the logging levels. Therefore the current policy would not be processed during logging level updates in this path. Reported-by: Andrew Stoycos <astoycos@redhat.com> Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit 8e2b643)

Updating the ACL logging by checking the namespace was added to the network policy handler as part of: ovn-org/ovn-kubernetes#2809 However, the policy was not added to the namespace before updating the logging levels. Therefore the current policy would not be processed during logging level updates in this path. Reported-by: Andrew Stoycos <astoycos@redhat.com> Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit 8e2b643) (cherry picked from commit 1cdf58a)

Updating the ACL logging by checking the namespace was added to the network policy handler as part of: ovn-org/ovn-kubernetes#2809 However, the policy was not added to the namespace before updating the logging levels. Therefore the current policy would not be processed during logging level updates in this path. Reported-by: Andrew Stoycos <astoycos@redhat.com> Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit 8e2b643) (cherry picked from commit 1cdf58a) (cherry picked from commit fee98b4)

trozet requested review from astoycos and dcbw February 8, 2022 20:45

trozet commented Feb 8, 2022

View reviewed changes

go-controller/pkg/ovn/ovn.go Outdated Show resolved Hide resolved

trozet force-pushed the retry_network_policy_on_fail branch from b8f70ae to 28f1cf9 Compare February 8, 2022 22:24

tssurya self-requested a review February 9, 2022 23:08

dcbw reviewed Feb 10, 2022

View reviewed changes

go-controller/pkg/ovn/namespace.go Show resolved Hide resolved

dcbw reviewed Feb 10, 2022

View reviewed changes

go-controller/pkg/ovn/policy_retry.go Outdated Show resolved Hide resolved

dcbw reviewed Feb 10, 2022

View reviewed changes

go-controller/pkg/ovn/policy.go Outdated Show resolved Hide resolved

dcbw reviewed Feb 10, 2022

View reviewed changes

go-controller/pkg/ovn/policy.go Show resolved Hide resolved

tssurya requested changes Feb 10, 2022

View reviewed changes

tssurya reviewed Feb 11, 2022

View reviewed changes

trozet commented Feb 11, 2022

View reviewed changes

go-controller/pkg/ovn/policy_test.go Outdated Show resolved Hide resolved

tssurya mentioned this pull request Feb 11, 2022

Retry delete logical port on failures #2787

Merged

trozet force-pushed the retry_network_policy_on_fail branch 2 times, most recently from 6ae2298 to da16b77 Compare February 11, 2022 18:56

Adds retry mechanism for Network Policy

25669a4

Leverages the same mechanism used by pods, except it also handles retrying deletion. Signed-off-by: Tim Rozet <trozet@redhat.com>

trozet force-pushed the retry_network_policy_on_fail branch from da16b77 to 25669a4 Compare February 11, 2022 19:00

tssurya approved these changes Feb 14, 2022

View reviewed changes

trozet merged commit 4aaa7b5 into ovn-org:master Feb 14, 2022

trozet mentioned this pull request Feb 15, 2022

Fixes race for namespace logging level update #2823

Merged

msherif1234 mentioned this pull request Mar 23, 2022

Adds retry mechanism for Node resource #2870

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds retry mechanism for Network Policy #2809

Adds retry mechanism for Network Policy #2809

trozet commented Feb 8, 2022

trozet commented Feb 8, 2022

trozet commented Feb 8, 2022

trozet commented Feb 8, 2022

trozet commented Feb 8, 2022

coveralls commented Feb 8, 2022 •

edited

trozet commented Feb 9, 2022

trozet commented Feb 9, 2022

tssurya Feb 10, 2022

trozet Feb 11, 2022

tssurya Feb 11, 2022

trozet Feb 11, 2022

dcbw Feb 11, 2022

tssurya Feb 10, 2022

trozet Feb 11, 2022

tssurya Feb 10, 2022

trozet Feb 11, 2022

tssurya Feb 11, 2022

trozet Feb 11, 2022

dcbw commented Feb 11, 2022 •

edited

tssurya left a comment

tssurya Feb 14, 2022

tssurya commented Feb 14, 2022

	// successfully cleaned up old policy, remove it from the retry cache
	// successfully created new policy, remove it from retry cache.

Adds retry mechanism for Network Policy #2809

Adds retry mechanism for Network Policy #2809

Conversation

trozet commented Feb 8, 2022

trozet commented Feb 8, 2022

trozet commented Feb 8, 2022

trozet commented Feb 8, 2022

trozet commented Feb 8, 2022

coveralls commented Feb 8, 2022 • edited

trozet commented Feb 9, 2022

trozet commented Feb 9, 2022

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

Choose a reason for hiding this comment

dcbw commented Feb 11, 2022 • edited

tssurya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tssurya commented Feb 14, 2022

coveralls commented Feb 8, 2022 •

edited

dcbw commented Feb 11, 2022 •

edited