Bug 2091157: [release-4.10] Free IPs and delete resources for completed pods #1152
Conversation
This pull request is to delete the logicial ports associated with a pod when a pods runs to completion. The intent of this change is to reduce the size of OVN databases by removing entries that are no longer needed. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com> Co-authored-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit 999f344)
Get on port cache was returning the actual object in the cache instead of a copy. Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit b382400)
With the update to deleteLogicalPort with pod completed status, there are several other places in the code that could have stale applications of the old pod IP. This commit modifies those to also remove the pod IP from their usage during pod update. Additionally, other places in the code rely on the logical port cache to get the portInfo. However, this may be removed after deleteLogicalPort. Therefore remove getting the logical port where IPs can be retrieved via the kapi pod object itself. Main paths affected: - Network Policy: updating port groups and addr sets - Namespace exgw: get pod ips from pod object, not port cache - Egress IP: update pod handling for completed pod - Hybrid Overlay: update pod handling for completed pod Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit b94351d)
The nested allocator calls were propagating an error type up during an IP release. However in the bitmap allocator function it was never possible to error during an IP release. Remove the return type. Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit 1a430bc)
Changes-Include: - During pod sync we allocate IPs of all existing pods, but we should ignore completed pods - During namespace add we add all of the pod IPs to the ns address set, but we should ignore completed pods - During processing of delete event for a completed pod, we were trying to delete the pod again, which would try to free the IP that was previously released and could be in use by another pod. We should ignore delete events for completed resources as they would have been handled during update. - On node add, we add all existing pods on that node back to retry as an "add". We should skip completed pods here. - We now check during deletion of a completed pod (should happen on update only) to make sure no other running pods are using this IP as a failsafe to ensure we never release an IP in use by another pod or the related OVN config Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit 7bcc8da)
Upon fetching all of the NATs on a router, if the NATs or the router dont exist this should not be an error for deletion. Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit 8ad0d79)
openshift-ci
bot
added
bugzilla/severity-medium
|
@trozet: This pull request references Bugzilla bug 2091157, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 6 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
the
approved
|
/hold Investigating how "allows allocation after pods are completed" passes in 4.11/upstream. In this case the IP is provided in the pod status when myPod2 is created: This is the same IP as the already created pod. So when first pod is deleted, the IP is not freed because we detect it is in use by myPod2. However, creating myPod2 fails because it does not have any annotation yet with this IP. This seems like a bug in the test case, not sure how it passes upstream. |
openshift-ci
bot
added
the
do-not-merge/hold
|
Figured out there was a regression only in 4.11: ovn-org/ovn-kubernetes#3045 |
The pods were being incorrectly created with the expected IP already present in pod status field...before it was ever even allocated by OVNK. This would cause the tests to function incorrectly as OVNK checks this field to know whether or not it is safe to deallocate IPs (in case any other pod has a duplicate IP). Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit 2a17c21) (cherry picked from commit 1b4e65f)
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/assign @tssurya |
|
/retest-required |
| @@ -1240,8 +1246,18 @@ func (oc *Controller) WatchNodes() { | |||
| if err != nil { | |||
| klog.Errorf("Unable to list existing pods on node: %s, existing pods on this node may not function") | |||
| } else { | |||
| oc.addRetryPods(pods.Items) | |||
| oc.requestRetryPods() | |||
| filteredPods := make([]kapi.Pod, 0) | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
| @@ -424,7 +425,7 @@ var _ = ginkgo.Describe("OVN Pod Operations", func() { | |||
| ) | |||
|
|
|||
| myPod2, err := fakeOvn.fakeClient.KubeClient.CoreV1().Pods(t.namespace).Create(context.TODO(), | |||
| newPod(t2.namespace, t2.podName, t2.nodeName, t2.podIP), metav1.CreateOptions{}) | |||
| newPod(t2.namespace, t2.podName, t2.nodeName, ""), metav1.CreateOptions{}) | |||
There was a problem hiding this comment.
| ) | ||
|
|
||
| myPod2, err := fakeOvn.fakeClient.KubeClient.CoreV1().Pods(t.namespace).Create(context.TODO(), | ||
| newPod(t2.namespace, t2.podName, t2.nodeName, t2.podIP), metav1.CreateOptions{}) |
There was a problem hiding this comment.
nit: we need to remove this t2.podIP no?
https://github.com/ovn-org/ovn-kubernetes/pull/3045/files#diff-ce4c9ab66a07bb4649f94a9b633d3f0d71468ceef64f0dcd8be92f9e1c8d82b0R536 ?
| @@ -527,7 +527,7 @@ var _ = ginkgo.Describe("OVN Pod Operations", func() { | |||
| ) | |||
|
|
|||
| myPod2, err := fakeOvn.fakeClient.KubeClient.CoreV1().Pods(t.namespace).Create(context.TODO(), | |||
| newPod(t2.namespace, t2.podName, t2.nodeName, t2.podIP), metav1.CreateOptions{}) | |||
| newPod(t2.namespace, t2.podName, t2.nodeName, ""), metav1.CreateOptions{}) | |||
There was a problem hiding this comment.
oh you fixed it in this commit! all good.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: trozet, tssurya The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@trozet: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/label backport-risk-assessed |
openshift-ci
bot
added
the
backport-risk-assessed
|
/assign @anuragthehatter |
|
/label cherry-pick-approved |
openshift-ci
bot
added
the
cherry-pick-approved
|
@trozet: All pull requests linked via external trackers have merged: Bugzilla bug 2091157 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Not a totally clean backport due to generic retry watcher library added in 4.11.