Koushik Any updates?
Animesh > -----Original Message----- > From: Animesh Chaturvedi [mailto:animesh.chaturv...@citrix.com] > Sent: Wednesday, February 13, 2013 7:08 PM > To: Vijayendra Bhamidipati; Frank Zhang; Kelven Yang > Cc: Wei Zhou; Koushik Das; cloudstack > Subject: RE: Review Request: Provide customizable instance names for guest > VMs in cloudstack > > 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+prov > > ide > > 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+prov > > ide > > 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 > > > > > >
