You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by priyankparihar <gi...@git.apache.org> on 2015/12/04 07:37:23 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
GitHub user priyankparihar opened a pull request:
https://github.com/apache/cloudstack/pull/1165
CLOUDSTACK-9104: VM naming convention in case vmware is used
I have reverted all the changes. Now functionality is same as it was in earlier version.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/priyankparihar/cloudstack CLOUDSTACK-9104
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1165.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 #1165
----
commit 2588925e9c82a9914b76e06204a04f4a5ff1ddb1
Author: Priyank Parihar <pr...@citrix.com>
Date: 2015-11-05T15:18:00Z
CLOUDSTACK-9104: VM naming convention in case vmware is used
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1165#issuecomment-164374286
@priyankparihar Fair enough, please see a code review comment. Once you fix the null check, I think your PR is good to go. Also, write a unit test to enforce the changes; i.e. the unit test should break in future if someone tries to change the VM naming convention in 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: CLOUDSTACK-9104: VM naming convention in ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1165#issuecomment-162413895
@DaanHoogland yes I can do that, I'm personally dealt with a lot of vmware/cloudstack naming issues. The syntax/pattern has changes several times in the past between 4.0, 4.2, 4.3 causing VM sync issues when users upgrade.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1165#discussion_r47469908
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1889,11 +1889,20 @@ int getReservedCpuMHZ(VirtualMachineTO vmSpec) {
// Pair<internal CS name, vCenter display name>
private Pair<String, String> composeVmNames(VirtualMachineTO vmSpec) {
- String vmInternalCSName = vmSpec.getName();
- String vmNameOnVcenter = vmSpec.getName();
- if (_instanceNameFlag && vmSpec.getHostName() != null) {
- vmNameOnVcenter = vmSpec.getHostName();
+
+ String vmInternalCSName = null;
+ String vmNameOnVcenter = null;
+ if(vmSpec != null)
+ {
+ vmInternalCSName = vmNameOnVcenter = vmSpec.getName();
+ if (_instanceNameFlag == true) {
+ String[] tokens = vmInternalCSName.split("-");
+ assert (tokens.length >= 3); // vmInternalCSName has format i-x-y-<instance.name>
+ vmNameOnVcenter = String.format("%s-%s-%s-%s", tokens[0], tokens[1], tokens[2], vmSpec.getHostName());
--- End diff --
Do something like;
String hostString = vmSpec.getHostName();
if (hostString == null) {
hostString = // get value from the vm suffix global setting (by default "VM");
}
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Posted by priyankparihar <gi...@git.apache.org>.
Github user priyankparihar commented on the pull request:
https://github.com/apache/cloudstack/pull/1165#issuecomment-164388358
@bhaisaab I think null check is not required. I have verified( because in case of null if automatically picks some random value). But if you still think it may break something then please notify.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Posted by priyankparihar <gi...@git.apache.org>.
Github user priyankparihar commented on the pull request:
https://github.com/apache/cloudstack/pull/1165#issuecomment-164348885
@bhaisaab
User with different account cannot create VMs with the same name, which was possible earlier (I am not sure in which CCP version). That time naming convention used was like this “I-<user-id>-<Display-Name>”
Currently if vm.instancename.flag is set to true the VM name will be exactly as display name given.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1165#discussion_r47469773
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1889,11 +1889,20 @@ int getReservedCpuMHZ(VirtualMachineTO vmSpec) {
// Pair<internal CS name, vCenter display name>
private Pair<String, String> composeVmNames(VirtualMachineTO vmSpec) {
- String vmInternalCSName = vmSpec.getName();
- String vmNameOnVcenter = vmSpec.getName();
- if (_instanceNameFlag && vmSpec.getHostName() != null) {
- vmNameOnVcenter = vmSpec.getHostName();
+
+ String vmInternalCSName = null;
+ String vmNameOnVcenter = null;
+ if(vmSpec != null)
+ {
+ vmInternalCSName = vmNameOnVcenter = vmSpec.getName();
+ if (_instanceNameFlag == true) {
+ String[] tokens = vmInternalCSName.split("-");
+ assert (tokens.length >= 3); // vmInternalCSName has format i-x-y-<instance.name>
+ vmNameOnVcenter = String.format("%s-%s-%s-%s", tokens[0], tokens[1], tokens[2], vmSpec.getHostName());
--- End diff --
@priyankparihar alright, even if we go with this change. If vmSpec.getHostName() was null then we've a problem with the vm name (if _instanceNameFlag, i.e. the global setting was true).
Once you put a null check, it should be good to go.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Posted by priyankparihar <gi...@git.apache.org>.
Github user priyankparihar closed the pull request at:
https://github.com/apache/cloudstack/pull/1165
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1165#issuecomment-164401688
@priyankparihar I won't be able to give a LGTM without the null check, split string related code and a unit test that breaks if someone tries to change the convention in 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: CLOUDSTACK-9104: VM naming convention in ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1165#issuecomment-164178352
@bhaisaab @priyankparihar what is the verdict on this change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1165#issuecomment-162341273
@priyankparihar I do not know any vmware users
@bhaisaab Do you have any insight on this change? or do you know any candidate reviewer for 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: CLOUDSTACK-9104: VM naming convention in ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1165#discussion_r46787171
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1889,11 +1889,20 @@ int getReservedCpuMHZ(VirtualMachineTO vmSpec) {
// Pair<internal CS name, vCenter display name>
private Pair<String, String> composeVmNames(VirtualMachineTO vmSpec) {
- String vmInternalCSName = vmSpec.getName();
- String vmNameOnVcenter = vmSpec.getName();
- if (_instanceNameFlag && vmSpec.getHostName() != null) {
- vmNameOnVcenter = vmSpec.getHostName();
+
+ String vmInternalCSName = null;
+ String vmNameOnVcenter = null;
+ if(vmSpec != null)
+ {
+ vmInternalCSName = vmNameOnVcenter = vmSpec.getName();
+ if (_instanceNameFlag == true) {
+ String[] tokens = vmInternalCSName.split("-");
+ assert (tokens.length >= 3); // vmInternalCSName has format i-x-y-<instance.name>
+ vmNameOnVcenter = String.format("%s-%s-%s-%s", tokens[0], tokens[1], tokens[2], vmSpec.getHostName());
--- End diff --
@priyankparihar can you comment on the issues and motivation around this? Do you think we have users who would want to keep hostname as VM names on vCenter side if the global setting is enabled (the _instanceNameFlag is true).
If this is not true, by default the name (as returned by the vmSpec) is like: i-[0-9]*-[0-9]*-VM. (The suffix "VM", is taken from another global setting where the default value is "VM").
---
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.
---