Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[release-4.7] Bug 2105274: EGW: Clean Stale Conntrack Entries #1194

Merged
merged 5 commits into from Jul 19, 2022
Merged

[release-4.7] Bug 2105274: EGW: Clean Stale Conntrack Entries #1194

merged 5 commits into from Jul 19, 2022

Conversation

tssurya
Copy link
Contributor

@tssurya tssurya commented Jul 14, 2022

NOTE: Full on conflicts, It's almost like a new PR
/assign @trozet
/assign @dcbw

@tssurya
Vendor github.com/j-keck/arping library
7f0e7ea 
Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
(cherry picked from commit f12f04d)

 Conflicts in 4.12:
	go-controller/go.sum
	go-controller/vendor/modules.txt

 because ovn-org/ovn-kubernetes#3001 is missing

(cherry picked from commit b21c935)
(cherry picked from commit 5437e0e)

 Conflicts in 4.10:
	go-controller/go.mod
	go-controller/vendor/modules.txt
 due to difference in module positions and explicit way of writing
 "explicit" in modules.txt file

(cherry picked from commit 37cebe6)

 Conflicts in 4.9:
	go-controller/go.sum
	go-controller/vendor/modules.txt
 due to missing k8sbump PRs between versions

(cherry picked from commit f74b8d1)
(cherry picked from commit 3dd5534)

 Conflicts in 4.7:
	go-controller/go.sum
@tssurya
CARRY VENDOR: TEMP: netlink: Conntrack Label Filter
2e97265 
This commit can be reverted when
vishvananda/netlink#784
merges and we can re-vendor the next netlink release

Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
(cherry picked from commit ecd92a8)
(cherry picked from commit 153bfe7)
(cherry picked from commit 79544d5)
(cherry picked from commit 5599844)

 Conflicts:
	go-controller/vendor/github.com/vishvananda/netlink/conntrack_linux.go
 because netlink vendoring hasn't been done (was done in
ovn-org/ovn-kubernetes#2042 which isn't in 4.9)

(cherry picked from commit d3cba2c)
(cherry picked from commit 0eb102b)
@tssurya
Add utils for namespace annotations
91bc711 
Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
(cherry picked from commit 739395d)

 Conflicts:
	go-controller/pkg/ovn/egressfirewall_test.go

because ovn-org/ovn-kubernetes#2927 is missing

(cherry picked from commit 49ae83e)
(cherry picked from commit cd37c50)

 Conflicts in 4.10:
	go-controller/pkg/ovn/namespace.go
 because
 openshift@e80f270#diff-3696280f7dbfece2e1136d5fa06a3339a05a8a3067710f7510ecdefba51bcb29
 is missing in 4.10

(cherry picked from commit 2b0d654)

 Conflicts in 4.9:
	go-controller/pkg/ovn/egressgw.go because ovn-org/ovn-kubernetes@3514740#diff-aae5aec1d5a36916dcc466832ab980fa0db49e1fb794a14b1416cdcf9df7b5c3L380 is missing in 4.9
	go-controller/pkg/ovn/namespace.go because openshift@f68d302#diff-3696280f7dbfece2e1136d5fa06a3339a05a8a3067710f7510ecdefba51bcb29L172 is missing in 4.9
	go-controller/pkg/ovn/policy_test.go because openshift@0019f39#diff-2a5ea421d13f2f639ad217cada1f5c741e9ba6d68abcf431a700c0f68b71e04d
is missing in 4.9

(cherry picked from commit a311363)

 Conflicts in 4.8:
	minor conflict in go-controller/pkg/ovn/namespace.go around isNamespaceMulticastEnabled

(cherry picked from commit 6229b88)

 Conflicts in 4.7:
	go-controller/pkg/ovn/egressgw.go
	go-controller/pkg/ovn/namespace.go - had to change some logic in UpdateExternalGatewayPodIPsAnnotation
	go-controller/pkg/ovn/policy_test.go
@openshift-ci openshift-ci bot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jul 14, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2022

@tssurya: This pull request references Bugzilla bug 2105274, which is invalid:

  • expected dependent Bugzilla bug 2105272 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

[release-4.7] Bug 2105274: EGW: Clean Stale Conntrack Entries

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.

@tssurya
Cleanup UDP conntrack entries when EPs are deleted
5136837 
This PR:
2) cleanups conntrack entries only for UDP during endpoint
deletion.
See kubernetes/kubernetes#108523 (comment)
for details.

Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
(cherry picked from commit 8f46895)
(cherry picked from commit 23114aa)

 Conflicts in 4.9:
	go-controller/pkg/node/gateway_shared_intf.go
	go-controller/pkg/util/kube.go
   because ETP code is missing in 4.9
(ovn-org/ovn-kubernetes#2136)
NOTE: I had to add nodeIPManager to nodePortWatcher

(cherry picked from commit 6ff7b10)

 Conflicts in 4.8:
	go-controller/pkg/node/gateway_shared_intf.go
  because
https://github.com/openshift/ovn-kubernetes/commit/68a22408ccb3ef5232eb2715428a85fa7a1138d7#diff-d3aa58d9b58a0a09264f0[…]c4656411ae2dc1ac68fb3c4R619
  is missing

(cherry picked from commit c5e2ae6)

 Conflicts in 4.7:
	go-controller/pkg/node/gateway_shared_intf.go
	go-controller/pkg/node/node.go
 did not bring in the actual service entries deletion logic,
 simply brought the `DeleteConntrack` skeleton.
@tssurya
EGW: Delete stale conntrack entries
d3e30f5 
This commit adds logic to delete the conntrack entries
that contain src MAC address in the "labels" field when
using ECMP routes on the GR.

Logic:

1) annotate the namespace each time an exgw is added/deleted with list of ips
2) add new informer for namespace on node side checking only if gw ip annotation OR external-gws annotation changed
3) ovnkube node on namespace change, iterates through all the ips and initiates an arp request via ovnk and collects the MACs
4) once all the responses come back, we have all the known macs
5) we search for ct entries for any pod ip belonging to the namespace, if ct_label is loaded with a mac not in our list we flush it
6) we run the above in a goroutine as well set which will run every 5mins looping through all relevant namespaces.

Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
(cherry picked from commit 5a3a8b8)
(cherry picked from commit c3f54d0)
(cherry picked from commit f78c8b6)

 Conflicts in 4.10:
	go-controller/pkg/factory/factory.go & go-controller/pkg/factory/types.go
        because
openshift@b4738c7#diff-a3beed34d05206e3dd33290edad67e97d8a649c65624502860736c2532d88399R82
is missing

	go-controller/pkg/node/node.go
        because
openshift@e80f270#diff-9c3b7372332fd16d60dba539a94dd0f819d262d1fbabd1e5d9c09e47da5af0ff
is missing

	go-controller/pkg/ovn/egressgw.go
        because
openshift@4e3c26a#diff-aae5aec1d5a36916dcc466832ab980fa0db49e1fb794a14b1416cdcf9df7b5c3
is missing

in 4.10

(cherry picked from commit 6ffe806)

 Conflicts in 4.9:
	go-controller/pkg/cni/helper_linux.go
	go-controller/pkg/node/gateway_shared_intf.go
	go-controller/pkg/node/node.go
	go-controller/pkg/ovn/egressgw.go
	go-controller/pkg/ovn/egressgw_test.go
	go-controller/pkg/util/net_linux.go
	go-controller/pkg/util/net_linux_unit_test.go
	test/e2e/util.go
	test/scripts/e2e-cp.sh

(cherry picked from commit 7bfd4ef)

 Conflicts in 4.8:
	go-controller/pkg/factory/factory.go
	go-controller/pkg/node/node.go
	go-controller/pkg/ovn/egressgw.go
	go-controller/pkg/ovn/namespace.go
	go-controller/pkg/ovn/namespace_test.go
	test/e2e/external_gateways.go
	test/scripts/e2e-cp.sh

(cherry picked from commit 20de360)

 Conflicts in 4.7:
	go-controller/pkg/factory/types.go
	go-controller/pkg/node/node.go
	go-controller/pkg/ovn/egressgw.go
	go-controller/pkg/ovn/namespace.go
	go-controller/pkg/ovn/namespace_test.go
ALL testing has been thrown out the window since 4.7 didn't have
external_gateways file (it was too much pain to shift all that to
e2e_test.go specially when we don't even run these tests on 4.7)
	test/e2e/e2e.go
	test/e2e/external_gateways.go
	test/e2e/util.go
	test/scripts/e2e-cp.sh
@trozet
Copy link
Contributor

trozet commented Jul 15, 2022

/lgtm
/label backport-risk-assessed

@openshift-ci openshift-ci bot added backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. lgtm Indicates that a PR is ready to be merged. labels Jul 15, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 15, 2022

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2022
@openshift-bot
Copy link
Contributor

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 16, 2022

@openshift-bot: This pull request references Bugzilla bug 2105274, which is invalid:

  • expected dependent Bugzilla bug 2105272 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

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.

@trozet
Copy link
Contributor

trozet commented Jul 16, 2022

/retest-required

@openshift-bot
Copy link
Contributor

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 17, 2022

@openshift-bot: This pull request references Bugzilla bug 2105274, which is invalid:

  • expected dependent Bugzilla bug 2105272 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

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.

@trozet
Copy link
Contributor

trozet commented Jul 17, 2022

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 17, 2022

@tssurya: all tests passed!

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.

@openshift-bot
Copy link
Contributor

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 18, 2022

@openshift-bot: This pull request references Bugzilla bug 2105274, which is invalid:

  • expected dependent Bugzilla bug 2105272 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

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.

@tssurya
Copy link
Contributor Author

tssurya commented Jul 18, 2022

/bugzilla refresh

@openshift-ci openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jul 18, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 18, 2022

@tssurya: This pull request references Bugzilla bug 2105274, which is valid.

6 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.z) matches configured target release for branch (4.7.z)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
  • dependent bug Bugzilla bug 2105272 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
  • dependent Bugzilla bug 2105272 targets the "4.8.z" release, which is one of the valid target releases: 4.8.0, 4.8.z
  • bug has dependents

Requesting review from QA contact:
/cc @anuragthehatter

In response to this:

/bugzilla refresh

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.

@anuragthehatter
Copy link

/label qe-approved
/label cherry-pick-approved

@openshift-ci openshift-ci bot added qe-approved Signifies that QE has signed off on this PR cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Jul 19, 2022
@openshift-ci openshift-ci bot merged commit b1abcda into openshift:release-4.7 Jul 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 19, 2022

@tssurya: All pull requests linked via external trackers have merged:

Bugzilla bug 2105274 has been moved to the MODIFIED state.

In response to this:

[release-4.7] Bug 2105274: EGW: Clean Stale Conntrack Entries

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanluisvaladas juanluisvaladas Awaiting requested review from juanluisvaladas

@trozet trozet Awaiting requested review from trozet

@anuragthehatter anuragthehatter Awaiting requested review from anuragthehatter

Assignees

@trozet trozet

@dcbw dcbw

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tssurya @trozet @openshift-bot @anuragthehatter @dcbw