-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fixed issues with test_vpc_vpn script #1156
Conversation
Test Remote Access VPN in VPC ... === TestName: test_vpc_remote_access_vpn | Status : SUCCESS === Ran 1 test in 169.692s OK FAIL: Test VPN in VPCTraceback (most recent call last): The second test failure is not related to these changes. It is something to do with the connectivity issues between the vms. |
@sanju1010 Thanks for the PR. Can you make it PEP8 compliant again please? Most tests seem to create the offering the test needs. Isn't that a better idea? I will run your version of the test. The previous one always succeeds for me, but I agree with the hypervisor specific part. That should be generic. Will run it and report back. |
Just tested this on 4.6.1-SNAPSHOT with the command:
and it failed:
I'm pretty sure this has to do with: https://github.com/sanju1010/cloudstack/blob/09409a7ac264e864c45146927b86229c8e67fbc8/test/integration/smoke/test_vpc_vpn.py/#L711 field 7 was changed to field 6 of the ping output |
ping @sanju1010 |
Doesn't work for me either:
The original one works:
|
Hi Remi, As I mentioned in my PR the second test failure is related to connectivity Thanks,
|
@sanju1010 the error lies in the script due to different outputs. the test seems to use macchinina as template, which has the following output for ping: ping -c 1 -t 1 x.y.z.40PING x.y.z.40 (x.y.z.40): 56 data bytes --- x.y.z.40 ping statistics --- The cut on line 711 picks field 6, which is 'received' and not 0. a regular debian derivate has the following output: where your cut would match '+1' and work. conclusion; either change the template or change the cut |
👎 Tests are failing, we need to fix it before we consider a merge, sorry. |
So removed my last comment about field 7 to 6. We need to choose 1 type of template os for both kvm and xen and base the test on that. @sanju1010: the machininna was already in there and working for kvm at least. would you mind making that template work? |
Sanjeev N on dev@cloudstack.apache.org replies: |
@sanju1010 it works for XenServer as well atleast, haven't tried VMware. But you'll have to change your script to adjust to the different output |
Sanjeev N on dev@cloudstack.apache.org replies: |
Gotcha. We could add templates for the other hypervisors (vmware and hyperv) from http://dl.openvm.eu/cloudstack/macchinina/x86_64/ And remove the default_hypervisor variable and replace it for a dynamic template selection based on the nosetests hypervisor parameter. |
Sanjeev N on dev@cloudstack.apache.org replies: |
Are you still working on this? It's currently holding us a bit because we need to do some work on this test as well. For instance, I want to make it rVPC compliant. The point is: I'm trying to avoid conflicts when merging your changes with my changes. If that would be fine, I would like to suggest I take it over. I already applied some fixes to the test_internal_lb.py, which was suffering from the same problem. See PR #1204 Is that okay if I take it over? Cheers, |
Sanjeev N on dev@cloudstack.apache.org replies: |
Sanjeev N on sanjeev@apache.org replies: |
Hi @sanju1010, @wilderrodrigues asked me to extend the test_vpc_vpn script and in the proces i added the other hypervisors. |
Is this PR still relevant or did the changes in the other mentioned PRs make this PR outdated? If it is still active, please rebase the PR to get rid of merge conflicts so we can more easily test and merge this code. Thx... |
There are few issues with this test file. Please refer to JIRA Ticket https://issues.apache.org/jira/browse/CLOUDSTACK-9102 for details on the issues that have been fixed.