CLOUDSTACK-9339 Virtual Routers don't handle Multiple Public Interfaces correctly #1659
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| "-A PREROUTING -s %s/32 -m state --state NEW -j MARK --set-xmark 0x%s/0xffffffff" % \ | ||
| (rule["internal_ip"], device[len("eth"):])]) | ||
| self.fw.append(["mangle", "", | ||
| "-A PREROUTING -s %s/32 -m state --state NEW -j CONNMARK --save-mark --nfmask 0xffffffff --ctmask 0xffffffff" % \ |
Contributor
There was a problem hiding this comment.
Is there value in extracting the 0xffffffff to a constant?
Contributor
|
@murali-reddy - thank you for picking this up. Let me know when you think it's appropriate to close PR #1519 and I shall do so. |
Contributor
|
@dsclose what is your opinion of the changes in this PR relative to your work? Do you see any gaps or missing pieces? |
Contributor
|
@murali-reddy Looking through the Marvin tests, there is a lot of boilerplate code and duplicated setup in test cases. It feels like there is an opportunity to introduce one or more base classes that would not only reduce the duplication/boilerplate, but make building future VR test cases easier. Do you agree? |
Contributor
|
@murali-reddy what is the status of addressing the review feedback. Also, have addressed the VPC-related issues as well? |
Contributor
Author
|
@blueorangutan package |
|
@murali-reddy a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-20 |
Contributor
Author
|
@blueorangutan test |
|
@murali-reddy a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests |
|
Trillian test result (#21)
|
Contributor
|
@murali-reddy 8 errors occurred in the VMware test run. Could you please investigate these failures? |
Contributor
Author
|
@jburwell i am investigating them. Just so you know, blueorangutan is running full smoke test suite. Similar failures are seen in other PR as well. So some of the tests may have been failing for some time and need to be fixed. From the above list of failures test_router_dhcphosts are due to test errors that is being addressed by #1683 I have addressed below two test failures. support for internal LB was not added in the logic creating ips.json databag, I made a fix for that as well. test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80 rest of the failures i am investigating if they are regressions, and test failures. |
Contributor
|
@murali-reddy FYI |
Contributor
Author
|
@ustcweizhou thanks, i have fixed it earlier today and tested, am trying to fix other test failures and re-run the CI |
Contributor
|
|
||
|
|
||
| firewall_rule.delete(self.apiclient) | ||
|
|
Contributor
There was a problem hiding this comment.
I think creating and validating a firewall rule can be moved to net utility class, as this has possibilities to be reused.
Contributor
|
@blueorangutan test centos7 xenserver-65sp1 |
|
@jburwell a Trillian-Jenkins test job (centos7 mgmt + xenserver-65sp1) has been kicked to run smoke tests |
|
Trillian test result (tid-533)
|
|
Trillian test result (tid-532)
|
Member
|
Test lgtm, @murali-reddy are we good on this PR? I'm seeing some failures though not sure if they related to your changes. /cc @jburwell @abhinandanprateek |
Member
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Member
|
@murali-reddy @abhinandanprateek let me know any help needed from my end? |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-323 |
Member
|
@blueorangutan test matrix |
|
@rhtyd a Trillian-Jenkins matrix job (centos6 mgmt + xs65sp1, centos7 mgmt + vmware55u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests |
|
Trillian test result (tid-581)
|
|
Trillian test result (tid-591)
|
Contributor
|
LGTM on code review and testing @murali-reddy @rhtyd |
Contributor
Author
|
Can this merged now? test failures are specific to redundent VR/VPC VR which have been failing in 4.8/4.9/master. |
Member
|
@blueorangutan help |
|
@rhtyd I understand these words: "help", "hello", "thanks", "package", "test" Blessed contributors for kicking Trillian test jobs: ['rhtyd', 'jburwell', 'murali-reddy', 'abhinandanprateek', 'PaulAngus', 'borisstoyanov', 'karuturi'] |
Contributor
|
Yes I think test failures does not seem to be caused by this PR, LGTM based on code review. |
borisstoyanov
approved these changes
Dec 6, 2016
Member
|
Thanks @murali-reddy @borisstoyanov I'll proceed with merging this now. @murali-reddy do you want to run the component test on this PR or on 1753/1754? |
Contributor
Author
|
@rhtyd please proceed with merge. I have not added the test suite(test_multiple_public_interfaces.py) part of this PR as smoke tests, because it takes around an hour for execution so they are best fit for component testing. Also tests need routable additional public IP range for the test case to succeed. I will run component test against 1753/1754 and report back results. |
|
Trillian test result (tid-597)
|
Contributor
Author
|
component test results. If any one wish to run component test test_multiple_public_interfaces.py, add a new public ip range in the test_daya.py, in the 'publiciprange' dict object. Test iptable rules in case we have IP associated with a network which is in ... === TestName: test_iptable_rules | Status : SUCCESS === Ran 6 tests in 1539.183s OK |
…es correctly -when processing static nat rule, add a mangle table rule, to mark the traffic from the guest vm when it has associated static nat rule so that traffic gets routed using the route tabe of the device which has public ip associated -fix the case where nic_device_id is empty when ip is getting disassociated resulting in empty deviceid in ips.json -add utility methods in CsRule, and CsRoute to add 'ip rule' and 'ip route' rules respectivley -ensure traffic from all public interfaces are connection marked with device number, and restored for the reverse traffic. use the connection marked number to do device specific routing table lookup fill the device specific routing table with default route -component tests for testing multiple public interfaces of VR
Member
|
Travis failure is to do with a test_volumes failures in one of the component tests, not related to this PR. Will merge, and handle the Travis failure separately. |
asfgit
pushed a commit
that referenced
this pull request
Dec 7, 2016
…non_vpc CLOUDSTACK-9339 Virtual Routers don't handle Multiple Public Interfaces correctlyAs pointed out in CLOUDSTACK-9339, in case of multiple public IP's from different public IP ranges are associated with VR, VR functionality is broken from 4.6. Below are the brief list of problems specific to non-VPC networks addressed in the PR. This PR handles both VPC and non-VPC scenarios. - reverse traffic for the connections accepted on the eth3 and above public interfaces are getting blocked. Need a rule for e.g "-A FORWARD -i eth3 -o eth0 -m state --state RELATED,ESTABLISHED -j ACCEPT" in the FORWARD chain of filter table to permit reverse path traffic for established connections. - outbound public traffic from eth0 to eth3 (or for interfaces above like eth4 eth5 etc) needs rule to run through FW_OUTBOUND chain in the filter table - network stats on public interfaces eth3 are getting gathered - default gateway is missing in the device specific routing table, resulting in traffic to be looked up in main routing table - creating a device specific route table is generating "from all lookup Table_eth3" in the ip rules, resulting in rest of the traffic getting blocked. Picked few commits from #1519 from dsclose (#1519) submitted for 4.7 Marvin tests are added to test below - Static NAT works on the public interfaces above eth2, in case non-vpc networks - Portforwarding works on the public interfaces above eth2, in case non-vpc networks - Route tables are configured as expected for the device specific table for the public interfaces above eth2, in case non-vpc networks - IP tables rules are as expected for the traffic from and to the public interfaces above eth2, in case non-vpc networks * pr/1659: CLOUDSTACK-9339 Virtual Routers don't handle Multiple Public Interfaces correctly Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Contributor
|
@murali-reddy @rhtyd Possible regression introduce by the PR1659 see https://issues.apache.org/jira/browse/CLOUDSTACK-9770 |
Member
|
@murali-reddy @abhinandanprateek @DaanHoogland can you comment on this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As pointed out in CLOUDSTACK-9339, in case of multiple public IP's from different public IP ranges are associated with VR, VR functionality is broken from 4.6. Below are the brief list of problems specific to non-VPC networks addressed in the PR. This PR handles both VPC and non-VPC scenarios.
ip rules, resulting in rest of the traffic getting blocked.
Picked few commits from #1519 from dsclose (#1519) submitted for 4.7
Marvin tests are added to test below