MCO-1248: port {azure,gcp}-routes.sh from iptables to nftables #4494
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-merge-robot
added
the
needs-rebase
danwinship
changed the title
openshift-merge-robot
removed
the
needs-rebase
|
/test all |
|
@danwinship: The
The following commands are available to trigger optional jobs:
Use
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-sigs/prow repository. |
|
/test all |
danwinship
changed the title
|
/payload-aggregate periodic-ci-openshift-release-master-ci-4.17-e2e-gcp-ovn-upgrade 5 |
|
@danwinship: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/fa4ddbd0-4c3c-11ef-8c5d-c73ffa4621dc-0 |
|
/payload-aggregate periodic-ci-openshift-release-master-ci-4.17-e2e-gcp-ovn-upgrade 6 |
|
@danwinship: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/91e1b5e0-4c5e-11ef-9aa0-de06f78ca57b-0 |
Change them to say "OVN-Kubernetes is not running" rather than "Plugin is SDN", since the plugin is never SDN any more, and the script gets run before ovn-k starts up anyway, so previously it was logging "Plugin is SDN" for a while at startup. Also remove the log message from remove_stale_routes, since that function is always run along with add_routes, so we don't need to log them same thing in both of them.
gcp-routes had a rule "so that existing flows (with an entry in conntrack) continue to be balanced, even if the DNAT entry is removed" (which was then also copied into azure-routes and alibaba-routes). The only way this iptables rule would actually be needed is if (a) your masters have an iptables-based firewall (which they shouldn't, on OCP), and (b) the firewall is so aggressive that it even drops packets from established connections (which no firewall should do anyway). At any rate, even if the rule *was* necessary in some clusters, it won't work in future nftables-only versions of RHCOS anyway, because nftables doesn't let "accept" rules in one table override "drop"/"reject" rules in another table; if your firewall is broken and dropping packets that it shouldn't, you have to actually fix your firewall rules, not hack around them somewhere else.
danwinship
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@danwinship: This pull request references MCO-1248 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/payload-aggregate periodic-ci-openshift-release-master-ci-4.17-e2e-azure-ovn-upgrade 6 |
1 similar comment
|
/payload-aggregate periodic-ci-openshift-release-master-ci-4.17-e2e-azure-ovn-upgrade 6 |
|
@danwinship: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7163e640-4e74-11ef-85b3-ebdf6b3ccdbe-0 |
|
ok, gcp payload passed in the first run, azure payload passed in the second run (after failing all 6 times in the original run because of a bug, showing that the payload test does actually depend on this script working) |
|
/lgtm |
|
/assign @djoshy |
|
/approve I'll admit this is not my domain so I'll defer to @jcaamano. The payloads look good too. Holding in case this requires QE testing, but feel free to unhold if not required. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, djoshy, jcaamano 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 |
openshift-ci
bot
added
the
approved
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel The failure mode for this is that a very small percentage of apiserver connections would fail during upgrades, which we have checks for in the upgrade jobs, but which is hard to notice in human testing (since the failed connections would get retried so everything still works, just a little bit more slowly than it should have). So I think the payload jobs validate this better than QE could. |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
|
/hold Revision 6989891 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/hold Revision 6989891 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-merge-bot
bot
merged commit f1e1137
into
openshift:master
iptables is going away in RHEL10; this PR ports {azure,gcp}-routes.sh from iptables to nftables.
What's different:
inetfamily so we can put both sets of rules together (and we don't need to check if IPv6 is enabled or not; the IPv6 rule will just never match anything if IPv6 is disabled.)Other than that (and the commit that removes an unnecessary ACCEPT rule), there are no changes in functionality; I had previously started an attempt to refactor them first (#3619, #3673, #3674, #3675) but wasn't having much luck with that, and we need to get rid of iptables, so...