Koushik If you are satisfied with the patch please commit it
Animesh > -----Original Message----- > From: Venkata Siva Vijayendra Bhamidipati > [mailto:nore...@reviews.apache.org] On Behalf Of Venkata Siva Vijayendra > Bhamidipati > Sent: Thursday, February 07, 2013 7:27 PM > To: Frank Zhang; Kelven Yang > Cc: Wei Zhou; Koushik Das; Vijayendra Bhamidipati; cloudstack > Subject: Re: Review Request: Provide customizable instance names for guest > VMs in cloudstack > > > > > On Feb. 1, 2013, 9:50 a.m., Koushik Das wrote: > > > There is another config parameter instance.name based on which a fixed > suffix is appended to the internal name. What happens to this parameter in the > context of this fix? Also as part of this fix you are allowing to suffix the > display > name to internal name. Sometime back I saw a request in the cs-user list to > allow account name/id to be appended to the internal instance name. Would it > be possible to provide a list of well defined attributes (for e.g. display > name, > account etc.) + fixed suffix. > > > > Wei Zhou wrote: > > Is it neccessary to add i-<>-<> as a suffix to displayname or internal > > name? > > > > As a user, I totally agree with the requirements which were posted on > https://cwiki.apache.org/confluence/display/CLOUDSTACK/Allow+user+provide > d+hostname%2C+internal+VM+name+on+hypervisor+for+guest+VMs > > It seems that the result of this patch does not match the requirements. > > > > In setup/db/db/schema-40to410.sql, the field should be > 'vm.instancename.flag', not 'vm.hostname.flag'. > > > > Wei Zhou wrote: > > Does this patch apply on master? > > I fail to apply on master, and change source codes manually. > > I have the same testing result which is fine. > > > > a suggestion: > > can you change createDhcpEntryCommand(VirtualRouter, UserVm, NicVO, > Commands) in > com.cloud.network.router.VirtualNetworkApplianceManagerImpl, so that the > OS hostname of VM can be set to displayname? > > Hi Koushik, Wei, > > Thanks for the reviews. Wei, thanks a lot for catching the > vm.instancename.flag > error in the sql -- the diffs got mixed up and I will reupload the patch after > checking that it applies on top of tree (master). > > The instance.name parameter is a global setting which would apply to all guest > VMs. The requirement was to have a way to easily identify each guest VM > using a suffix that would match the display name. The vm.instancename.flag > will let the user set each guest VM's name differently. The instance.name flag > has been preserved to not change existing behavior. > > The i-<>-<> notation as a suffix is required for vmsync scripts to identify > and > differentiate between cloudstack and non cloudstack VMs, and also between > system and guest VMs. This is why we have an r/s/v/i-<>-<> naming convention > for all VMs on cloudstack. If this naming convention is not followed, scripts > implementing vmsync functionality break. > > These changes in the requirements were not captured in > https://cwiki.apache.org/confluence/display/CLOUDSTACK/Allow+user+provide > d+hostname%2C+internal+VM+name+on+hypervisor+for+guest+VMs > I will edit the contents of the link to reflect the above points. > > Also, it was determined that the hostname should not be changed as part of > this > feature. Will update the above link with this as well. > > Regarding generic attributes, yes indeed it should be possible to do that. We > could have different flags for the same, or do it in some other way. If the > community wants to have it, I could do it in another patch using a different > issue id. In this context, I have a question - I have set the maximum length > of > the VM internal instance name to 80 characters - if my memory is right, I > wasn't able to use more than 93 characters for ESX VMs. So I arbitrarily chose > 80 as the max length allowed. I do not know what the max lengths are for other > hypervisors - if anyone knows these limits for sure, and provide pointers for > the > same, it would be helpful - I will change the code accordingly. > > Finally, automated tests for the create VM API command already exist under > test/src/com/cloud/test/regression/. The above changes will be covered by > those tests. > > > Thanks, > Regards, > Vijay > > > - Venkata Siva Vijayendra > > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9202/#review16001 > ----------------------------------------------------------- > > > On Jan. 31, 2013, 7:21 p.m., Venkata Siva Vijayendra Bhamidipati wrote: > > > > ----------------------------------------------------------- > > This is an automatically generated e-mail. To reply, visit: > > https://reviews.apache.org/r/9202/ > > ----------------------------------------------------------- > > > > (Updated Jan. 31, 2013, 7:21 p.m.) > > > > > > Review request for cloudstack, Kelven Yang and Frank Zhang. > > > > > > Description > > ------- > > > > Patch to allow user to append display name to internal instance name of > guest VMs on cloudstack during guest VM creation. > > > > > > This addresses bug CS-778. > > > > > > Diffs > > ----- > > > > server/src/com/cloud/configuration/Config.java 4ae144e > > server/src/com/cloud/vm/UserVmManagerImpl.java ecf1242 > > server/src/com/cloud/vm/dao/VMInstanceDao.java 8b0a523 > > server/src/com/cloud/vm/dao/VMInstanceDaoImpl.java 85ad5d0 > > setup/db/db/schema-40to410.sql ed4946e > > > > Diff: https://reviews.apache.org/r/9202/diff/ > > > > > > Testing > > ------- > > > > Manual Testing: > > Set the global parameter vm.instancename.flag to true, restart mgmt server, > create a guest VM and provide a display Name value when doing so. The vm > created will have its internal name in the form of i-<>-<>-displayname. > > Set the global parameter vm.instancename.flag back to false, restart > > mgmt server, and create a guest VM providing the display Name. The vm > created will have the internal name in the form of i-<>-<> If no display name > is > provided during instance creation, the vm internal name will be in the form of > i-<>-<>. > > > > > > Thanks, > > > > Venkata Siva Vijayendra Bhamidipati > > > >
