You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by bhaisaab <gi...@git.apache.org> on 2016/01/25 18:36:17 UTC
[GitHub] cloudstack pull request: vmware: improve support for disk controll...
GitHub user bhaisaab opened a pull request:
https://github.com/apache/cloudstack/pull/1365
vmware: improve support for disk controllers
- Improve disk chain usage while attaching, migrating disks
- Gets root disk controller based diskDeviceBusName from volume's chain info
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shapeblue/cloudstack 4.7-vmware-diskchain
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1365.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 #1365
----
commit fb78e9992d86db0f6fbf9e95cbe104cc5eddb131
Author: Rohit Yadav <ro...@shapeblue.com>
Date: 2016-01-25T16:56:01Z
vmware: improve support for disk controllers
- Improve disk chain usage while attaching, migrating disks
- Gets root disk controller based diskDeviceBusName from volume's chain info
Signed-off-by: Rohit Yadav <ro...@shapeblue.com>
----
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-211479815
if you say so.
Be my guest.
---
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: [4.7] vmware: improve support for disks
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-177495811
@bhaisaab Thanks for the test and StringUtils fix.
**-** Could you please change the Access Levels of diskDeviceBusName and diskChain from public to private (at class VirtualMachineDiskInfo)?
I asked for a Javadoc at the getControllerFromDeviceBusName() method because at first I thought that it was a simple get of a private String, then I saw the logic inside it.
I agree that the method is clean and easy to understand, so it is your call. Maybe it would be interesting to change its name then.
As a javadoc I was expecting something simple as:
`/**Returns the diskDeviceBusName substring, with the index starting at ":"*/`
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-211565374
@rafaelweingartner I am not sure. @bhaisaab would you mind clarifying for us. I am trying to stay on top of this stuff, but I am very much playing catchup. :)
---
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: [4.7] vmware: improve support for disks
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216571886
@swill I haven't worked much on that file (I had fixed a typo in the name of the "ROOT_DISK_CONTROLLER" variable).
At the first look I would agree with @mike-tutkowski, but I have a limited background on this file. I need to look with more care to be helpful.
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58786110
--- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java ---
@@ -2137,7 +2141,8 @@ public int getScsiDiskControllerKey(String diskController) throws Exception {
}
assert (false);
- throw new Exception(diskController + " Controller Not Found");
+ throw new Exception("Scsi disk controller of type " + diskController + " not found among configured devices:" + deviceList +
--- End diff --
Throwing ``Exception`` is an anti-pattern. Use a specific checked or unchecked exception type to better express the nature of the problem (e.g. ``IllegalStateException``). Also, since an exception is being thrown, ``assert(false)`` is not necessary and 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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-211785845
@swill we're good with this PR and it is not dependent on any other PR. For building noredist rpms/deb, one can use this repo with the dep-jars -- https://github.com/bhaisaab/cloudstack-nonoss
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58743708
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -5455,6 +5456,15 @@ private void encryptAndStorePassword(UserVmVO vm, String password) {
}
}
+ public void persistDeviceBusInfo(UserVmVO vm, String rootDiskController) {
+ String existingVmRootDiskController = vm.getDetail(VmDetailConstants.ROOK_DISK_CONTROLLER);
--- End diff --
what about fixing the typo "ROOK_DISK_CONTROLLER"?
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58904519
--- Diff: plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java ---
@@ -1363,24 +1363,17 @@ private Answer attachVolume(Command cmd, DiskTO disk, boolean isAttach, boolean
AttachAnswer answer = new AttachAnswer(disk);
if (isAttach) {
- String dataDiskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
- String rootDiskController = controllerInfo.get(VmDetailConstants.ROOK_DISK_CONTROLLER);
- DiskControllerType rootDiskControllerType = DiskControllerType.getType(rootDiskController);
-
- if (dataDiskController == null) {
- dataDiskController = getLegacyVmDataDiskController();
- } else if ((rootDiskControllerType == DiskControllerType.lsilogic) ||
- (rootDiskControllerType == DiskControllerType.lsisas1068) ||
- (rootDiskControllerType == DiskControllerType.pvscsi) ||
- (rootDiskControllerType == DiskControllerType.buslogic)) {
- //TODO: Support mix of SCSI controller types for single VM. If root disk is already over
- //a SCSI controller then use the same for data volume as well. This limitation will go once mix
- //of SCSI controller types for single VM.
- dataDiskController = rootDiskController;
- } else if (DiskControllerType.getType(dataDiskController) == DiskControllerType.osdefault) {
- dataDiskController = vmMo.getRecommendedDiskController(null);
+ String diskController = null;
+ if (controllerInfo != null) {
+ diskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
}
- vmMo.attachDisk(new String[] {datastoreVolumePath}, morDs, dataDiskController);
+ if (diskController == null) {
+ diskController = getLegacyVmDataDiskController();
--- End diff --
Fixed
---
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: [4.7] vmware: improve support for disks
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r51717944
--- Diff: core/src/org/apache/cloudstack/storage/command/AttachAnswer.java ---
@@ -34,6 +37,12 @@ public AttachAnswer(DiskTO disk) {
setDisk(disk);
}
+ public AttachAnswer(DiskTO disk, Map<String, String> diskDetails) {
--- End diff --
@bhaisaab Thanks for fixing the access level. The code seems ok, just one more question.
Sorry to ask you now, I should have seen this before. Is this constructor used (**AttachAnswer(DiskTO disk, Map<String, String> diskDetails)**)?
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-211479011
@rafaelweingartner both your comments are opinionated and not technical; we've to agree to disagree here ask you to be practical and pragmatic here. I've left comments above and I disagree on both mentioned places. Finally, what you would consider may be a huge change but for me a +190/-42 lines is not huge. If you go on this path, you might even find issues with quarks and neutrinos, and there is no end to over-engineering.
---
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216433479
Obviously this merge conflict exists because something has been refactored in master which was not merged into previous versions. We need to understand what was done and how to make sure both functionality can exist together... I had to run to catch a train when I ran into this, so I have not gotten too deep into which PR was merged into master which is conflicting with this.
---
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: [4.7] vmware: improve support for disks
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216185039
tag:easypr
---
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: [4.7] vmware: improve support for disks
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r61833203
--- Diff: plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java ---
@@ -1363,24 +1364,15 @@ private Answer attachVolume(Command cmd, DiskTO disk, boolean isAttach, boolean
AttachAnswer answer = new AttachAnswer(disk);
if (isAttach) {
- String dataDiskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
- String rootDiskController = controllerInfo.get(VmDetailConstants.ROOK_DISK_CONTROLLER);
- DiskControllerType rootDiskControllerType = DiskControllerType.getType(rootDiskController);
-
- if (dataDiskController == null) {
- dataDiskController = getLegacyVmDataDiskController();
- } else if ((rootDiskControllerType == DiskControllerType.lsilogic) ||
- (rootDiskControllerType == DiskControllerType.lsisas1068) ||
- (rootDiskControllerType == DiskControllerType.pvscsi) ||
- (rootDiskControllerType == DiskControllerType.buslogic)) {
- //TODO: Support mix of SCSI controller types for single VM. If root disk is already over
- //a SCSI controller then use the same for data volume as well. This limitation will go once mix
- //of SCSI controller types for single VM.
- dataDiskController = rootDiskController;
- } else if (DiskControllerType.getType(dataDiskController) == DiskControllerType.osdefault) {
- dataDiskController = vmMo.getRecommendedDiskController(null);
+ String diskController = getLegacyVmDataDiskController();
--- End diff --
@swill as you can see the diff here, we need the latter block:
```
String diskController = getLegacyVmDataDiskController();
if (controllerInfo != null &&
!Strings.isNullOrEmpty(controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER))) {
diskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
```
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58743070
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1924,6 +1935,23 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) {
throw new InvalidParameterValueException("Cannot migrate ROOT volume of a stopped VM to a storage pool in a different VMware datacenter");
}
}
+
+ String rootVolChainInfo = vol.getChainInfo();
+ if ((vm.getType().equals(VirtualMachine.Type.User)) && (rootVolChainInfo != null) && (!rootVolChainInfo.isEmpty())) {
+ String rootDiskController = null;
+ try {
+ VirtualMachineDiskInfo infoInChain = _gson.fromJson(rootVolChainInfo, VirtualMachineDiskInfo.class);
+ if (infoInChain != null) {
+ rootDiskController = infoInChain.getControllerFromDeviceBusName();
+ }
+ UserVmVO userVmVo = _userVmDao.findById(vm.getId());
+ if ((rootDiskController != null) && (!rootDiskController.isEmpty())) {
+ _userVmDao.loadDetails(userVmVo);
+ _userVmMgr.persistDeviceBusInfo(userVmVo, rootDiskController);
+ }
+ } catch (JsonParseException ignored) {
--- End diff --
At least a warning/debug message should be printed. This approach of silencing exception is never a good idea.
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-206185015
@swill fyi, this was a production issue that was delivered as a patch and this was heavily tested by ShapeBlue internally against vmware environment with several rounds of upgrade tests. This is now running in one of the large prod. environments for weeks now. With this, I'm going to merge this fix on 4.5 (https://github.com/apache/cloudstack/pull/1366), please help merge on 4.7+.
---
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216572547
No worries @GabrielBrascher, I realized after I CCed you that you really just fixed a typo. :) Thanks for the feedback...
---
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-206435758
I would like to get at least one code review with a LGTM before I merge. I am pretty confident with the tests run. As long as we get 105 tests passing in that CI environment things should be good. This run was from a while ago before he cleaned up the tests that always fail in that env, so this is pretty consistent with the latest passing runs...
@jburwell would you mind doing a quick once through of this for me for the LGTM? You are going to regret offering. :P
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58940609
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,26 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
}
}
+ public void updateMissingRootDiskController(final VMInstanceVO vm, final String rootVolChainInfo) {
+ if (vm == null || !vm.getType().equals(VirtualMachine.Type.User) || Strings.isNullOrEmpty(rootVolChainInfo)) {
--- End diff --
We can avoid a NEP here by changing
`vm.getType().equals(VirtualMachine.Type.User)`
to
`VirtualMachine.Type.User.equals(vm.getType())`
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58782174
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1924,6 +1935,23 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) {
throw new InvalidParameterValueException("Cannot migrate ROOT volume of a stopped VM to a storage pool in a different VMware datacenter");
}
}
+
--- End diff --
agree with @rafaelweingartner. All methods should have a single responsibility -- it is critical to maintainability and testability. While it is not possible to fix this problem all at once, we certainly shouldn't be adding to 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58931640
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,25 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
}
}
+ public void updateMissingRootDiskController(final VMInstanceVO vm, final String rootVolChainInfo) {
+ if (vm != null && vm.getType().equals(VirtualMachine.Type.User) && !Strings.isNullOrEmpty(rootVolChainInfo)) {
--- End diff --
@rafaelweingartner yeah why not, fixed
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58744278
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
--- End diff --
What about two or three unit tests for this method?
---
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: [4.7] vmware: improve support for disks
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r61833516
--- Diff: plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java ---
@@ -1363,24 +1364,15 @@ private Answer attachVolume(Command cmd, DiskTO disk, boolean isAttach, boolean
AttachAnswer answer = new AttachAnswer(disk);
if (isAttach) {
- String dataDiskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
- String rootDiskController = controllerInfo.get(VmDetailConstants.ROOK_DISK_CONTROLLER);
- DiskControllerType rootDiskControllerType = DiskControllerType.getType(rootDiskController);
-
- if (dataDiskController == null) {
- dataDiskController = getLegacyVmDataDiskController();
- } else if ((rootDiskControllerType == DiskControllerType.lsilogic) ||
- (rootDiskControllerType == DiskControllerType.lsisas1068) ||
- (rootDiskControllerType == DiskControllerType.pvscsi) ||
- (rootDiskControllerType == DiskControllerType.buslogic)) {
- //TODO: Support mix of SCSI controller types for single VM. If root disk is already over
- //a SCSI controller then use the same for data volume as well. This limitation will go once mix
- //of SCSI controller types for single VM.
- dataDiskController = rootDiskController;
- } else if (DiskControllerType.getType(dataDiskController) == DiskControllerType.osdefault) {
- dataDiskController = vmMo.getRecommendedDiskController(null);
+ String diskController = getLegacyVmDataDiskController();
--- End diff --
I'll fix the merge conflict
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-178696450
@GabrielBrascher thanks, fixed the accessor
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58786157
--- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java ---
@@ -2137,7 +2141,8 @@ public int getScsiDiskControllerKey(String diskController) throws Exception {
}
assert (false);
- throw new Exception(diskController + " Controller Not Found");
+ throw new Exception("Scsi disk controller of type " + diskController + " not found among configured devices:" + deviceList +
+ ". Unable to find and return scsi disk controller key.");
--- End diff --
Throwing ``Exception`` is an anti-pattern. Use a specific checked or unchecked exception type to better express the nature of the problem (e.g. ``IllegalStateException``). Also, since an exception is being thrown, ``assert(false)`` is not necessary and 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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58766497
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
--- End diff --
2 or 3 was a matter of speaking. It would be needed tests to cover all of the expected behavior, which I do not know.
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58762975
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1924,6 +1935,23 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) {
throw new InvalidParameterValueException("Cannot migrate ROOT volume of a stopped VM to a storage pool in a different VMware datacenter");
}
}
+
+ String rootVolChainInfo = vol.getChainInfo();
+ if ((vm.getType().equals(VirtualMachine.Type.User)) && (rootVolChainInfo != null) && (!rootVolChainInfo.isEmpty())) {
+ String rootDiskController = null;
+ try {
+ VirtualMachineDiskInfo infoInChain = _gson.fromJson(rootVolChainInfo, VirtualMachineDiskInfo.class);
+ if (infoInChain != null) {
+ rootDiskController = infoInChain.getControllerFromDeviceBusName();
+ }
+ UserVmVO userVmVo = _userVmDao.findById(vm.getId());
+ if ((rootDiskController != null) && (!rootDiskController.isEmpty())) {
+ _userVmDao.loadDetails(userVmVo);
+ _userVmMgr.persistDeviceBusInfo(userVmVo, rootDiskController);
+ }
+ } catch (JsonParseException ignored) {
--- End diff --
Fixed
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58743442
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1924,6 +1935,23 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) {
throw new InvalidParameterValueException("Cannot migrate ROOT volume of a stopped VM to a storage pool in a different VMware datacenter");
}
}
+
--- End diff --
What about extracting lines 1939-1954 to a method? This can really help us to work on that code and the unit tests it deserves.
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58782658
--- Diff: utils/src/main/java/org/apache/cloudstack/utils/volume/VirtualMachineDiskInfo.java ---
@@ -39,4 +38,11 @@ public void setDiskDeviceBusName(String diskDeviceBusName) {
public void setDiskChain(String[] diskChain) {
this.diskChain = diskChain;
}
+
+ public String getControllerFromDeviceBusName() {
+ if (StringUtils.isEmpty(diskDeviceBusName) || !diskDeviceBusName.contains(":")) {
+ return null;
+ }
+ return diskDeviceBusName.substring(0, diskDeviceBusName.indexOf(":") - 1);
+ }
--- End diff --
Please add a unit test for this method.
---
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: [4.7] vmware: improve support for disk co...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-176495485
@bhaisaab Could you please do the following changes?
At **VirtualMachineVolumeChainInfo** class:
**1 -** create Javadoc blocks (would be nice to document all the class, but I think that at least the class and the "getControllerFromDeviceBusName" method deserves a documentation);
**2 -** simplify the **if** conditional at line **43**. Test with "( (StringUtils.isEmpty) || (!this.diskDeviceBusName.contains(":")) )" condition. The StringUtils.isEmpty method already checks if the String is empty("") or null (https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html).
**3 -** create test for the getControllerFromDeviceBusName method.
At **UserVmManagerImpl** class:
**1 -** document the "persistDeviceBusInfo(UserVmVO vm, String rootDiskController)" method (lines **5458-5466**).
**2 -** use the "StringUtils.isEmpty()" with the condition at line **5460**, instead of "(existingVmRootDiskController==null) || (existingVmRootDiskController.isEmpty())".
**3 -** create test for "persistDeviceBusInfo(UserVmVO vm, String rootDiskController)" method.
**VolumeApiServiceImpl**:
**1 -** Remove “**_**” from variables names (lines **200**, **252**, **267**): private variables with “**_**” at the beginning is common in C++ but not in Java.
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-215506015
These CI results basically show that the standard VM workflow in KVM is not affected. I don't have the ability to test this on VMware, but I know you guys are already using this, yes?
I think I am pending one LGTM and this is ready...
---
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-215505081
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 0
Errors: 0
```
**Associated Uploads**
**`/tmp/MarvinLogs/DeployDataCenter__Apr_28_2016_06_35_56_T0QPTY:`**
* [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1365/tmp/MarvinLogs/DeployDataCenter__Apr_28_2016_06_35_56_T0QPTY/dc_entries.obj)
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1365/tmp/MarvinLogs/DeployDataCenter__Apr_28_2016_06_35_56_T0QPTY/failed_plus_exceptions.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1365/tmp/MarvinLogs/DeployDataCenter__Apr_28_2016_06_35_56_T0QPTY/runinfo.txt)
**`/tmp/MarvinLogs/test_network_IQO9KO:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1365/tmp/MarvinLogs/test_network_IQO9KO/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1365/tmp/MarvinLogs/test_network_IQO9KO/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1365/tmp/MarvinLogs/test_network_IQO9KO/runinfo.txt)
**`/tmp/MarvinLogs/test_vpc_routers_WQUMTV:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1365/tmp/MarvinLogs/test_vpc_routers_WQUMTV/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1365/tmp/MarvinLogs/test_vpc_routers_WQUMTV/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1365/tmp/MarvinLogs/test_vpc_routers_WQUMTV/runinfo.txt)
Uploads will be available until `2016-06-28 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58782564
--- Diff: utils/src/main/java/org/apache/cloudstack/utils/volume/VirtualMachineDiskInfo.java ---
@@ -39,4 +38,11 @@ public void setDiskDeviceBusName(String diskDeviceBusName) {
public void setDiskChain(String[] diskChain) {
this.diskChain = diskChain;
}
+
+ public String getControllerFromDeviceBusName() {
+ if (StringUtils.isEmpty(diskDeviceBusName) || !diskDeviceBusName.contains(":")) {
+ return null;
--- End diff --
Return an empty string rather than null to avoid potential NPEs.
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58780807
--- Diff: plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java ---
@@ -1363,24 +1363,17 @@ private Answer attachVolume(Command cmd, DiskTO disk, boolean isAttach, boolean
AttachAnswer answer = new AttachAnswer(disk);
if (isAttach) {
- String dataDiskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
- String rootDiskController = controllerInfo.get(VmDetailConstants.ROOK_DISK_CONTROLLER);
- DiskControllerType rootDiskControllerType = DiskControllerType.getType(rootDiskController);
-
- if (dataDiskController == null) {
- dataDiskController = getLegacyVmDataDiskController();
- } else if ((rootDiskControllerType == DiskControllerType.lsilogic) ||
- (rootDiskControllerType == DiskControllerType.lsisas1068) ||
- (rootDiskControllerType == DiskControllerType.pvscsi) ||
- (rootDiskControllerType == DiskControllerType.buslogic)) {
- //TODO: Support mix of SCSI controller types for single VM. If root disk is already over
- //a SCSI controller then use the same for data volume as well. This limitation will go once mix
- //of SCSI controller types for single VM.
- dataDiskController = rootDiskController;
- } else if (DiskControllerType.getType(dataDiskController) == DiskControllerType.osdefault) {
- dataDiskController = vmMo.getRecommendedDiskController(null);
+ String diskController = null;
+ if (controllerInfo != null) {
+ diskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
}
- vmMo.attachDisk(new String[] {datastoreVolumePath}, morDs, dataDiskController);
+ if (diskController == null) {
+ diskController = getLegacyVmDataDiskController();
--- End diff --
Why not set ``diskController`` to ``getLegacyVmDataDiskController`` when initializing the variable and avoid this additional conditional?
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58780261
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
+ GuestOsDescriptorType guestOsId = GuestOsDescriptorType.fromValue(cloudGuestOs);
+ if (guestOsId != null) {
+ s_logger.debug("Found guest OS mapping name for guest os: " + guestOs);
--- End diff --
I agree with the log message. However, I believe that adding the “isDebugEnabled()” would not be needed. The Logger.debug will check that, of course, it might be a little costly, but for legibility, I would let it as it is.
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58782481
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -5455,6 +5456,15 @@ private void encryptAndStorePassword(UserVmVO vm, String password) {
}
}
+ public void persistDeviceBusInfo(UserVmVO vm, String rootDiskController) {
+ String existingVmRootDiskController = vm.getDetail(VmDetailConstants.ROOT_DISK_CONTROLLER);
+ if (StringUtils.isEmpty(existingVmRootDiskController) && !StringUtils.isEmpty(rootDiskController)) {
+ vm.setDetail(VmDetailConstants.ROOT_DISK_CONTROLLER, rootDiskController);
+ _vmDao.saveDetails(vm);
+ s_logger.debug("Persisted device bus information rootDiskController=" + rootDiskController + " for vm: " + vm.getDisplayName());
--- End diff --
Wrap this ``debug`` call in a ``if (isDebugEnabled()`` block. Also, please add the vm id to the log message to assist with debugging.
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58803540
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
+ GuestOsDescriptorType guestOsId = GuestOsDescriptorType.fromValue(cloudGuestOs);
+ if (guestOsId != null) {
+ s_logger.debug("Found guest OS mapping name for guest os: " + guestOs);
--- End diff --
Ok, got it. I agree with you
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58784849
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
+ GuestOsDescriptorType guestOsId = GuestOsDescriptorType.fromValue(cloudGuestOs);
+ if (guestOsId != null) {
+ s_logger.debug("Found guest OS mapping name for guest os: " + guestOs);
--- End diff --
@rafaelweingartner the issue is the string concatenation that is performed before the method is invoked. It is best practice to wrap all debug and trace log calls in ``isDebugEnabled`` or ``isTraceEnabled`` to avoid the pressure on the constant pool and garbage collector from such string concatenation across the codebase.
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58778722
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
+ GuestOsDescriptorType guestOsId = GuestOsDescriptorType.fromValue(cloudGuestOs);
+ if (guestOsId != null) {
--- End diff --
@jburwell @rafaelweingartner if I do what you are proposing and if `guestOsId` here is null, I would need to return translateGuestOsIdentifier again
---
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: [4.7] vmware: improve support for disks
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r52853711
--- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/GuestOsDescriptorType.java ---
@@ -0,0 +1,46 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.hypervisor.vmware.mo;
+
+public enum GuestOsDescriptorType
+{
+ windows9Guest,
+ windows9_64Guest,
+ windows9Server64Guest,
+ rhel7Guest,
+ rhel7_64Guest,
+ debian7Guest,
+ debian7_64Guest,
+ debian8Guest,
+ debian8_64Guest,
+ coreos_64Guest,
+ darwin12_64Guest,
+ darwin13_64Guest,
+ darwin14_64Guest;
+
+ public static GuestOsDescriptorType fromValue(String value) {
+ if (value == null) {
+ return null;
+ }
+ GuestOsDescriptorType guestOsType = null;
+ try {
+ guestOsType = valueOf(value);
+ } catch (IllegalArgumentException e) {
+ }
+ return guestOsType;
+ }
--- End diff --
@bhaisaab Seems that the **com.cloud.utils.EnumUtils** class can do what you want (https://github.com/apache/cloudstack/blob/3ded3e90007d08fa98465f2b8c68b7fb075557c0/utils/src/main/java/com/cloud/utils/EnumUtils.java)
If you like of this approach, it would be something like `return EnumUtils.fromString(GuestOsDescriptorType.class, value);`
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58742032
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
--- End diff --
What about inverting the logic here? So, we can reduce one if-inception.
If(cloudGuestOs == null){
return translateGuestOsIdentifier(cpuArchitecture, guestOs, cloudGuestOs).value();
}
...the rest of the code that was initially inside the if.
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r51719809
--- Diff: core/src/org/apache/cloudstack/storage/command/AttachAnswer.java ---
@@ -34,6 +37,12 @@ public AttachAnswer(DiskTO disk) {
setDisk(disk);
}
+ public AttachAnswer(DiskTO disk, Map<String, String> diskDetails) {
--- End diff --
@GabrielBrascher Thanks. I started some small feature to be able to save/retrieve disk details, it is likely I dropped it but forgot to remove remove this -- I'll check and remove this.
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58904000
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
+ GuestOsDescriptorType guestOsId = GuestOsDescriptorType.fromValue(cloudGuestOs);
+ if (guestOsId != null) {
+ s_logger.debug("Found guest OS mapping name for guest os: " + guestOs);
--- End diff --
Removed this, bumping the sdk version to 6.0 we won't need this extra layer of translation
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58779767
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
+ GuestOsDescriptorType guestOsId = GuestOsDescriptorType.fromValue(cloudGuestOs);
+ if (guestOsId != null) {
--- End diff --
@bhaisaab , you are right! Sorry for my stubbornness, I missed that, my eyes are tired, I must be getting old :)
@jburwell, I agree with the use of StringUtils though.
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-213329597
@GabrielBrascher @swill @jburwell @rafaelweingartner please share your LGTM or comment what needs fixing, 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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58903873
--- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/GuestOsDescriptorType.java ---
@@ -0,0 +1,46 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.hypervisor.vmware.mo;
+
+public enum GuestOsDescriptorType
+{
+ windows9Guest,
+ windows9_64Guest,
+ windows9Server64Guest,
+ rhel7Guest,
+ rhel7_64Guest,
+ debian7Guest,
+ debian7_64Guest,
+ debian8Guest,
+ debian8_64Guest,
+ coreos_64Guest,
+ darwin12_64Guest,
+ darwin13_64Guest,
+ darwin14_64Guest;
+
+ public static GuestOsDescriptorType fromValue(String value) {
+ if (value == null) {
+ return null;
+ }
+ GuestOsDescriptorType guestOsType = null;
+ try {
+ guestOsType = valueOf(value);
+ } catch (IllegalArgumentException e) {
+ }
+ return guestOsType;
+ }
--- End diff --
Removed this, bumping the sdk version to 6.0 we won't need this extra layer of translation
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58763970
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
--- End diff --
Curious, why 2 or 3; not 10 or 100?
This method can be removed if PR https://github.com/apache/cloudstack/pull/1397 is accepted, which bumps vmware dependency version to sdk 6.0 which has the additional guest os identifier (i.e. no need to this extra translation).
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58762482
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -5455,6 +5456,15 @@ private void encryptAndStorePassword(UserVmVO vm, String password) {
}
}
+ public void persistDeviceBusInfo(UserVmVO vm, String rootDiskController) {
+ String existingVmRootDiskController = vm.getDetail(VmDetailConstants.ROOK_DISK_CONTROLLER);
--- End diff --
Fixed
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r52050632
--- Diff: core/src/org/apache/cloudstack/storage/command/AttachAnswer.java ---
@@ -34,6 +37,12 @@ public AttachAnswer(DiskTO disk) {
setDisk(disk);
}
+ public AttachAnswer(DiskTO disk, Map<String, String> diskDetails) {
--- End diff --
@GabrielBrascher thanks, fixed.
---
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: [4.7] vmware: improve support for disks
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216304460
Thanks @DaanHoogland -- though there is a way to run ESXi6 with vCenter 6 on KVM (http://rohit.yadav.xyz/logs/vmware-esxi-vcenter-on-kvm-cloudstack)
/cc @swill this is ready for merge now, 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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-211264892
@swill can we get this merged, while ShapeBlue has tested this internally if you test with your non-vmware CI that should confirm it does not break anything else; ideally we would need a vmware-based CI to confirm this but if you can consider our internal testing results. 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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r53929677
--- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/GuestOsDescriptorType.java ---
@@ -0,0 +1,46 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.hypervisor.vmware.mo;
+
+public enum GuestOsDescriptorType
+{
+ windows9Guest,
+ windows9_64Guest,
+ windows9Server64Guest,
+ rhel7Guest,
+ rhel7_64Guest,
+ debian7Guest,
+ debian7_64Guest,
+ debian8Guest,
+ debian8_64Guest,
+ coreos_64Guest,
+ darwin12_64Guest,
+ darwin13_64Guest,
+ darwin14_64Guest;
+
+ public static GuestOsDescriptorType fromValue(String value) {
+ if (value == null) {
+ return null;
+ }
+ GuestOsDescriptorType guestOsType = null;
+ try {
+ guestOsType = valueOf(value);
+ } catch (IllegalArgumentException e) {
+ }
+ return guestOsType;
+ }
--- End diff --
Thanks @GabrielBrascher I checked that with latest VMware SDK I could remove this wrapper. I've pushed another PR on master that bumps up the vmware sdk version to 6.0. I'll remove this soon, and resend for a review.
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58763620
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
--- End diff --
You've proposed a cosmetics fix that does not work either. If `guestOsId` is not null always (on line 4737) only then your suggestion makes sense; the return method is used as a catchall 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1365
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58768337
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1924,6 +1935,23 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) {
throw new InvalidParameterValueException("Cannot migrate ROOT volume of a stopped VM to a storage pool in a different VMware datacenter");
}
}
+
--- End diff --
I disagree with you here. Those huge (really huge) methods that ACS has are the rock in our shoes when writing unit and integration tests (not talking about functional tests here). It is almost impossible to write and maintain a test case for a method with 50, 100, 500+ lines.
Even though a method is not used in more than one place, I believe it is a good practice to break them into smaller ones whenever possible. That facilitates the writing of unit and integration tests cases.
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58767464
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
--- End diff --
@bhaisaab, I did not understand what you said.
The way it is coded right now when “cloudGuestOs == null ”, it will be executed the line 4740. Do you agree with that?
My suggestion is not purely cosmetics; it is to reduce code complexity. You can maintain that behavior while removing cyclomatic complexity and that I think it is worth.
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58785250
--- Diff: utils/src/test/java/org/apache/cloudstack/utils/volume/VirtualMachineDiskInfoTest.java ---
@@ -0,0 +1,46 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package org.apache.cloudstack.utils.volume;
+
+import com.google.gson.GsonBuilder;
+import com.google.gson.JsonParseException;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.runners.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class VirtualMachineDiskInfoTest {
+
+ @Test
+ public void testGetControllerFromDeviceBusName() {
+ VirtualMachineDiskInfo vmDiskInfo = new VirtualMachineDiskInfo();
+ vmDiskInfo.setDiskDeviceBusName("scsi0:0");
+ vmDiskInfo.setDiskChain(new String[]{"[somedatastore] i-3-VM-somePath/ROOT-1.vmdk"});
+ Assert.assertEquals(vmDiskInfo.getControllerFromDeviceBusName(), "scsi");
+ }
--- End diff --
Please add test cases to pass null and blank strings, as well as, a strings without a ":" into the method.
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58785773
--- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java ---
@@ -2118,8 +2118,12 @@ public int getScsiDiskControllerKey(String diskController) throws Exception {
List<VirtualDevice> devices = (List<VirtualDevice>)_context.getVimClient().
getDynamicProperty(_mor, "config.hardware.device");
+ String deviceList = "";
--- End diff --
Why not represent as an actual ``List`` and use ``Joiner`` to render it to a ``String``?
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r60035619
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,26 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
}
}
+ public void updateMissingRootDiskController(final VMInstanceVO vm, final String rootVolChainInfo) {
+ if (vm == null || !vm.getType().equals(VirtualMachine.Type.User) || Strings.isNullOrEmpty(rootVolChainInfo)) {
+ return;
+ }
+ String rootDiskController = null;
+ try {
+ final VirtualMachineDiskInfo infoInChain = _gson.fromJson(rootVolChainInfo, VirtualMachineDiskInfo.class);
+ if (infoInChain != null) {
--- End diff --
@jburwell I understand the idea of being defensive against NPE, but we should be pragmatic too. Otherwise, we will end up adding tons of null checks that are not needed.
---
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.
---
Re: [GitHub] cloudstack pull request: [4.7] vmware: improve support
for disks
Posted by Rafael Weingärtner <ra...@gmail.com>.
As I said I understand the check of "vm!= null". I was only referring to
the "vm.getType". I did not know that it could not be null.
I think what you were trying to say is that, once "vm" object is not null,
there is no way "getType" can return null, right? If that is a sure thing,
I am ok with the way it is right now.
On Mon, Apr 18, 2016 at 2:12 PM, bhaisaab <gi...@git.apache.org> wrote:
> Github user bhaisaab commented on a diff in the pull request:
>
> https://github.com/apache/cloudstack/pull/1365#discussion_r60096454
>
> --- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
> @@ -1835,6 +1847,26 @@ private Volume
> orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
> }
> }
>
> + public void updateMissingRootDiskController(final VMInstanceVO
> vm, final String rootVolChainInfo) {
> + if (vm == null ||
> !vm.getType().equals(VirtualMachine.Type.User) ||
> Strings.isNullOrEmpty(rootVolChainInfo)) {
> --- End diff --
>
> the order of evalution will be left to right -- the first case is vm
> == null; if this qualifies the if statement will branch into executing
> code; otherwise vm.getType() code will be executed (i.e. vm is not null);
> now here I think you're suggesting that perhaps vm.getType() may return
> null in which case we may have a NPE (unlikely based on db contraints (the
> type column should not be null), but I'll modify as suggested.
>
>
> ---
> 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.
> ---
>
--
Rafael Weingärtner
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r60096454
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,26 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
}
}
+ public void updateMissingRootDiskController(final VMInstanceVO vm, final String rootVolChainInfo) {
+ if (vm == null || !vm.getType().equals(VirtualMachine.Type.User) || Strings.isNullOrEmpty(rootVolChainInfo)) {
--- End diff --
the order of evalution will be left to right -- the first case is vm == null; if this qualifies the if statement will branch into executing code; otherwise vm.getType() code will be executed (i.e. vm is not null); now here I think you're suggesting that perhaps vm.getType() may return null in which case we may have a NPE (unlikely based on db contraints (the type column should not be null), but I'll modify as suggested.
---
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: [4.7] vmware: improve support for disks
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58781801
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1924,6 +1935,23 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) {
throw new InvalidParameterValueException("Cannot migrate ROOT volume of a stopped VM to a storage pool in a different VMware datacenter");
}
}
+
--- End diff --
@bhaisaab I have to agree with @rafaelweingartner. Create methods is also a great way to improve the code (no just a way to get a reusable code). It helps on keep it readable, ensure its correct implementation by implementing tests, and allows to create Javadoc. I think that it could improve this code too.
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58906077
--- Diff: server/test/com/cloud/vm/UserVmManagerTest.java ---
@@ -928,4 +928,13 @@ public void testUpdateVmNicIpFailure3() throws Exception {
CallContext.unregister();
}
}
+
+ @Test
+ public void testPersistDeviceBusInfo() {
+ when(_vmMock.getDetail(any(String.class))).thenReturn(null);
+ _userVmMgr.persistDeviceBusInfo(_vmMock, null);
+ verify(_vmDao, times(0)).saveDetails(any(UserVmVO.class));
+ _userVmMgr.persistDeviceBusInfo(_vmMock, "lsilogic");
+ verify(_vmDao, times(1)).saveDetails(any(UserVmVO.class));
--- End diff --
Fixed
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58906935
--- Diff: utils/src/test/java/org/apache/cloudstack/utils/volume/VirtualMachineDiskInfoTest.java ---
@@ -0,0 +1,46 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package org.apache.cloudstack.utils.volume;
+
+import com.google.gson.GsonBuilder;
+import com.google.gson.JsonParseException;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.runners.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class VirtualMachineDiskInfoTest {
+
+ @Test
+ public void testGetControllerFromDeviceBusName() {
+ VirtualMachineDiskInfo vmDiskInfo = new VirtualMachineDiskInfo();
+ vmDiskInfo.setDiskDeviceBusName("scsi0:0");
+ vmDiskInfo.setDiskChain(new String[]{"[somedatastore] i-3-VM-somePath/ROOT-1.vmdk"});
+ Assert.assertEquals(vmDiskInfo.getControllerFromDeviceBusName(), "scsi");
+ }
--- End diff --
Fixed
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58941182
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,26 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
}
}
+ public void updateMissingRootDiskController(final VMInstanceVO vm, final String rootVolChainInfo) {
+ if (vm == null || !vm.getType().equals(VirtualMachine.Type.User) || Strings.isNullOrEmpty(rootVolChainInfo)) {
+ return;
+ }
+ String rootDiskController = null;
+ try {
+ final VirtualMachineDiskInfo infoInChain = _gson.fromJson(rootVolChainInfo, VirtualMachineDiskInfo.class);
+ if (infoInChain != null) {
--- End diff --
Do we need this check ?
is it possible that `gson.fromJson` returns a null ?
I believe it does not return null values. If it cannot execute the unmarshall process, an exception happens, right?
---
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216442130
@rhtyd thanks for fixing the merge conflicts. \U0001f44d
@GabrielBrascher and @mike-tutkowski, I know you guys have spent some time with this file, would you guys mind reviewing the approach that @rhtyd has taken to resolve the merge conflict and make sure you guys are happy with it? I will also do a CI run against master before I do any additional merges to verify we are all good before we move on.
Thanks for the support on this guys...
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58904980
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -5455,6 +5456,15 @@ private void encryptAndStorePassword(UserVmVO vm, String password) {
}
}
+ public void persistDeviceBusInfo(UserVmVO vm, String rootDiskController) {
+ String existingVmRootDiskController = vm.getDetail(VmDetailConstants.ROOT_DISK_CONTROLLER);
+ if (StringUtils.isEmpty(existingVmRootDiskController) && !StringUtils.isEmpty(rootDiskController)) {
+ vm.setDetail(VmDetailConstants.ROOT_DISK_CONTROLLER, rootDiskController);
+ _vmDao.saveDetails(vm);
+ s_logger.debug("Persisted device bus information rootDiskController=" + rootDiskController + " for vm: " + vm.getDisplayName());
--- End diff --
Fixed
---
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r61807823
--- Diff: plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java ---
@@ -1363,24 +1364,15 @@ private Answer attachVolume(Command cmd, DiskTO disk, boolean isAttach, boolean
AttachAnswer answer = new AttachAnswer(disk);
if (isAttach) {
- String dataDiskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
- String rootDiskController = controllerInfo.get(VmDetailConstants.ROOK_DISK_CONTROLLER);
- DiskControllerType rootDiskControllerType = DiskControllerType.getType(rootDiskController);
-
- if (dataDiskController == null) {
- dataDiskController = getLegacyVmDataDiskController();
- } else if ((rootDiskControllerType == DiskControllerType.lsilogic) ||
- (rootDiskControllerType == DiskControllerType.lsisas1068) ||
- (rootDiskControllerType == DiskControllerType.pvscsi) ||
- (rootDiskControllerType == DiskControllerType.buslogic)) {
- //TODO: Support mix of SCSI controller types for single VM. If root disk is already over
- //a SCSI controller then use the same for data volume as well. This limitation will go once mix
- //of SCSI controller types for single VM.
- dataDiskController = rootDiskController;
- } else if (DiskControllerType.getType(dataDiskController) == DiskControllerType.osdefault) {
- dataDiskController = vmMo.getRecommendedDiskController(null);
+ String diskController = getLegacyVmDataDiskController();
--- End diff --
This has been merged into `4.7` and have forward merged into `4.8` without a problem.
I have merge conflicts trying to merge this from `4.8` into `master` though.
Here is the merge conflict:
```
AttachAnswer answer = new AttachAnswer(disk);
if (isAttach) {
<<<<<<< HEAD
String dataDiskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
String rootDiskController = controllerInfo.get(VmDetailConstants.ROOT_DISK_CONTROLLER);
DiskControllerType rootDiskControllerType = DiskControllerType.getType(rootDiskController);
if (dataDiskController == null) {
dataDiskController = getLegacyVmDataDiskController();
} else if ((rootDiskControllerType == DiskControllerType.lsilogic) ||
(rootDiskControllerType == DiskControllerType.lsisas1068) ||
(rootDiskControllerType == DiskControllerType.pvscsi) ||
(rootDiskControllerType == DiskControllerType.buslogic)) {
//TODO: Support mix of SCSI controller types for single VM. If root disk is already over
//a SCSI controller then use the same for data volume as well. This limitation will go once mix
//of SCSI controller types for single VM.
dataDiskController = rootDiskController;
} else if (DiskControllerType.getType(dataDiskController) == DiskControllerType.osdefault) {
dataDiskController = vmMo.getRecommendedDiskController(null);
=======
String diskController = getLegacyVmDataDiskController();
if (controllerInfo != null &&
!Strings.isNullOrEmpty(controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER))) {
diskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
>>>>>>> 4.8
}
if (DiskControllerType.getType(diskController) == DiskControllerType.osdefault) {
diskController = vmMo.getRecommendedDiskController(null);
}
vmMo.attachDisk(new String[] {datastoreVolumePath}, morDs, diskController);
} else {
```
Please advise...
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58903775
--- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java ---
@@ -2137,7 +2141,8 @@ public int getScsiDiskControllerKey(String diskController) throws Exception {
}
assert (false);
- throw new Exception(diskController + " Controller Not Found");
+ throw new Exception("Scsi disk controller of type " + diskController + " not found among configured devices:" + deviceList +
+ ". Unable to find and return scsi disk controller key.");
--- End diff --
Fixed
---
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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-211555695
@swill my final state is LGTM, will this be merged forward?
I believe @bhaisaab has mentioned that he has another PR that bumps VMware dependencies and that makes this PR obsolete, right? Or am my mistaking this with other 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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58929616
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,25 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
}
}
+ public void updateMissingRootDiskController(final VMInstanceVO vm, final String rootVolChainInfo) {
+ if (vm != null && vm.getType().equals(VirtualMachine.Type.User) && !Strings.isNullOrEmpty(rootVolChainInfo)) {
--- End diff --
very nice the extraction you did here.
What about the short-circuit for the condition at line 1851?
```
if (vm == null || !VirtualMachine.Type.User.equals(vm.getType()) || Strings.isNullOrEmpty(rootVolChainInfo)) {
return;
}
...
the rest of the code (lines 1852-1865)
...
```
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58903799
--- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java ---
@@ -2118,8 +2118,12 @@ public int getScsiDiskControllerKey(String diskController) throws Exception {
List<VirtualDevice> devices = (List<VirtualDevice>)_context.getVimClient().
getDynamicProperty(_mor, "config.hardware.device");
+ String deviceList = "";
--- End diff --
Removed, was not necessary
---
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216893135
Just as an update for everyone. CI ran against master with the merge conflict fix and everything went smoothly, so we are all set. Thanks for the help on this @rhtyd.
---
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: [4.7] vmware: improve support for disks
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216446492
@swill It seems reasonable to me. It doesn't look like equivalent functionality to what was previously there, but presumably that was why it was changed?
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58906447
--- Diff: utils/src/main/java/org/apache/cloudstack/utils/volume/VirtualMachineDiskInfo.java ---
@@ -39,4 +38,11 @@ public void setDiskDeviceBusName(String diskDeviceBusName) {
public void setDiskChain(String[] diskChain) {
this.diskChain = diskChain;
}
+
+ public String getControllerFromDeviceBusName() {
+ if (StringUtils.isEmpty(diskDeviceBusName) || !diskDeviceBusName.contains(":")) {
+ return null;
--- End diff --
In either case, we'll need to do a comparison to check if the string is null or empty. Returning an empty string might cover up potential functional issues, while NPEs are bad but can expose the issue without hiding 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-211359119
@swill , @bhaisaab
There are still questions to be addressed here. One example is the question at “VolumeApiServiceImpl” line1857. Another one is at “VolumeApiServiceImpl” line 1851.
There maybe more, some message were hidden due to code changes. This is a huge PR, which makes it pretty hard to review and then to check the changes that are being made (the ones that are consequences of code review). I think we need more work here.
---
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216541819
@mike-tutkowski, yes I think the change is expected. I just want some more eyes on this since it did not go through the usual CI and code review channels because we were fixing a merge conflict, so I figured it was prudent to do a bit of an 'after the fact' code review to make sure everyone is comfortable with the current state.
I meant to CC @nvazquez as well since I know he has spent time with this file 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 pull request: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58787329
--- Diff: server/test/com/cloud/vm/UserVmManagerTest.java ---
@@ -928,4 +928,13 @@ public void testUpdateVmNicIpFailure3() throws Exception {
CallContext.unregister();
}
}
+
+ @Test
+ public void testPersistDeviceBusInfo() {
+ when(_vmMock.getDetail(any(String.class))).thenReturn(null);
+ _userVmMgr.persistDeviceBusInfo(_vmMock, null);
+ verify(_vmDao, times(0)).saveDetails(any(UserVmVO.class));
+ _userVmMgr.persistDeviceBusInfo(_vmMock, "lsilogic");
+ verify(_vmDao, times(1)).saveDetails(any(UserVmVO.class));
--- End diff --
Why not split these two ``verify`` into two separate test methods to better express the scenario being tested? It would help with diagnosing the specific failure and make the code more maintainable.
Also, please add test cases for when the ``existingVmRootDiskController`` is empty and ``rootDiskController`` is empty.
---
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: [4.7] vmware: improve support for disks
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216906241
Thanks @swill for verifying
---
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: [4.7] vmware: improve support for disk co...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-176867564
@GabrielBrascher Thanks for reviewing. I'm avoiding comments in the code, methods are pretty small, self-explanatory. Tests have been added, along with the StringUtils fix. I've deduped a class defition as well. I've deduped a class definition as well. Regarding the "_" issue, it's a codebase wide issue. For now I'm sticking to how rest of the private vars are named.
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-207019005
@swill @rafaelweingartner @jburwell fixed outstanding issues and added more unit 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.
---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216221452
@agneya2001 @jburwell @DaanHoogland @sureshanaparti LGTM/review please, 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: [4.7] vmware: improve support for disks
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r60095758
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,26 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
}
}
+ public void updateMissingRootDiskController(final VMInstanceVO vm, final String rootVolChainInfo) {
+ if (vm == null || !vm.getType().equals(VirtualMachine.Type.User) || Strings.isNullOrEmpty(rootVolChainInfo)) {
--- End diff --
@bhaisaab I was no talking about removing the "vm != null", that check I know we need to execute.
I was only refering to the "vm.getType().equals(VirtualMachine.Type.User)", the "vm.getType" can return a null value.
Did you understand what I mean? Just inverting the comparison as I said before.
from
`vm.getType().equals(VirtualMachine.Type.User)`
to
`VirtualMachine.Type.User.equals(vm.getType())`
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58780935
--- Diff: plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java ---
@@ -1363,24 +1363,17 @@ private Answer attachVolume(Command cmd, DiskTO disk, boolean isAttach, boolean
AttachAnswer answer = new AttachAnswer(disk);
if (isAttach) {
- String dataDiskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
- String rootDiskController = controllerInfo.get(VmDetailConstants.ROOK_DISK_CONTROLLER);
- DiskControllerType rootDiskControllerType = DiskControllerType.getType(rootDiskController);
-
- if (dataDiskController == null) {
- dataDiskController = getLegacyVmDataDiskController();
- } else if ((rootDiskControllerType == DiskControllerType.lsilogic) ||
- (rootDiskControllerType == DiskControllerType.lsisas1068) ||
- (rootDiskControllerType == DiskControllerType.pvscsi) ||
- (rootDiskControllerType == DiskControllerType.buslogic)) {
- //TODO: Support mix of SCSI controller types for single VM. If root disk is already over
- //a SCSI controller then use the same for data volume as well. This limitation will go once mix
- //of SCSI controller types for single VM.
- dataDiskController = rootDiskController;
- } else if (DiskControllerType.getType(dataDiskController) == DiskControllerType.osdefault) {
- dataDiskController = vmMo.getRecommendedDiskController(null);
+ String diskController = null;
+ if (controllerInfo != null) {
+ diskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
}
- vmMo.attachDisk(new String[] {datastoreVolumePath}, morDs, dataDiskController);
+ if (diskController == null) {
+ diskController = getLegacyVmDataDiskController();
+ }
+ if (DiskControllerType.getType(diskController) == DiskControllerType.osdefault) {
+ diskController = vmMo.getRecommendedDiskController(null);
+ }
--- End diff --
Lines 1366-1375 seem ripe to extraction to a method with package default visibility and a set of unit 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.
---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58778373
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
+ GuestOsDescriptorType guestOsId = GuestOsDescriptorType.fromValue(cloudGuestOs);
+ if (guestOsId != null) {
+ s_logger.debug("Found guest OS mapping name for guest os: " + guestOs);
--- End diff --
This ``debug`` call should be wrapped in a ``if (s_logger.isDebugEnabled())`` block. Also, please include the value of ``cloudGuestOs`` in the log message to explain from which value it was mapped.
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58778117
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4729,6 +4729,17 @@ private VirtualMachineGuestOsIdentifier translateGuestOsIdentifier(String cpuArc
return VirtualMachineGuestOsIdentifier.OTHER_GUEST;
}
+ private String translateGuestOsIdentifierEx(String cpuArchitecture, String guestOs, String cloudGuestOs) {
+ if (cloudGuestOs != null) {
--- End diff --
agreed with @rafaelweingartner -- short circuiting out of the method greatly reduces complexity by reducing nesting.
Also, it seems like the check should be ``Strings.isBlankorNull`` to check for empty and, as well as, null values.
---
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: [4.7] vmware: improve support for disks
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r59486755
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,26 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
}
}
+ public void updateMissingRootDiskController(final VMInstanceVO vm, final String rootVolChainInfo) {
+ if (vm == null || !vm.getType().equals(VirtualMachine.Type.User) || Strings.isNullOrEmpty(rootVolChainInfo)) {
+ return;
+ }
+ String rootDiskController = null;
+ try {
+ final VirtualMachineDiskInfo infoInChain = _gson.fromJson(rootVolChainInfo, VirtualMachineDiskInfo.class);
+ if (infoInChain != null) {
--- End diff --
Since it is a third-party library, it never safe to make such an assumption. Better to be defensive in the event that the behavior of the library varies or changes in the future.
---
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-207022111
I can't specifically test the VMware functionality, but I can validate that it does not break anything else. I have added this to my queue for CI.
---
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: [4.7] vmware: improve support for disks
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-216248411
did a code walk through and saw no extravaganzas and some useful extractions and renamings. As for the functionality, I have no clue. I am not a vmware user, let alone expert but for what it's worth: 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 pull request: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r58763269
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1924,6 +1935,23 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) {
throw new InvalidParameterValueException("Cannot migrate ROOT volume of a stopped VM to a storage pool in a different VMware datacenter");
}
}
+
--- End diff --
Not necessary unless this method needs to be used somewhere else.
---
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: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r60188424
--- Diff: pom.xml ---
@@ -92,7 +92,7 @@
<cs.servlet.version>2.5</cs.servlet.version>
<cs.jstl.version>1.2</cs.jstl.version>
<cs.selenium.server.version>1.0-20081010.060147</cs.selenium.server.version>
- <cs.vmware.api.version>5.5</cs.vmware.api.version>
+ <cs.vmware.api.version>6.0</cs.vmware.api.version>
--- End diff --
@rafaelweingartner @swill I'll moved version change from the other PR here ^^
---
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: [4.7] vmware: improve support for disks
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-211554643
I think this one is pretty close. I want to run CI against it since there have been changes since the last CI run. I also need some LGTM code reviews. I don't see any yet even though there has been some pretty extensive review of this PR so far. Can @jburwell @GabrielBrascher and @rafaelweingartner revisit this PR and give me your final review of the current state. Thx...
---
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: [4.7] vmware: improve support for disks
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-199232343
### ACS CI BVT Run
**Sumarry:**
Build Number 114
Hypervisor xenserver
NetworkType Advanced
Passed=105
Failed=13
Skipped=4
**The follwing tests have known issues**
test_vpc_remote_access_vpn
test_vpc_site2site_vpn
test_01_test_vm_volume_snapshot
test_02_vpc_privategw_static_routes
test_03_rvpc_privategw_static_routes
test_04_change_offering_small
ContextSuite context=TestNiciraContoller>:setup
test_01_primary_storage_iscsi
test02_internallb_haproxy_stats_on_all_interfaces
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80
ContextSuite context=TestDeployVM>:setup
test_04_extract_template
test_04_extract_Iso
test_07_list_default_iso
test_dedicateGuestVlanRange
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* integration.smoke.test_guest_vlan_range.TestDedicateGuestVlanRange
* test_dedicateGuestVlanRange Failing since 4 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_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_vm_life_cycle.TestDeployVM
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 pull request: [4.7] vmware: improve support for disks
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r60095074
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,26 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
}
}
+ public void updateMissingRootDiskController(final VMInstanceVO vm, final String rootVolChainInfo) {
+ if (vm == null || !vm.getType().equals(VirtualMachine.Type.User) || Strings.isNullOrEmpty(rootVolChainInfo)) {
--- End diff --
@rafaelweingartner we'll get NPE anyway, we'll have to check vm != null as in what you've suggested we're doing equals against vm.getType() (therefore vm!=null check needed here).
---
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.
---