You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by anshul1886 <gi...@git.apache.org> on 2015/12/23 07:22:44 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9198: Virtual router gets depl...
GitHub user anshul1886 opened a pull request:
https://github.com/apache/cloudstack/pull/1278
CLOUDSTACK-9198: Virtual router gets deployed in disabled Pod
While starting the router, send the user from the callingContext instead of defaulting to System user.
https://issues.apache.org/jira/browse/CLOUDSTACK-9198
To test:
----------
Verify that Virtual Router is not getting deployed in disabled Pod.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 CLOUDSTACK-9198
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1278.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 #1278
----
commit 228c67d9ed1f0329c53b162997f015b85e1404f7
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-11-02T08:28:44Z
CLOUDSTACK-9198: Virtual router gets deployed in disabled Pod. While starting the
router, send the user from the callingContext instead of defaulting to System user
----
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by cloudmonger <gi...@git.apache.org>.
Github user cloudmonger commented on the issue:
https://github.com/apache/cloudstack/pull/1278
### ACS CI BVT Run
**Sumarry:**
Build Number 405
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=1
Skipped=7
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* test_non_contigiousvlan.py
* test_extendPhysicalNetworkVlan Failed
**Skipped tests:**
test_01_test_vm_volume_snapshot
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_11_ss_nfs_version_on_ssvm
test_nested_virtualization_vmware
test_3d_gpu_support
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_loadbalance.py
test_routers.py
test_reset_vm_on_reboot.py
test_deploy_vms_with_varied_deploymentplanners.py
test_network.py
test_router_dns.py
test_login.py
test_deploy_vm_iso.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_routers_network_ops.py
test_disk_offerings.py
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@rafaelweingartner Sorry for the late reply. But it seems you have misunderstood me here. I am not pointing finger here. I was just trying to point out that there may be developer concern there to not remove it. Some changes looks quite simple but have potential to cause regressions. That's why I have done this way. I prefer to have code improvements of that type of changes in a separate PR. If anything goes wrong then they can be easily reverted without causing any regression. In other words fix for some bug doesn't get accidentally removed.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@abhinandanprateek #1860 should take care of last host id scenario. Other case of user specifying host, should we not allow deploying VM in disabled Pod? That's a separate issue by the way.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@rhtyd , Rebased against laster master.
---
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-9198: Virtual router gets depl...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1278#issuecomment-170300127
I don't understand how the non-system user prevents the VR's deployment on a disabled Pod.
It seems that neither user or account will be used in the VR start method (line 266). Please correct me if I am wrong.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1278
Hi @rafaelweingartner @anshul1886 @GabrielBrascher,
I've read this PR's comments several times and I think I could understand @anshul1886's point. Please correct me if I'm wrong. The execution of `getCallingAccount()` is setting the context with the proper account, and I think that's fine, as next methods will use it (e.g. `orchestrateStart` in `VirtualMachineManagerImpl` lines 829-831).
I also agree with @rafaelweingartner and @GabrielBrascher that even though the context is being set, variables `user` and `caller` on `start` method (defined on line 266) are not being used. @anshul1886, if no validations are required and the context is already set, don't you think that those unused parameters can be removed?
---
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-9198: Virtual router gets depl...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/1278#issuecomment-219621121
@rhtyd There are no conflicts here so I don't understand what will be achieve by rebase?
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@anshul1886, this pointing finger thing is not good.
I do not know why people did not do the work as it should have been done before. I was probably not around when that was done. I only asked you to remove those variables because you were touching the code in which they are found. It is not only with you, every time I review a code and there is room for improvements, I always suggest it. I also measure my suggestions, I will never ask something huge; normally I ask/suggest for small and concise improvements such as the removal of unused variables/blocks of codes.
I was probably present in most of the PRs created by @nvazquez, you can see how this type of discussion improved greatly all of the code he had already worked on.
If you do not want to remove something that is not being used is fine. However, I would like a clarification. If the variables you are changing are not used (as you finally admitted), then how can changing them solve the problem you reported on CLOUDSTACK-9198?
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 closed the pull request at:
https://github.com/apache/cloudstack/pull/1278
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@anshul1886 Should not we disallow any VM to be deployed in a disabled Pod, why just the VR ?
cc @rhtyd @karuturi
Lets merge this shapeblue/cloudstack-apple@359f652 instead
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@abhinandanprateek @koushik-das That change is intentional to allow admin users to deploy VM on disabled. Ticket which shows that it is intentional https://issues.apache.org/jira/browse/CLOUDSTACK-7047.
---
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-9198: Virtual router gets depl...
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1278#issuecomment-222276364
### ACS CI BVT Run
**Sumarry:**
Build Number 68
Hypervisor xenserver
NetworkType Advanced
Passed=69
Failed=4
Skipped=3
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* test_vpc_vpn.py
* ContextSuite context=TestRVPCSite2SiteVpn>:setup Failing since 2 runs
* ContextSuite context=TestVpcRemoteAccessVpn>:setup Failing since 3 runs
* ContextSuite context=TestVpcSite2SiteVpn>:setup Failing since 3 runs
* test_vm_life_cycle.py
* test_10_attachAndDetach_iso Failing since 2 runs
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_disk_offerings.py
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@abhinandanprateek, That's the behaviour we disallow VM to be deployed in disabled Pod. But Virtual router was getting deployed as it was getting called through system user for which we allow deployment.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@koushik-das @anshul1886 If we allow admin to deploy resources on disabled resources then this fix is fine. Even with the fix a non admin user will be able to restart a VM on a disabled resource, so limited by that. Boils down to definition of disabled.
I had done a fix in past that will prevent deployment on disabled pods even for admin, created a Pr for that now, check this out too: https://github.com/apache/cloudstack/pull/1979/files. I think it just boils down to the definition of disabled.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@anshul1886 If you trace the method planDeployment in DeploymentPlanningManagerImpl then you will see that orderCluster is getting invoked at line 500. By that time I think the deployment plan for a VM that has hostid or lasthost id populated is already returned (line 472 and 369). This will allow vm to be started on a disabled pod as well as any VM that is created for a host in a disabled pod to be allowed as host id is pre-set for such VMs. There should be additional checks above to disallow a disabled pod.
---
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-9198: Virtual router gets depl...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/1278#issuecomment-171169936
@GabrielBrascher System User can start VR on disabled Pod.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1278
Great @anshul1886, we seem to start converging.
So, what you call setting context, is the execution of \u201corg.apache.cloudstack.context.CallContext.getCallingAccount()\u201d, that returns an account object(the account that is executing the API call). This method works in a lazy way, which means, it loads the \u201caccount\u201d only when it is called at the first time. And this is what you consider setting the context.
Now let\u2019s get back to what I am questioning you.
Let\u2019s get as an example the method you talked about before \u201ccom.cloud.storage.VolumeApiServiceImpl.uploadVolume(UploadVolumeCmd)\u201d. At line 275, it loads the account object, which is then sent as a parameter to the method \u201ccom.cloud.storage.VolumeApiServiceImpl.validateVolume(Account, long, Long, String, String, String, Long)\u201d. Then, at line 381 the account object is used to check if the account executing the command has the proper permissions.
This is exactly my point here. Please check the variables, they are not being used anywhere. The variables I am mentioning are only passed as parameters, but the methods that receive them, do not make use of these variables.
I am starting to think I am not seeing things here; I think we could benefit from some fresh eyes here. @DaanHoogland, @rhtyd, @nvazquez, could you lend us your fresh eyes for 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-9198: Virtual router gets depl...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1278#issuecomment-171284609
@anshul1886 I am sorry if I wasn't clear. You are using the com.cloud.network.router.NetworkHelperImpl.startVirtualRouter(DomainRouterVO, User, Account, Map<Param, Object>) method (implemented at line 347) which executes the com.cloud.network.router.NetworkHelperImpl.start(DomainRouterVO, User, Account, Map<Param, Object>, DeploymentPlan) method (line 266) with the user and account (modified by your code). My point is that the user and account parameters are not used by the start method. My suggestion is to remove those unused parameters from start and startVirtualRouter methods.
Are you sure that the change you introduce fixed your problem?
---
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-9198: Virtual router gets depl...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1278#issuecomment-216219194
@anshul1886 please rebase against master, thanks
an integration test would be great
---
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-9198: Virtual router gets depl...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1278#issuecomment-171619337
@anshul1886 I did not understand what you wanted to express presenting those commits, would you care to explain? I noticed the commits you pointed out, are pretty old and do not correspond to the code we have in master branch today.
The Point that @GabrielBrascher touched makes sense to me, have you looked at the piece of code he pointed at?
I looked at the code, the variables you changed are not used. If they should be used, that has to be properly coded, if not, they should be removed.
In details for you:
You changed the method com.cloud.network.router.NetworkHelperImpl.startRouters(RouterDeploymentDefinition), to use the called user as the one returned by the code “_accountMgr.getActiveUser(CallContext.current().getCallingUserId())” and the Account by the one returned by the method“CallContext.current().getCallingAccount();”.
Then, at line 336, it is called the method “com.cloud.network.router.NetworkHelperImpl.startVirtualRouter(DomainRouterVO, User, Account, Map<Param, Object>)” with the aforementioned variables you changed. That method does not use either one use of those variables you changed, it only uses them to execute the method “com.cloud.network.router.NetworkHelperImpl.start(DomainRouterVO, User, Account, Map<Param, Object>, DeploymentPlan)”. Those executions occur at lines 349, 387 or 412, depending on some logic.
After that, the method “com.cloud.network.router.NetworkHelperImpl.start(DomainRouterVO, User, Account, Map<Param, Object>, DeploymentPlan)”, receives its parameters and performs its job. However, it does not use the “User” and “Account” parameters in any of its operations.
At the end, the variables were not being used before your change and they are not being used now. Therefore, I do not see how that simple change can solve a problem.
Do you intend to alter the code of “com.cloud.network.router.NetworkHelperImpl.start(DomainRouterVO, User, Account, Map<Param, Object>, DeploymentPlan)” too ?
---
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-9198: Virtual router gets depl...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1278#discussion_r49269955
--- Diff: server/src/com/cloud/network/router/NetworkHelperImpl.java ---
@@ -509,7 +511,9 @@ public DomainRouterVO deployRouter(final RouterDeploymentDefinition routerDeploy
if (startRouter) {
try {
- router = startVirtualRouter(router, _accountMgr.getSystemUser(), _accountMgr.getSystemAccount(), routerDeploymentDefinition.getParams());
+ final Account caller = CallContext.current().getCallingAccount();
--- End diff --
@anshul1886 could you please create a method for the new lines? As it is a duplicate code, a method keeps the code clean and helps in future works (especially if well documented).
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@nvazquez @rafaelweingartner @GabrielBrascher, That method is called from multiple places. There are many places where we can do these kind of changes. I would prefer to have those kind of changes in PR specific to that so that they are easy to track and test for that specific purpose.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@anshul1886 @koushik-das I think with above info the fix looks good.
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@rafaelweingartner, Yes we can agree on those changes. But with additional description when the CallContext.current().getCallingAccount() call is made then it is setting the proper context which is global in nature and passed automatically to next method calls.
---
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-9198: Virtual router gets depl...
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1278#issuecomment-202714007
### ACS CI BVT Run
**Sumarry:**
Build Number 142
Hypervisor xenserver
NetworkType Advanced
Passed=99
Failed=3
Skipped=4
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* integration.smoke.test_privategw_acl.TestPrivateGwACL
* test_01_vpc_privategw_acl Failed
* integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
* test_01_reset_vm_on_reboot Failed
* integration.smoke.test_volumes.TestCreateVolume
* test_06_download_detached_volume Failing since 2 runs
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso
**Passed test suits:**
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData
integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_snapshots.TestSnapshotRootDisk
integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_disk_offerings.TestCreateDiskOffering
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@anshul1886 I am not sure I thought deployment planner for VR and a regular VM is same. If a regular VM is not allowed in a cluster that is under disabled pod then the VR should also be not allowed. Is there some override somewhere ?
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@abhinandanprateek, Yes they are using same deployment planner. But VR was getting created in disabled Pod but not regular VM because for VR deployment system user context was getting set. In planner checks are not done/skipped for system user. So if you observe here fix is to use user from calling context instead of system user. Hope this clears your doubt.
One such example:
`if (!isRootAdmin(vmProfile)) {
List<Long> disabledPods = listDisabledPods(plan.getDataCenterId());
if (!disabledPods.isEmpty()) {
if (s_logger.isDebugEnabled()) {
s_logger.debug("Removing from the podId list these pods that are disabled: " + disabledPods);
}
podsWithCapacity.removeAll(disabledPods);
}
}`
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@GabrielBrascher @rafaelweingartner, Please go through deployment planner code. Deployment planner decides the VM placement and virtual router also goes through that. Deployment planners uses allocators and planners. In those planners and allocators you will find the admin account related checks as pointed out in one of my previous comment. My intent to point out those old commits of Calling context introduction was that with that introduction there is calling context is passed around implicit. Here the code changes are making sure that context gets changed to proper user.
Hope this clears your doubt.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@rafaelweingartner stop start destroy etc all methods related to virtual router code have these parameters? Questions there is why are they not removed in the first place when they CallContext got introduced and removed at some places. That's why I would prefer to have different PR specifically for this. If something breaks then that can be easily tackled without the need of reverting some bug fix which may get lost later.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1278
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-545
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1278
First, I would like to apologize for being so stubborn, but if I do not understand something I like to ask questions. I hope you do not mind.
Before we proceed with the discussion. Is the description I created about the changes you are applying accurate?
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@anshul1886 Should not we disallow any VM to be deployed in a disabled Pod, why just the VR ?
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed...
Posted by anshul1886 <gi...@git.apache.org>.
GitHub user anshul1886 reopened a pull request:
https://github.com/apache/cloudstack/pull/1278
CLOUDSTACK-9198: Virtual router gets deployed in disabled Pod
While starting the router, send the user from the callingContext instead of defaulting to System user.
https://issues.apache.org/jira/browse/CLOUDSTACK-9198
## To test:
Verify that Virtual Router is not getting deployed in disabled Pod.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 CLOUDSTACK-9198
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1278.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 #1278
----
commit d3594526294601bcfda01415f370f4bc0e71e910
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-11-02T08:28:44Z
CLOUDSTACK-9198: Virtual router gets deployed in disabled Pod. While starting the
router, send the user from the callingContext instead of defaulting to System user
----
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@anshul1886 what method that is called in multiple places are you talking about?
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@anshul1886 I would like to raise the point previously discussed by me and @rafaelweingartner. I think that we should pay attention if the change of user and caller will really do the job. So far I do not see how this PR changes the behavior.
Basically this code changes two parameters in startVirtualRouter [_callerUser_ and _caller_ when calling startVirtualRouter(router, callerUser, caller, routerDeploymentDefinition.getParams())]. However, those parameters are only used inside startVirtualRouter when calling the method start(router, user, caller, params, null).
```
if (router.getRole() != Role.VIRTUAL_ROUTER || !router.getIsRedundantRouter()) {
return start(router, user, caller, params, null);
}
```
The problem is that the method **start** does not use either the _user_ and the _caller_ parameters in the overridden implementation (the one that you are using).
```
protected DomainRouterVO start(DomainRouterVO router, final User user, final Account caller, final Map<Param, Object> params, final DeploymentPlan planToDeploy)
throws StorageUnavailableException, InsufficientCapacityException, ConcurrentOperationException, ResourceUnavailableException {
s_logger.debug("Starting router " + router);
try {
_itMgr.advanceStart(router.getUuid(), params, planToDeploy, null);
} catch (final OperationTimedoutException e) {
throw new ResourceUnavailableException("Starting router " + router + " failed! " + e.toString(), DataCenter.class, router.getDataCenterId());
}
if (router.isStopPending()) {
s_logger.info("Clear the stop pending flag of router " + router.getHostName() + " after start router successfully!");
router.setStopPending(false);
router = _routerDao.persist(router);
}
// We don't want the failure of VPN Connection affect the status of
// router, so we try to make connection
// only after router start successfully
final Long vpcId = router.getVpcId();
if (vpcId != null) {
_s2sVpnMgr.reconnectDisconnectedVpnByVpc(vpcId);
}
return _routerDao.findById(router.getId());
}
```
Sorry, but I can't see how your code alters the behavior as intended. Can you please show that by changing the parameters _user_ and _caller_ you are changing the behavior?
Thanks in advance.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@anshul1886 Should not we disallow any VM to be deployed in a disabled Pod, why just the VR ?
cc @rhtyd @karuturi
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@rafaelweingartner, Basically CallContext.current().getCallingAccount(); is setting the proper context which is later used. If you have observed in code all the api calls have a similar call before proceeding further. One random example:
```java
@ActionEvent(eventType = EventTypes.EVENT_VOLUME_UPLOAD, eventDescription = "uploading volume", async = true)
public VolumeVO uploadVolume(UploadVolumeCmd cmd) throws ResourceAllocationException {
Account caller = CallContext.current().getCallingAccount();
```
For more details refer https://cwiki.apache.org/confluence/display/CLOUDSTACK/Using+CallContext and commits 7928963 and 11e1e58 which I have mentioned earlier.
Hopefully this clears your doubts.
---
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-9198: Virtual router gets depl...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/1278#issuecomment-171536737
Refer commits 7928963d16 and 11e1e585 to get info about Calling context.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@blueorangutan package
---
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-9198: Virtual router gets depl...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1278#discussion_r49552290
--- Diff: server/src/com/cloud/network/router/NetworkHelperImpl.java ---
@@ -509,7 +511,9 @@ public DomainRouterVO deployRouter(final RouterDeploymentDefinition routerDeploy
if (startRouter) {
try {
- router = startVirtualRouter(router, _accountMgr.getSystemUser(), _accountMgr.getSystemAccount(), routerDeploymentDefinition.getParams());
+ final Account caller = CallContext.current().getCallingAccount();
--- End diff --
@GabrielBrascher Extracting those lines in a method will remove the contextual information from them and will impact the understanding in future so it is done this way.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the issue:
https://github.com/apache/cloudstack/pull/1278
I think disabled resource related checks are only applicable for regular users, root/system users can go ahead and deploy VMs in disabled resources as well.
---
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 #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1278
@anshul1886 sorry, but I still do not understand you.
Your changes affect `NetworkHelperImpl.java` class. Do we agree on that?
You changed lines 334 and 512. You changed the call `startVirtualRouter`; this method is implemented in the same `NetworkHelperImpl.java` class. Before you change the call was `startVirtualRouter(router, _accountMgr.getSystemUser(), _accountMgr.getSystemAccount(), routerDeploymentDefinition.getParams())`; then, you altered the code to `startVirtualRouter(router, callerUser, caller, routerDeploymentDefinition.getParams())`.
You changed the way the code is retrieving objects \u201cuser\u201d and \u201caccount\u201d; before the user was retrieved as `_accountMgr.getSystemUser()`, now it is retrieved as `_accountMgr.getActiveUser(CallContext.current().getCallingUserId())`. The account was obtained as `_accountMgr.getSystemAccount()` and now it is obtained as `CallContext.current().getCallingAccount()`.
Can we agree on this?
You are telling me to look at the Deployment planner. However, what I fail to understand is the following.
Looking at lines 334 and 512, the `user` and `account` are passed as parameters to `com.cloud.network.router.NetworkHelperImpl.startVirtualRouter(DomainRouterVO, User, Account, Map<Param, Object>)`. This method is in the same ` NetworkHelperImpl` class. The only thing the ` startVirtualRouter` method does with the mentioned objects is to send them as parameters to ` com.cloud.network.router.NetworkHelperImpl.start(DomainRouterVO, User, Account, Map<Param, Object>, DeploymentPlan)` at lines 349, 387, or 412. Then, the method to ` com.cloud.network.router.NetworkHelperImpl.start(DomainRouterVO, User, Account, Map<Param, Object>, DeploymentPlan)` does not use the ` User` and `Account` objects.
Can we agree on that?
---
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.
---