You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by harikrishna-patnala <gi...@git.apache.org> on 2015/11/30 07:55:37 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9094: Multiple threads are bei...
GitHub user harikrishna-patnala opened a pull request:
https://github.com/apache/cloudstack/pull/1140
CLOUDSTACK-9094: Multiple threads are being used to collect the stats…
CLOUDSTACK-9094: Multiple threads are being used to collect the stats from the same VR
Same thread is being intialised by two managers, VirtualNetworkApplianceManager and VpcVirtualNetworkApplianceManager
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/harikrishna-patnala/cloudstack CLOUDSTACK-9094
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1140.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 #1140
----
commit c9cc2ccd6dd69bc267ded6e654ec7167a330b6df
Author: Harikrishna Patnala <ha...@citrix.com>
Date: 2015-11-30T06:44:15Z
CLOUDSTACK-9094: Multiple threads are being used to collect the stats from the same VR
Same thread is being intialised by two managers, VirtualNetworkApplianceManager and VpcVirtualNetworkApplianceManager
----
---
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-9094: Multiple threads are bei...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-163536428
@bhaisaab
I thought only RMs would merge PR... isn't that so?
Plus, the PR was merged without one single person executing any test against it.
@remibergsma, could you please revert it so someone can actually test the change?
Cheers,
Wilder
---
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-9094: Multiple threads are bei...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1140#discussion_r46256253
--- Diff: server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java ---
@@ -685,6 +685,16 @@ public void finalizeStop(final VirtualMachineProfile profile, final Answer answe
}
@Override
+ public boolean start() {
+ return true;
--- End diff --
@harikrishna-patnala Hi Hari, can you explain what the changes are doing to fix the issue of multiple thread synchronisation ? Also, what happens across multiple mgmt servers since we don't have any distributed consensus implementation in place, I suppose the statcollector values might be different.
---
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-9094: Multiple threads are bei...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1140
---
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-9094: Multiple threads are bei...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-162434058
@koushik-das thanks, in that case LGTM.
Will merge now.
---
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-9094: Multiple threads are bei...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-163140058
@DaanHoogland the fix is simple. We do not need special /extra process for VPC routers.
@bhaisaab this issue does not exist in 4.5, as @koushik-das mentioned, it is a regression bug in 4.6.
LGTM
this is almost same to commit e444a03, so we can merge it or cherry pick the old commit
---
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-9094: Multiple threads are bei...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-163542373
@harikrishna-patnala This is reverted because of the lack of test report, not the logic of your code. If you feel it must go in anyway please rebase the code and make a new PR, make sure you add test instruction to up the chance reviewers actually 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-9094: Multiple threads are bei...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-162418800
LGTM based on code changes.
@bhaisaab Since VpcVirtualNetworkApplianceManager is derived from VirtualNetworkApplianceManager, start() call on both managers ended up calling VirtualNetworkApplianceManager.start(). This resulted in the same threads/executors to get initialised/started twice.
---
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-9094: Multiple threads are bei...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-163537454
@wilderrodrigues it was merged before the freeze so I didn't revert it as RM. But you are right about the not being tested.
---
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-9094: Multiple threads are bei...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-162472783
Thanks for picking this up, guys.
I still think there is calling code that should not be allowed to call start twice. It should skip the VirtualNetworkApplianceManager.start() in case we are dealing with a VPC.
More important this has been allowed to become regression because it was not put into tests.
/me is looking for an :i told you so: emoticon.
---
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-9094: Multiple threads are bei...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-162469279
@koushik-das good find, that means we also need to fix this for 4.6. We would still need a round of tests just in case.
@harikrishna-patnala can you open a PR against 4.6 branch (and 4.5 branch if needed). 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9094: Multiple threads are bei...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-163140236
sorry guys, just notice it is already 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.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9094: Multiple threads are bei...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-163283297
@wilderrodrigues can you have a look? you rmoved the code that is reintroduced with this PR.
---
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-9094: Multiple threads are bei...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-162464615
Guys, this got merged on only code review. No tests are described showing the old and new behaviours. No unit tests are added. No regression tests are added. It is like this fix is not important.
My intuition says that if the VpcVirtualNetworkApplianceManager.start() is called then VirtualNetworkApplianceManager.start() should not have been and the real bug is not in this code at all.
---
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-9094: Multiple threads are bei...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-162468306
Actually I was looking at the history and this is what I found. This was originally fixed with commit
commit e444a03819ccf72f61cb04e8428d20cc65b145e1
Author: Wei Zhou <w....@leaseweb.com>
Date: Thu Nov 14 16:51:28 2013 +0100
remove duplicated scheduled tasks from VpcVirtualNetworkApplianceManagerImpl
Then was removed with commit
commit a5d6f90f666798e7575c6b7d3af5b3024af8768e
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: Mon Jan 26 17:33:50 2015 +0100
Implementing redundant router arguments to add redundant_state
Implementing the arguments on the python side
And then fixed again with this PR. Not sure why it got removed at the first place.
---
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-9094: Multiple threads are bei...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1140#issuecomment-162465934
@DaanHoogland let's revert, and ask @harikrishna-patnala to add more tests?
---
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.
---