CLOUDSTACK-9564: Fix memory leaks in VmwareContextPool #1729
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
rohityadavcloud
force-pushed
the
vmware-memleak-fix
branch
from
dfabcc8 to
5490617
Compare
|
@blueorangutan help |
|
@rhtyd I understand these words: "help", "hello", "thanks", "package", "test" |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-94 |
|
@blueorangutan test centos6 vmware-55u3 |
|
@rhtyd a Trillian-Jenkins test job (centos6 mgmt + vmware-55u3) has been kicked to run smoke tests |
rohityadavcloud
force-pushed
the
vmware-memleak-fix
branch
from
5490617 to
ed964aa
Compare
|
Trillian test result (tid-181)
|
|
Trillian test result (tid-182)
|
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-95 |
|
@blueorangutan test centos7 vmware-55u3 |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests |
rohityadavcloud
force-pushed
the
vmware-memleak-fix
branch
from
ed964aa to
3584a99
Compare
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-97 |
rohityadavcloud
force-pushed
the
vmware-memleak-fix
branch
from
3584a99 to
39a2f0b
Compare
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-98 |
|
Trillian test result (tid-189)
|
|
@blueorangutan test centos6 vmware-55u3 |
|
@rhtyd a Trillian-Jenkins test job (centos6 mgmt + vmware-55u3) has been kicked to run smoke tests |
|
Trillian test result (tid-188)
|
|
Trillian test result (tid-194)
|
|
Pinging for review -- @sateesh-chodapuneedi @koushik-das @sureshanaparti @karuturi @murali-reddy and other vmware contributors/maintainers |
|
Trillian test result (tid-205)
|
|
Trillian test result (tid-206)
|
|
LGTM from code change. |
rohityadavcloud
force-pushed
the
vmware-memleak-fix
branch
from
39a2f0b to
25f58f1
Compare
|
Thanks @murali-reddy |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
| _pool.put(context.getPoolKey(), l); | ||
| Queue<VmwareContext> ctxList = _pool.get(context.getPoolKey()); | ||
| if (ctxList == null) { | ||
| ctxList = EvictingQueue.create(_maxIdleQueueLength); |
There was a problem hiding this comment.
Should we get a synchronised evictingqueue ?
There was a problem hiding this comment.
Yes. There may be a case, where two threads may be trying to create evicting queue for a poolKey.
There was a problem hiding this comment.
Since all access is in synchronized blocks this is not required. But a synchronised evictingqueue could have let you avoid some of synchronised blocks.
Again, this is not required as it is.
| oldestContext.close(); | ||
| } catch (Throwable t) { | ||
| s_logger.error("Unexpected exception caught while trying to purge oldest VmwareContext", t); | ||
| } |
There was a problem hiding this comment.
Although we are using a bounded queue now and that will prevent memory leaks, do we know why it should be throwing old elements -or- why the contexts are not cleaned up programmatically instead of forcefully restricting the size ?
There was a problem hiding this comment.
In the registerContext method we check if the queue is full; in case it is full, we remove the oldest element and close it properly. I'll ping you on the lines where it happens.
rohityadavcloud
force-pushed
the
vmware-memleak-fix
branch
from
25f58f1 to
e6dab39
Compare
rohityadavcloud
commented
Nov 1, 2016
| if (oldestContext != null) { | ||
| try { | ||
| oldestContext.close(); | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
@abhinandanprateek ^^ here we close oldest context, before adding a new one. The queue provided the FIFO semantics to purge old contexts and keep new ones around, which is why I switched from previously used arraylist.
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-122 |
|
@blueorangutan test centos7 vmware-55u3 |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests |
|
@rhtyd LGTM on code review. |
rohityadavcloud
force-pushed
the
vmware-memleak-fix
branch
from
e6dab39 to
9dfedca
Compare
In a recent management server crash, it was found that the largest contributor to memory leak was in VmwareContextPool where a registry is held (arraylist) that grows indefinitely. The list itself is not used anywhere or consumed. There exists a hashmap (pool) that returns a list of contexts for existing poolkey (address/username) that is used instead. This fixes the issue by removing the arraylist registry, and limiting the length of the context list for a given poolkey. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
rohityadavcloud
force-pushed
the
vmware-memleak-fix
branch
from
9dfedca to
90a3d97
Compare
|
Thanks @murali-reddy @abhinandanprateek I'll proceed with merging this. We can explore considering using apache-commons pool in future. |
asfgit
pushed a commit
that referenced
this pull request
Dec 5, 2016
CLOUDSTACK-9564: Fix memory leaks in VmwareContextPoolIn a recent management server crash, it was found that the largest contributor to memory leak was in VmwareContextPool where a registry is held (arraylist) that grows indefinitely. The list itself is not used anywhere or consumed. There exists a hashmap (pool) that returns a list of contexts for existing poolkey (address/username) that is used instead. This fixes the issue by removing the arraylist registry, and limiting the length of the context list for a given poolkey. @blueorangutan package * pr/1729: CLOUDSTACK-9564: Fix memory leaks in VmwareContextPool Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✖centos6 ✔centos7 ✔debian. JID-326 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In a recent management server crash, it was found that the largest contributor
to memory leak was in VmwareContextPool where a registry is held (arraylist)
that grows indefinitely. The list itself is not used anywhere or consumed. There
exists a hashmap (pool) that returns a list of contexts for existing poolkey
(address/username) that is used instead.
This fixes the issue by removing the arraylist registry, and limiting the
length of the context list for a given poolkey.
@blueorangutan package