Fix public IPs not being removed from the VR when deprovisioned#1907
Fix public IPs not being removed from the VR when deprovisioned#1907asfgit merged 1 commit intoapache:masterapache/cloudstack:masterfrom
Conversation
|
LGTM We run this code in production without issues for a few months now (MissionCriticalCloud/cosmic#114) |
| 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): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
ok, got it; thanks.
After reading the code and explanations, I believe the changes are ok.
LGTM
There was a problem hiding this comment.
@swill @rafaelweingartner
#1871 has already been merged into 4.9 and master, so I think guest_gw is not necessary here.
There was a problem hiding this comment.
@ustcweizhou I am not sure that change 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...
|
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... |
|
@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 I am guessing this the PR changed because of the changes introduced with #1871, right? |
|
@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. This was removed from the PR. The rest of the code is the same as the original pull request. |
|
Ok, great. |
|
This has the required reviews and both @remibergsma (sbp) and us (cloud.ca) have been using this fix in production. |
|
tag:mergeready |
|
LGTM @karuturi this is ready for merge |
|
code LGTM |
Fix public IPs not being removed from the VR when deprovisionedThis 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. * pr/1907: Fix public IPs not being removed from the VR when deprovisioned Signed-off-by: Rajani Karuturi <rajani.karuturi@accelerite.com>
Correção para que o `StatsCollector` não realize divisão por zero Closes apache#1907 See merge request scclouds/scclouds!1028
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.