-
Notifications
You must be signed in to change notification settings - Fork 105
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.
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
Add drop cache when change nodes state #1947
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bkopilov The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @bkopilov. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test Would it make sense to drop the cache directly in
|
[bkopilov] Yes , the cache is a reall risk if something goes wrong during tests or events. |
src/tests/base_test.py
Outdated
@@ -644,7 +644,8 @@ def set_iptables_rules_for_nodes( | |||
for node in given_nodes: | |||
given_node_ips.append(node.ips[0]) | |||
cluster.nodes.shutdown_given(given_nodes) | |||
|
|||
# Clear cached info about nodes after shutdown | |||
cluster.nodes.drop_cache() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to do the shutdown in the first place? Any chance you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The iptables fixture from base_test.py run shutdown, no idea if we really need shutdown.
We have failures in CI in tests that use this fixture and i suspect that when the libvirt comes back we dont have the latest ip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So let's try to remove the shutdown if it serves no purpose
There's no reason to just do more actions and increase our chances of failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you recommend to fix all other cases when we stop/ reboot nodes during test lifetime ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can drop the cache for those other cases (which are outside of this code scope)
Can you send me some links? I'll take a look at those tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, lets not remove the shutdown, it was implemented long time ago, there was a reason - if i am not mistaken if for the rules to apply, proxy tests are highly depended on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check this a bit
Either way Adrien's suggestion to embed the cache-drop in the shutdown function is a better way to construct this solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack , yes Adrien's change will cover our needs but please note that we still may have an issue if user will try to call to Node().shutdown() . but it good enough ,if no objection i am going to drop_cahce for: shutdown and reboot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good compromise
Iptables check does not work when having a chain of sources When we insert a rule with multiple source, iptables split the list and adding each source with same conditions On check action iptables wont catch case when having a mix of source when 1 is not in the table. looks like iptables check only one. In case check action returned wrong result add/delete rule chnaged and may trigger failures due to unexpected results Also deletion , iptables delete only the first match when having duplicate rules. Example: iptables --insert FORWARD -p tcp -j DROP -s 192.168.128.10,192.168.128.13 results with: -A FORWARD -s 192.168.128.13/32 -p tcp -m tcp --dport 80 -j DROP -A FORWARD -s 192.168.128.10/32 -p tcp -m tcp --dport 80 -j DROP We will get different result from iptables for: iptables --check FORWARD -p tcp -j DROP -s 7.7.7.7,192.168.128.10 iptables --check FORWARD -p tcp -j DROP -s 192.168.128.10,7.7.7.7 Means that only 1 is the best approch here. Run delete rules till all duplicate rules gone
40036ce
to
562ebc0
Compare
@bkopilov: The following test 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. |
Code has changed into another change |
When the machines started again the cached info must be refreshed. In case machines state changed like stop/reboot cashed info may hide bugs in the node controllers or fail tests. This patch is continuation of openshift#1947 Due to some mismach i created new patch.
@bkopilov can we close this PR? |
Yes please . |
/close |
@osherdp: Closed this PR. 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. |
Current iptables fixture tears down nodes in order modify iptables. When the machines started again the cached info must be refreshed.
In case machines state changed like stop/reboot cashed info may hide bugs in the node controllers or fail tests.