CLOUDSTACK-9203 Implement security group move on updateVM API call #1297

DaanHoogland · 2015-12-30T09:14:14Z

cherry-picked from a exoscale internal fix

Conflicts:
api/src/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java
server/src/com/cloud/vm/UserVmManager.java
server/src/com/cloud/vm/UserVmManagerImpl.java

DaanHoogland · 2015-12-30T09:16:24Z

@borisroman @wido as of now this is just a place holder as some extra requirements are put in by @resmo and no testing is done yet. I will update with at least the extra parameter and hopefully some test data. @resmo can you help with the testing? Thanks @llambiel and @pyr for the contribution.

DaanHoogland · 2015-12-30T09:19:01Z

ping @ustcweizhou

DaanHoogland · 2015-12-31T22:02:01Z

done some testing:

with cloudmonkey:

🐵 > update virtualmachine id=39f058ad-3021-4753-9c96-1aa922a74632 securitygroupnames=exceptional
Error 431: Virtual machine must be stopped prior to update security groups

🐵 > stop virtualmachine id=39f058ad-3021-4753-9c96-1aa922a74632
...
🐵 > update virtualmachine id=39f058ad-3021-4753-9c96-1aa922a74632 securitygroupnames=exceptional
...

resulting in

DaanHoogland · 2015-12-31T22:03:31Z

pinging every badoy again: @borisroman @wido @resmo @llambiel @pyr @bhaisaab @wilderrodrigues

resmo · 2016-01-01T09:46:26Z

LGTM

wido · 2016-01-05T12:33:35Z

server/src/com/cloud/vm/UserVmManagerImpl.java

Shouldn't there be a check for NULL here?

The updateVirtualMachine below can handle the null value (see line 2463)

wido · 2016-01-05T12:34:05Z

One comment I have on the code. Otherwise it looks good.

wido · 2016-01-05T12:58:33Z

Checked all the commits, re-read the code and LGTM. Based on the tests already performed.

DaanHoogland · 2016-01-08T14:24:03Z

@borisroman @wido @resmo @llambiel @pyr @bhaisaab @wilderrodrigues I can run the tests again but I am the contributor. Can someone else, please?

ustcweizhou · 2016-01-12T09:17:52Z

this does not work if there are multiple shared network in advanced zone with sg.
I will fix it.

DaanHoogland · 2016-01-16T11:31:05Z

@wido @remibergsma Can we merge?

remibergsma · 2016-01-16T18:45:33Z

@DaanHoogland Did I miss the integration tests?

DaanHoogland · 2016-01-16T21:59:49Z

@remibergsma the api was tested in sg. We can run a full regression suit if you feel it is needed. I don't feel we need to test anything but the call with updated security groups.

DaanHoogland · 2016-01-16T22:00:18Z

btw it also is based on exoscale production code

ustcweizhou · 2016-01-16T22:05:20Z

LGTM, based on manual functional testing in advanced zone with security groups.

wido · 2016-01-27T20:02:15Z

Can we rebase against master (4.9)? Seems that this one is ready to merge

bvbharatk · 2016-03-28T00:31:58Z

ACS CI BVT Run

Sumarry:
Build Number 137
Hypervisor xenserver
NetworkType Advanced
Passed=101
Failed=1
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

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_reset_vm_on_reboot.TestResetVmOnReboot
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_volumes.TestCreateVolume
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

NuxRo · 2016-04-11T12:34:08Z

Daan,

How did you test?
I'm trying to apply the patch to either 4.8.0 or master and on neither it does so cleanly:

Hunk #7 FAILED at 467.
1 out of 7 hunks FAILED -- saving rejects to file api/src/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java.rej

DaanHoogland · 2016-04-11T12:39:01Z

@nux, it probably doesn't apply anymore. This PR is 4 months old :(

swill · 2016-04-11T14:03:58Z

@NuxRo asked that I review this. @DaanHoogland I see you opened this, would you mind rebasing. I suspect it is still valid considering the fact that not a huge amount of code has been merged since you opened this.

DaanHoogland · 2016-04-11T19:10:17Z

new integration run pending

DaanHoogland · 2016-04-12T07:23:46Z

CI bubble run

Commit Reference: de53cd1

initially two test failed. I reran the 'unusual' failure and it succeeded.

1297.results.network.txt
1297.results.vpc_routers.txt
1297.results.routers_network_ops.txt

DaanHoogland · 2016-04-12T07:25:36Z

@ustcweizhou Can you review, the rebasing led to more commits then expected. The result looks fine but I am not sure.

DaanHoogland · 2016-04-12T07:27:32Z

@remibergsma @swill this one test always failing in our environments doesn't sit good with me. It feels like we are accepting a crippled system to be carried. I'll spend some time there.

swill · 2016-04-12T13:50:15Z

@DaanHoogland for the test that often fails in my environment, check @remibergsma's comments here: #1449 (comment)

DaanHoogland · 2016-04-12T13:57:25Z

@swill that is definitely not the same issue as I'm having. it is in the same test however

DaanHoogland · 2016-05-20T23:09:18Z

@swill I think it is genuine.

swill · 2016-05-20T23:12:51Z

@DaanHoogland you think the issue that travis is having is a genuine issue, is that what you mean?

DaanHoogland · 2016-05-20T23:23:54Z

Yes, I'm just not sure it is Travis or the xen plug-in.

swill · 2016-05-20T23:29:15Z

OK. I don't really have the bandwidth to troubleshoot this (unfortunately). This is one of the many PRs that are on the bubble which I am trying to figure out what to do with. For now I am thinking not to merge it because this issue seems to be only happening with this PR. I am already almost a week late (of my own imposed deadline), so I have to make hard decisions soon on some of these.

DaanHoogland · 2016-05-20T23:40:37Z

I might find the time but it is long since I worked with xen. Also I must find new joy for this hobby. So no promises

swill · 2016-05-21T00:10:14Z

Have a nice weekend. :)

remibergsma · 2016-05-22T18:11:14Z

server/src/com/cloud/vm/UserVmManagerImpl.java

+        DataCenterVO zone = _dcDao.findById(vm.getDataCenterId());
+        Network defaultNetwork = _networkModel.getExclusiveGuestNetwork(zone.getId());
+
+        boolean isVmWare = (vm.getHypervisorType() == HypervisorType.VMware);


Small detail: Please check the spelling of VMware, I see lots of different variations and believe VMware is the right one.

DaanHoogland · 2016-05-23T10:26:18Z

@swill, i had a nice weekend, thanks. The test that is failing throws a different kind of exception if I run it on master in my bubble. I have just pushed a limited part of the change to see what it does and if the exception is still the same.

DaanHoogland · 2016-05-23T10:40:04Z

@swill, the test_scale_vm didn't error out because it wasn't run on this version! I have lost a tiny bit of confidence in our travis runs again. I will now start adding the rest of the code again.

DaanHoogland · 2016-05-23T12:59:55Z

got the test to run locally with

mvn clean install
ssh cs0 /data/shared/helper_scripts/cloudstack/build_run_deploy_test.sh -m /data/git/cs1/cloudstack/setup/dev/advanced.cfg -S -s -t -f /data/shared/helper_scripts/cloudstack/run_scale_vm.sh

or

ssh cs0 /data/shared/helper_scripts/cloudstack/build_run_deploy_test.sh -m /data/git/cs1/cloudstack/setup/dev/advanced.cfg -S -t -f /data/shared/helper_scripts/cloudstack/run_scale_vm.sh

, so the rest is office-time-work

NuxRo · 2016-05-23T13:45:41Z

Thanks Daan,

Does that mean we're good to go?

DaanHoogland · 2016-05-23T13:59:44Z

@NuxRo it means debugging the problem has started. what is in here now is not much (you can review it)

cherry-picked from a exoscale internal fix: Implement security group move on updateVM API call Prevent security group update while instance is running Conflicts: api/src/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java server/src/com/cloud/vm/UserVmManager.java server/src/com/cloud/vm/UserVmManagerImpl.java

Conflicts: api/src/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java api/src/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java

DaanHoogland · 2016-05-23T21:13:57Z

@NuxRo can you test if it is still working for you?
@ustcweizhou can you see if your fixes are still met?
@swill can you reschedule integration tests?

swill · 2016-05-24T03:56:08Z

CI RESULTS

Tests Run: 82
  Skipped: 0
   Failed: 0
   Errors: 3
 Duration: 8h 35m 03s

Summary of the problem(s):

ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestRVPCSite2SiteVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
    self.setUp()
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
    self.setupContext(ancestor)
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
    try_run(context, names)
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
    return func()
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 835, in setUpClass
    cls.template.download(cls.apiclient)
  File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
    elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_TJUSR5/results.txt

ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestVpcRemoteAccessVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
    self.setUp()
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
    self.setupContext(ancestor)
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
    try_run(context, names)
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
    return func()
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 293, in setUpClass
    cls.template.download(cls.apiclient)
  File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
    elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_TJUSR5/results.txt

ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestVpcSite2SiteVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
    self.setUp()
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
    self.setupContext(ancestor)
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
    try_run(context, names)
  File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
    return func()
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 472, in setUpClass
    cls.template.download(cls.apiclient)
  File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
    elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_TJUSR5/results.txt

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__May_22_2016_06_11_44_7O49QH:

/tmp/MarvinLogs/test_network_TJUSR5:

/tmp/MarvinLogs/test_vpc_routers_179GPQ:

Uploads will be available until 2016-07-24 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-05-24T03:57:26Z

This is coming back clean now. Those failures are not related to this PR.

DaanHoogland · 2016-05-24T08:36:49Z

@swill, do you know what those errors are about? (always a dizzy feeling when this happens)

swill · 2016-05-24T13:56:42Z

Apparently status is missing from the listTemplates response in some cases. I will investigate after we freeze and before we cut an RC to see if we can resolve it.

CLOUDSTACK-9203 Implement security group move on updateVM API call cherry-picked from a exoscale internal fix Conflicts: api/src/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java server/src/com/cloud/vm/UserVmManager.java server/src/com/cloud/vm/UserVmManagerImpl.java * pr/1297: CLOUDSTACK-9203 refactorred DeployVM code to be used by UpdateVM as well CLOUDSTACK-9203 security group update on running instance Signed-off-by: Will Stevens <[email protected]>

DaanHoogland force-pushed the CLOUDSTACK-9203 branch from 70e6897 to bf7ab1f Compare December 31, 2015 09:34

wido reviewed Jan 5, 2016
View reviewed changes

DaanHoogland closed this Apr 11, 2016

DaanHoogland reopened this Apr 11, 2016

DaanHoogland force-pushed the CLOUDSTACK-9203 branch from 4e86f9a to de53cd1 Compare April 11, 2016 19:08

DaanHoogland force-pushed the CLOUDSTACK-9203 branch from f72c8f2 to c55d82b Compare May 21, 2016 13:30

remibergsma reviewed May 22, 2016
View reviewed changes

DaanHoogland force-pushed the CLOUDSTACK-9203 branch 2 times, most recently from 00d83b5 to c8d848d Compare May 23, 2016 10:19

DaanHoogland force-pushed the CLOUDSTACK-9203 branch from c8d848d to 17ffcec Compare May 23, 2016 11:00

DaanHoogland closed this May 23, 2016

DaanHoogland force-pushed the CLOUDSTACK-9203 branch from 9723768 to 570b676 Compare May 23, 2016 11:48

DaanHoogland reopened this May 23, 2016

DaanHoogland force-pushed the CLOUDSTACK-9203 branch from 50992aa to 5877293 Compare May 23, 2016 20:35

CLOUDSTACK-9203 refactorred DeployVM code to be used by UpdateVM as well

78cd64a

Conflicts: api/src/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java api/src/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java

asfgit merged commit 78cd64a into apache:master May 26, 2016

DaanHoogland deleted the CLOUDSTACK-9203 branch August 16, 2023 11:30

CLOUDSTACK-9203 Implement security group move on updateVM API call #1297

CLOUDSTACK-9203 Implement security group move on updateVM API call #1297

Uh oh!

Conversation

DaanHoogland commented Dec 30, 2015

Uh oh!

DaanHoogland commented Dec 30, 2015

Uh oh!

DaanHoogland commented Dec 30, 2015

Uh oh!

DaanHoogland commented Dec 31, 2015

Uh oh!

DaanHoogland commented Dec 31, 2015

Uh oh!

resmo commented Jan 1, 2016

Uh oh!

wido Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

wido commented Jan 5, 2016

Uh oh!

wido commented Jan 5, 2016

Uh oh!

DaanHoogland commented Jan 8, 2016

Uh oh!

ustcweizhou commented Jan 12, 2016

Uh oh!

DaanHoogland commented Jan 16, 2016

Uh oh!

remibergsma commented Jan 16, 2016

Uh oh!

DaanHoogland commented Jan 16, 2016

Uh oh!

DaanHoogland commented Jan 16, 2016

Uh oh!

ustcweizhou commented Jan 16, 2016

Uh oh!

wido commented Jan 27, 2016

Uh oh!

bvbharatk commented Mar 28, 2016

ACS CI BVT Run

Uh oh!

NuxRo commented Apr 11, 2016

Uh oh!

DaanHoogland commented Apr 11, 2016

Uh oh!

swill commented Apr 11, 2016

Uh oh!

DaanHoogland commented Apr 11, 2016

Uh oh!

DaanHoogland commented Apr 12, 2016

CI bubble run

Uh oh!

DaanHoogland commented Apr 12, 2016

Uh oh!

DaanHoogland commented Apr 12, 2016

Uh oh!

swill commented Apr 12, 2016

Uh oh!

DaanHoogland commented Apr 12, 2016

Uh oh!

DaanHoogland commented May 20, 2016

Uh oh!

swill commented May 20, 2016

Uh oh!

DaanHoogland commented May 20, 2016

Uh oh!

swill commented May 20, 2016

Uh oh!

DaanHoogland commented May 20, 2016

Uh oh!

swill commented May 21, 2016

Uh oh!

remibergsma May 22, 2016

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented May 23, 2016

DaanHoogland commented May 23, 2016 •

edited

Loading