You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by swill <gi...@git.apache.org> on 2017/01/16 20:53:20 UTC
[GitHub] cloudstack pull request #1907: Fix public IPs not being removed from the VR ...
GitHub user swill opened a pull request:
https://github.com/apache/cloudstack/pull/1907
Fix public IPs not being removed from the VR when deprovisioned
This PR replaces #1706. It does not remove the IP from the database, but it does deprovision the IP correctly from the VR when the public IP is removed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/swill/cloudstack fix_vr_ip
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1907.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 #1907
----
commit 45e867903350b3468a47aa9aebc4bc403d246675
Author: Will Stevens <wi...@gmail.com>
Date: 2017-01-16T20:51:44Z
Fix public IPs not being removed from the VR when deprovisioned
----
---
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 issue #1907: Fix public IPs not being removed from the VR when de...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the issue:
https://github.com/apache/cloudstack/pull/1907
LGTM We run this code in production without issues for a few months now (https://github.com/MissionCriticalCloud/cosmic/pull/114)
---
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 #1907: Fix public IPs not being removed from the VR ...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1907#discussion_r97082665
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsAddress.py ---
@@ -659,7 +663,10 @@ def is_guest_gateway(self, bag, ip):
gw = interface.get_gateway()
logging.info("Interface has the following gateway ==> %s", gw)
- if bag['nw_type'] == "guest" and rip == gw:
+ guest_gw = self.config.cmdline().get_guest_gw()
+ logging.info("Interface has the following gateway ==> %s", guest_gw)
+
+ if bag['nw_type'] == "guest" and (rip == gw or rip == guest_gw):
--- End diff --
this works for isolated networks, but not for vpc tiers.
---
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 issue #1907: Fix public IPs not being removed from the VR when de...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the issue:
https://github.com/apache/cloudstack/pull/1907
code LGTM
---
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 issue #1907: Fix public IPs not being removed from the VR when de...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the issue:
https://github.com/apache/cloudstack/pull/1907
This has the required reviews and both @remibergsma (sbp) and us (cloud.ca) have been using this fix in production.
---
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 #1907: Fix public IPs not being removed from the VR ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1907#discussion_r100904660
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsAddress.py ---
@@ -659,7 +663,10 @@ def is_guest_gateway(self, bag, ip):
gw = interface.get_gateway()
logging.info("Interface has the following gateway ==> %s", gw)
- if bag['nw_type'] == "guest" and rip == gw:
+ guest_gw = self.config.cmdline().get_guest_gw()
+ logging.info("Interface has the following gateway ==> %s", guest_gw)
+
+ if bag['nw_type'] == "guest" and (rip == gw or rip == guest_gw):
--- End diff --
ok, got it.
---
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 #1907: Fix public IPs not being removed from the VR ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1907#discussion_r100897492
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsAddress.py ---
@@ -659,7 +663,10 @@ def is_guest_gateway(self, bag, ip):
gw = interface.get_gateway()
logging.info("Interface has the following gateway ==> %s", gw)
- if bag['nw_type'] == "guest" and rip == gw:
+ guest_gw = self.config.cmdline().get_guest_gw()
+ logging.info("Interface has the following gateway ==> %s", guest_gw)
+
+ if bag['nw_type'] == "guest" and (rip == gw or rip == guest_gw):
--- End diff --
This is more like a curiosity than anything else.
What do you think about using only `return bag['nw_type'] == "guest" and (rip == gw or rip == guest_gw)` ? It does not feel that we need the conditional here.
---
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 issue #1907: Fix public IPs not being removed from the VR when de...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1907
@swill LGTM for the changes.
The changes introduced now are different from the last ones, right? Now it is basically the addition of a log and the skipping of a processing flow if condition `not address['add'] and not ip.configured()` is met.
I am guessing this the PR changed because of the changes introduced with #1871, 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 #1907: Fix public IPs not being removed from the VR ...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1907#discussion_r100969281
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsAddress.py ---
@@ -659,7 +663,10 @@ def is_guest_gateway(self, bag, ip):
gw = interface.get_gateway()
logging.info("Interface has the following gateway ==> %s", gw)
- if bag['nw_type'] == "guest" and rip == gw:
+ guest_gw = self.config.cmdline().get_guest_gw()
+ logging.info("Interface has the following gateway ==> %s", guest_gw)
+
+ if bag['nw_type'] == "guest" and (rip == gw or rip == guest_gw):
--- End diff --
@swill @rafaelweingartner
#1871 has already been merged into 4.9 and master, so I think guest_gw is not necessary here.
---
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 #1907: Fix public IPs not being removed from the VR ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1907
---
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 issue #1907: Fix public IPs not being removed from the VR when de...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1907
Ok, great.
Thanks for the explanation.
---
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 issue #1907: Fix public IPs not being removed from the VR when de...
Posted by kiwiflyer <gi...@git.apache.org>.
Github user kiwiflyer commented on the issue:
https://github.com/apache/cloudstack/pull/1907
tag:mergeready
---
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 issue #1907: Fix public IPs not being removed from the VR when de...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the issue:
https://github.com/apache/cloudstack/pull/1907
@rafaelweingartner the only change was to remove the extra gateway verification if it was a 'guest' network because this is now handled by the code in #1871.
```
+ guest_gw = self.config.cmdline().get_guest_gw()
+ logging.info("Interface has the following gateway ==> %s", guest_gw)
+
+ if bag['nw_type'] == "guest" and (rip == gw or rip == guest_gw):
```
The rest of the code is the same as the original pull request.
---
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 issue #1907: Fix public IPs not being removed from the VR when de...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1907
LGTM @karuturi this is ready for merge
---
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 issue #1907: Fix public IPs not being removed from the VR when de...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the issue:
https://github.com/apache/cloudstack/pull/1907
I have squash merged the changes that @ustcweizhou requested. Would you mind reviewing again so we can get this important fix into 4.10. Thanks...
@rafaelweingartner, it looks like I lost your code review in the process of doing the squashed commit. Can you review and confirm your previous review?
Thanks guys...
---
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 #1907: Fix public IPs not being removed from the VR ...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1907#discussion_r100902927
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsAddress.py ---
@@ -659,7 +663,10 @@ def is_guest_gateway(self, bag, ip):
gw = interface.get_gateway()
logging.info("Interface has the following gateway ==> %s", gw)
- if bag['nw_type'] == "guest" and rip == gw:
+ guest_gw = self.config.cmdline().get_guest_gw()
+ logging.info("Interface has the following gateway ==> %s", guest_gw)
+
+ if bag['nw_type'] == "guest" and (rip == gw or rip == guest_gw):
--- End diff --
I find that makes the code a bit harder to read if we did that. Then it is not as obvious you are returning a boolean. This stuff is already complicated, for two lines of code, I don't think it makes sense to make it harder to understand. :)
---
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 #1907: Fix public IPs not being removed from the VR ...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1907#discussion_r101062037
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsAddress.py ---
@@ -659,7 +663,10 @@ def is_guest_gateway(self, bag, ip):
gw = interface.get_gateway()
logging.info("Interface has the following gateway ==> %s", gw)
- if bag['nw_type'] == "guest" and rip == gw:
+ guest_gw = self.config.cmdline().get_guest_gw()
+ logging.info("Interface has the following gateway ==> %s", guest_gw)
+
+ if bag['nw_type'] == "guest" and (rip == gw or rip == guest_gw):
--- End diff --
@ustcweizhou I am not sure that changes removes the need for this, but it looks like it could potentially. Maybe I can run some tests to see if it can be removed given #1871 has been merged...
---
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.
---