NP-641: Avoid firewalld flush of iptables#1300
Conversation
openshift-ci
bot
added
the
approved
There was a problem hiding this comment.
We need to rephrase this explanation. I would not include a reference to the issue, which is currently private. Also, we should only explain what we're trying to solve without future statements.
There was a problem hiding this comment.
We have talked about this in the past, but IMHO, as a project maintainer, it always helped me to figure out why certain things were done in projects, it is true that you can always look at the code/commit and find the original reference. But for example it gets harder when the file has moved around and the git reference becomes harder to track.
There was a problem hiding this comment.
This file will end up on user systems, so we need to review the text in the comments, like documentation. Let's discuss it further and reach a conclusion backed up by our content experts.
packaging/rpm/microshift.spec
Outdated
There was a problem hiding this comment.
I would not include NP-641 issue reference in the comment.
There was a problem hiding this comment.
It was standard practice in rpm packaging at RedHat to include references to bugs, in fact at some point in history I remember you could not push a packaging change without the reference to a bz# has this changed?
There was a problem hiding this comment.
The current history in the file does not include any issue references. If we decide to reference any issues, we should probably trace the history back and add it all along. For now, I would stick out.
|
/retitle NP-641: Avoid firewalld flush of iptables |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mangelajo: This pull request references NP-641 which is a valid jira issue. DetailsIn 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. |
|
/hold I am doing more testing this morning to verify |
openshift-ci
bot
added
the
do-not-merge/hold
|
/unhold It seems to work well on my tests. |
openshift-ci
bot
removed
the
do-not-merge/hold
Disables firewalld access to iptables related nftables translator, this avoids firewalld flush of iptables under any circumstance, firewalld will work at nftables level, while MicroShift/OVN can continue to work at iptables level (translated to nft) under the hood.
|
/retest |
1 similar comment
|
/retest |
|
@mangelajo: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/approve |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ggiguash, mangelajo, zshi-redhat The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherrypick release-4.12 |
|
@mangelajo: #1300 failed to apply on top of branch "release-4.12": DetailsIn 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. |
`firewall --reload` won't flush iptable rules after the fix: openshift#1300 Signed-off-by: Zenghui Shi <zshi@redhat.com>
7 participants
Disables firewalld access to iptables related nftables translator,
this avoids firewalld flush of iptables under any circumstance,
firewalld will work at nftables level, while MicroShift/OVN can
continue to work at iptables level (translated to nft) under the hood.