[GitHub] cloudstack pull request #1594: CLOUDSTACK-9407: vm_network_map table doesnt ...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1594


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1594: CLOUDSTACK-9407: vm_network_map table doesnt ...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1594#discussion_r67907402
  
--- Diff: 
engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java ---
@@ -1577,8 +1577,9 @@ private void advanceStop(final VMInstanceVO vm, final 
boolean cleanUpEvenIfUnabl
 }
 
 try {
-_networkMgr.release(profile, cleanUpEvenIfUnableToStop);
-s_logger.debug("Successfully released network resources for 
the vm " + vm);
+s_logger.debug("Not releasing network resources until expunge 
command is sent");
--- End diff --

I decided to remove try-block as it only logged info, and include it on 
method's javadoc, as I don't think it could be worth logging. Do you agree?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1594: CLOUDSTACK-9407: vm_network_map table doesnt ...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1594#discussion_r67907224
  
--- Diff: 
engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java ---
@@ -1577,8 +1577,9 @@ private void advanceStop(final VMInstanceVO vm, final 
boolean cleanUpEvenIfUnabl
 }
 
 try {
-_networkMgr.release(profile, cleanUpEvenIfUnableToStop);
-s_logger.debug("Successfully released network resources for 
the vm " + vm);
+s_logger.debug("Not releasing network resources until expunge 
command is sent");
+//_networkMgr.release(profile, cleanUpEvenIfUnableToStop);
+//s_logger.debug("Successfully released network resources for 
the vm " + vm);
--- End diff --

Done, thanks!


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1594: CLOUDSTACK-9407: vm_network_map table doesnt ...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1594#discussion_r67907159
  
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -2086,6 +2089,18 @@ public boolean expunge(UserVmVO vm, long 
callerUserId, Account caller) {
 }
 }
 
+/**
+ * Release network resources, it was done on vm stop previously.
+ * @param id vm id
+ * @throws ConcurrentOperationException
+ * @throws ResourceUnavailableException
+ */
+private void releaseNetworkResourcesOnExpunge(long id) throws 
ConcurrentOperationException, ResourceUnavailableException {
+final VMInstanceVO vmInstance = _vmDao.findById(id);
--- End diff --

Done, thanks for pointing 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1594: CLOUDSTACK-9407: vm_network_map table doesnt ...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1594#discussion_r67572107
  
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -2086,6 +2089,18 @@ public boolean expunge(UserVmVO vm, long 
callerUserId, Account caller) {
 }
 }
 
+/**
+ * Release network resources, it was done on vm stop previously.
+ * @param id vm id
+ * @throws ConcurrentOperationException
+ * @throws ResourceUnavailableException
+ */
+private void releaseNetworkResourcesOnExpunge(long id) throws 
ConcurrentOperationException, ResourceUnavailableException {
+final VMInstanceVO vmInstance = _vmDao.findById(id);
--- End diff --

It is possible for ``_vmDao.findById`` to return ``null``.  Please handling 
for that case.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1594: CLOUDSTACK-9407: vm_network_map table doesnt ...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1594#discussion_r67571914
  
--- Diff: 
engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java ---
@@ -1577,8 +1577,9 @@ private void advanceStop(final VMInstanceVO vm, final 
boolean cleanUpEvenIfUnabl
 }
 
 try {
-_networkMgr.release(profile, cleanUpEvenIfUnableToStop);
-s_logger.debug("Successfully released network resources for 
the vm " + vm);
+s_logger.debug("Not releasing network resources until expunge 
command is sent");
--- End diff --

Please include context information, such as descriptions and IDs of the 
network resources, in the log message.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1594: CLOUDSTACK-9407: vm_network_map table doesnt ...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1594#discussion_r67571985
  
--- Diff: 
engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java ---
@@ -1577,8 +1577,9 @@ private void advanceStop(final VMInstanceVO vm, final 
boolean cleanUpEvenIfUnabl
 }
 
 try {
-_networkMgr.release(profile, cleanUpEvenIfUnableToStop);
-s_logger.debug("Successfully released network resources for 
the vm " + vm);
+s_logger.debug("Not releasing network resources until expunge 
command is sent");
+//_networkMgr.release(profile, cleanUpEvenIfUnableToStop);
+//s_logger.debug("Successfully released network resources for 
the vm " + vm);
--- End diff --

Please remove commented code.  It is cruft, and git history allows us to 
see how code has changed overtime.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1594: CLOUDSTACK-9407: vm_network_map table doesnt ...

[nvazquez]

GitHub user nvazquez opened a pull request:

https://github.com/apache/cloudstack/pull/1594

CLOUDSTACK-9407: vm_network_map table doesnt get cleaned up properly

JIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-9407

### Introduction
It was found out that in production environments `vm_network_map` table 
entries were slowly growing. It was investigated how this entries were cleaned 
up.

### Behaviour
On vm creation, vm mappings are inserted on `vm_network_map`.
On vm stop, mappings are deleted from `vm_network_map` for vm, as a result 
of the release of its nics.

### Problem
If created vm is stopped from hypervisor side (at least on vSphere in which 
we tested it), when CloudStack realizes vm is stopped it doesn't clean up 
`vm_network_table,` and, as cleanup is made during vm stop, when vm is 
eventually destroyed and expunged it won't clean up their entries in that table.

### Proposed solution
We propose to move `vm_network_map` table cleanup to expunge command 
instead of stop command.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/nvazquez/cloudstack vmnetworkmapissue

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1594.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 #1594


commit d3f3fb0590f14c540493d5f27c18cea13fa092af
Author: nvazquez 
Date:   2016-06-06T14:47:45Z

CLOUDSTACK-9407: Release network resources on expunge command




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---