You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wido <gi...@git.apache.org> on 2016/01/05 18:07:56 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
GitHub user wido opened a pull request:
https://github.com/apache/cloudstack/pull/1309
CLOUDSTACK-9210: Pass secondary IPs to default_network_rules() function
This is a mandatory argument but it was NOT passed which caused the
re-programming of security groups to fail.
Simple fix to just add the argument since the variable is available
there.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wido/cloudstack CLOUDSTACK-9210
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1309.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1309
----
commit 239148c31bd2cb0dce19aaef63391073564d3530
Author: Wido den Hollander <wi...@widodh.nl>
Date: 2016-01-05T17:06:34Z
CLOUDSTACK-9210: Pass secondary IPs to default_network_rules() function
This is a mandatory argument but it was NOT passed which caused the
re-programming of security groups to fail.
Simple fix to just add the argument since the variable is available
there.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169320044
@remibergsma Can we merge this one? It fixes a bug.
To clarify, this is how the method has been defined:
<pre>def default_network_rules(vm_name, vm_id, vm_ip, vm_mac, vif, brname, sec_ips):</pre>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169063660
@DaanHoogland @remibergsma @resmo Could you take a look?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169067941
@DaanHoogland We do not have any unit testing in place for Python. If you look just 10 lines up you see the method being called exactly that way.
Without this line the script simply errors out with a Python stacktrace.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-172261610
I agree we need to fix some issues like this , which is difficult to be reproduced but it is obviously an issue from code view.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169095290
btw LGTM if you don't want to improve further.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169632599
@wido I have no way to test this myself. If you want this in a point release, please point the PR to 4.7 branch and reference the new PR to this one (so we can count the reviews). It will then end up in both 4.7.1 and 4.8.0.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169066689
simple fix but a test on the contents in the called method makes sense as well. Not sure how to test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169095187
@wido My point exactly. 10 lines up a conditional should guard the split() call
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169069686
Forgot to add, pylint still works fine. It scores 3.54/10. Not great, but a lint check still succeeds.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169134133
@DaanHoogland No idea. Might work with Xen if we manage those Instances through libvirt. But in case of KVM we can probably do this with libvirt. Might need to patches in libvirt though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169133424
@wido will your proposal from CLOUDSTACK-1164 work for other hypervisors as well?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169682247
@remibergsma I'm find with 4.8 since that should be here this month.
I really don't know how to test this. My python foo-skills tell me this patch is right. The function invocation was wrong and is now done right.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169117271
@DaanHoogland I agree, this script isn't the best in the world.
In my opinion we should offload this work to libvirt:
- http://libvirt.org/formatnwfilter.html
- https://issues.apache.org/jira/browse/CLOUDSTACK-1164
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-169267155
LGTM, just code review. simple but really make sense.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1309#issuecomment-172259457
@remibergsma We have this one running in production right now and it is working as expected.
I spent some time in trying to get a test-case up and running, but I can't. I can just tell you that by looking at the code this was a stupid thing which wasn't caught during the addition of secondary IPs.
Our logs were full with Python stack traces and they are now gone :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9210: Pass secondary IPs to de...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1309
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---