Uh oh!
There was an error while loading. Please reload this page.
Fixes for VirtualRouters in Basic Networking, especially with mutliple ranges in VLANs - #1547
Conversation
| def get_dns(self): | ||
| conf = self.cmdline().idata() | ||
| dns = [] | ||
| <<<<<<< e09480728f4ed77d0bd0bdc6a6c0966d90f7ee26 |
There was a problem hiding this comment.
I think you want to squash these out with the last commit in your PR. We wouldn't want this in our history you dutch commercial fan ;)
There was a problem hiding this comment.
Yeah spotted it just too late... Thought I fixxed it but my git-merge-fu isn't fully up to spec yet ;)
DaanHoogland
commented
May 13, 2016
@The-Loeki 6 commits with the same message is not helpful and indeed confusing. Can you change them to shortly describe the individual fixes, or if you must squash them? |
The-Loeki
commented
May 13, 2016
@DaanHoogland click on the (...) 👯 |
DaanHoogland
commented
May 13, 2016
@The-Loeki those don't show in a short listing |
The-Loeki
commented
May 13, 2016
@DaanHoogland like it better now? |
DaanHoogland
commented
May 13, 2016
👍 maybe squash the conflict entries out? |
5d2cda8 to
4b5d316CompareThe-Loeki
commented
May 13, 2016
Like this then 🎯 |
DaanHoogland
commented
May 13, 2016
I like the first two. that way all the info is there for who takes the trouble to click the (...) and for those that are to lazy there is a clear subtract in less then 72 chars. |
DaanHoogland
commented
May 15, 2016
started the integration tests on this |
DaanHoogland
commented
May 15, 2016
code looks clean. I don't understand all if it yet so I may ask questions yet. but anaother set of eyes required; |
DaanHoogland
commented
May 15, 2016
@swill can you schedule this one. Would be nice to get in before freeze. Mine test effort failed (see dev@) |
| conf = self.cmdline().idata() | ||
| dns = [] | ||
| if not self.cl.get_use_ext_dns(): | ||
| if not self.is_vpc() and self.cl.is_redundant(): |
There was a problem hiding this comment.
these should not be changed.
There was a problem hiding this comment.
@DaanHoogland As I can see, the CsGuestNetwork means the vpc tiers.
For isolated networks/shared networks, the dns is set in CsConfig.get_dns().
So, I think the change in CsConfig.get_dns() is wrong.
There was a problem hiding this comment.
@ustcweizhou / @DaanHoogland: This should revert the behaviour
ustcweizhou
commented
May 16, 2016
guys, please donot merge the changes on VR before a LGTM with functional testing. |
DaanHoogland
commented
May 16, 2016
@ustcweizhou the integration tests are functional tests. If there is extra functionality to be tested please provide the test for it. And at least describe what is missing in the tests, here |
@DaanHoogland is correct; we're running this code in test-production. Keep in mind however that we only use Basic Networking Zones, so I did my best to keep the behaviour in other circumstances (barring obvious bugs) the same, but I can't test those either. Regarding @ustcweizhou 's comment; the only change I perceive there is that, analogous to what's in the CsGuestNetwork, the code seems to imply that the As I encountered a merge conflict while building this PR, I think this is a relatively new conditional which seems to mirror the one (incorrectly) in CsGuestNetwork (one depends on the If this both is true, I propose to change the On the tests side, the (re)added use_extdns and ipaliases functionality both seem to lack functional testing. |
DaanHoogland
commented
May 17, 2016
@The-Loeki maybe it makes sense to split the PR, especially on the part where there is discussion. |
The-Loeki
commented
May 17, 2016
Maybe, but for now it's still just one conditional which needs just one decision/explanation. |
swill
commented
May 17, 2016
I am currently testing this in both a basic and advanced zone. I will post results when I have them... |
CI RESULTSNOTE: THIS IS THE FIRST TIME I HAVE EVER RUN CI AGAINST A BASIC NETWORKING CONFIG, SO I DON'T KNOW WHAT ERRORS OF THESE ARE GOING TO HAPPEN ON MASTER. Summary of the problem(s): Associated Uploads
Uploads will be available until Comment created by |
swill
commented
May 17, 2016
I think we can ignore a lot of this. My tests are mainly setup to test advanced networking. I have to revisit this to know how to test the basic networking stuff correctly. I have tests running against this PR for advanced networking to see if anything is broken on the advanced side because of this. |
DaanHoogland
commented
May 17, 2016
@swill I think @ustcweizhou has a point. So at least that one commit tag:needslove. The rest should pass our regular suite. We can also create a Marvin definition for a basic zone and run tests on those. @The-Loeki@wido do you have a suitable definition to match your environment ready? |
swill
commented
May 18, 2016
CI RESULTSSummary of the problem(s): Associated Uploads
Uploads will be available until Comment created by |
I have seen these errors on two other PRs tonight, so I don't think they are related to this PR. I think this is a pretty clean CI result. I need to track down where those errors come from and see if they are on master right now as well. BTW, these results are for an Advanced zone. So this just verifies that this did not introduce new bugs for that use case. We still need proper tests for a Basic zone setup. |
wido
commented
May 18, 2016
@DaanHoogland I haven't touched this code much. @The-Loeki ran into this issue when working on our Private Cloud (which is where we test upgrades first). We don't have a modified Marvin suite. The main problem was when you have multiple IPv4 ranges in a POD only the first/primary one would be configured on the VR. This PR is to make sure that the VR configures all ranges sent to it. |
linas-ipxo
commented
May 18, 2016
Disclaimer: i'm very new to the whole ACS, but i have built and deployed custom releases for ourselves. I understand the project structure somewhat. So.. I'm using advanced networking with security groups. After these patches, the VR doesn't load - fails when the scripts get executed with KeyErorr 'gateway'. This is in the ipaliases part. I'm not sure what the ip aliases are for there, but it would seem strange that it an ip alias would require gateway. |
DaanHoogland
commented
Jun 7, 2016
Yes @wido ,they look good to me but I am not running a cloud at the moment so hence I asked @ustcweizhou to have a look. |
wido
commented
Jun 7, 2016
Ok, thanks @DaanHoogland , sounds good! Looking forward to more input or LGTMs :) |
remibergsma
commented
Jun 8, 2016
I see some tests failing on VPCs: Will rebuild from scratch to see if it fails again. |
wido
commented
Jun 8, 2016
@remi: Thanks for the feedback! @DaanHoogland reported a failure as well, but his second test succeeded without any problems. I don't have experience with VPC, but as far as I can tell this PR does not touch any VPC code. In Daan's original results the 'test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL' test went fine.
|
remibergsma
commented
Jun 8, 2016
@wido FYI all Python code is shared between basic/advanced and router/vpc so it's important to test both. Will try to look into it again in the next days. Not saying it's broken, just want to be careful. |
swill
commented
Jun 8, 2016
CI RESULTSAssociated Uploads
Uploads will be available until Comment created by |
swill
commented
Jun 8, 2016
The above is a test against basic networking. |
remibergsma
commented
Jun 8, 2016
Can you also run one against advanced please @swill? |
swill
commented
Jun 8, 2016
The tests run against basic networking hardly test the functionality of the network though. Apparently most of the tests for the network are only for advanced networking. I will run against advanced networking again now... |
wido
commented
Jun 8, 2016
Thanks for the feedback @remibergsma and @swill ! Hopefully the results come back positive. |
swill
commented
Jun 8, 2016
Should this set of test have the "basic" tag? It only has the "sg" tag, which I am not targeting.
|
swill
commented
Jun 9, 2016
CI RESULTSAssociated Uploads
Uploads will be available until Comment created by |
Above is a run against advanced networking. Nice clean run. 👍 I am trying to test a bunch of basic zone networking tests, but I had never previously verified if they were broken. I have run them against this PR, but there are a bunch of issues. I am going to run the same set of tests against master to see if there is a difference in the results. Unverified tests run against this PR. Doing the same against master now... (details masked intentionally as this set of tests is unverified) |
swill
commented
Jun 9, 2016
I ran the same network tests I mentioned above against master and I had the following results. All of the errors were the same on I am not sure if this PR fixed this issue or if it is just an intermittent issue, but this shows that this PR did not introduce any new issues into master (according to these unverified tests). Given that this fixes issues in basic networking and does not appear to introduce any new issues, I am inclined to add it to the 4.9 release. @wido, @remibergsma and @DaanHoogland do you guys agree with this decision? Thanks... |
swill
commented
Jun 9, 2016
BTW, these tests results are for the following tests With |
wido
commented
Jun 10, 2016
@swill I agree, but that's also because this bug is hitting and hurting us. However, I need other people to say LGTM :) |
swill
commented
Jun 10, 2016
@wido judging from your testing and commentary, I believe this has your LGTM? You have not officially specified that it does. |
wido
commented
Jun 12, 2016
@swill: Yes, LGTM from me. But @The-Loeki and I tested this together, so I would prefer more LGTMs, but also want this bug fixed :) |
DaanHoogland
commented
Jun 12, 2016
@swill Pinging @ustcweizhou again, I know leaseweb has an interest in this. They use advanced zones in several different setups so their ok is a good indication we can merge (together with pcxtreme their ok) |
swill
commented
Jun 13, 2016
@ustcweizhou can you review this PR please. I think we are ready on this one and since it fixes issues on master I would like to get it in before I cut the RC. Thanks... |
swill
commented
Jun 14, 2016
@ustcweizhou I can't wait forever on this. What I will likely do if I don't hear back from you is merge this and then cut the RC right away. I am hoping to cut the RC tomorrow, so if you can give me feedback before then I would appreciate it. |
NuxRo
commented
Jun 14, 2016
+1 merging, I'm also interested in this if it affects Advanced zone + Security Groups. |
wido
commented
Jun 19, 2016
Great, if the RC is cut we can test that better. I'm about to leave on a 2 week holiday however. |
bvbharatk
commented
Jun 20, 2016
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no):https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests: Skipped tests: Passed test suits: |
swill
commented
Jun 20, 2016
There have been a couple issues that have been reported on |
Fixes for VirtualRouters in Basic Networking, especially with mutliple ranges in VLANsDuring the last few modifications on the SystemVM scripts, it turns out quite a lot of stuff broke in our setups. This PR fixes a number of things: * Multiple IP's per VLAN interface are now supported & working again, including DNS, DHCP ranges, password and metadata services * `useextdns` fixed (I had a small merge conflict with an attempt to fix this at 4.7, but these fixes are more comprehensive) * CLOUDSTACK-8303 * Apache configs better in line with best-practices and distro-expected locations * Added a few more helper functions & getters & setters for utility * some minor cleanup & fixes * pr/1547: Remove /etc/apache2/sites-enabled/000-default in cloud-early-config SysVM various fixes to previous refactorings * make CORS include a regular glob-matched one * fix NameVirtualHost in CsApp.py as well * even moar cleanups SysVM: Cleanup and removal of old (and dangerous) config files * ports.conf * default & default-ssl sites * SSL config in httpd.conf * deprecated & dead setup_redundant_router in cloud-early-config SysVM cloud-early-config: Intermediate fix for SecStore & CORS * Take setup from vhost.template rather than default(-ssl) * should move into Python CS code as well * Move CORS setup to separate conf * Modify vhost template to Optionally include the cors file * Add NameVirtualHost to vhost template for feature parity with ports.conf * Take setup from vhost.template rather than default(-ssl) VR cloud-early-config: Commonize Apache2 common setup VR cloud-early-config: Fix Apache2 alias cleanup VR: consistent SSL setup, vhost is not an example, but a template VR CsConfig: reintroduce old get_dns() behaviour for redundant non-VPC's VR CsAddress fixes: * cleanup imports, * fix to_str(), * improve & fix service post_config logic * don't arpPing when there's no gateway VR CsApp: Expose config to classes, move vhost confs to proper location, allow for multiple IP's per intf, sanitize servername, don't open port 53 if no DNS is foreseen VR CsConfig: Add is_router(), is_dns(), has_dns(), has_metadata(), use_extdns(), fix get_dns() with use_extdns() VR CsDhcp: allow multiple ranges & finite lease time (fixes CLOUDSTACK-8303) VR CsGuestNetwork obey useextdns VR merge.py ipalias fix & dhcpconfig stub notification Signed-off-by: Will Stevens <williamstevens@gmail.com>
During the last few modifications on the SystemVM scripts, it turns out quite a lot of stuff broke in our setups.
This PR fixes a number of things:
useextdnsfixed (I had a small merge conflict with an attempt to fix this at 4.7, but these fixes are more comprehensive)