Hello Dimitris Sorry for the late reply.
2013/1/22 Dimitris Aragiorgis <[email protected]>: > * Michael Hanselmann <[email protected]> [2013-01-17 14:44:57 +0100]: >> For future reference: Send and discuss the design document *before* >> working on significant amounts of code. > > Well, I sent the whole patch set along with the design doc > for the following reasons: > > - To backup the design with a working implementation. I think it is > important to see how a completely new feature can be implemented. Unfortunately this is only useful so far. If there are underlying design issues you may end up re-writing a lot of code. In addition you might have to migrate your deployment to the version eventually implemented in the upstream code. Of course it's fine to have local changes. For bigger changes, however, it's usually better to discuss the design before. >> > +In order migration to succeed, the -incoming process should be started >> >> What is an “-incoming process”? > > This is the process started in the target node (during a migration). It > is the same command as the one in the source node, except for this extra > option. I will rephrase it accordingly. Thanks! >> > +concerning disks, because they are included in kvm_cmd entry. >> >> What is a “kvm_cmd entry”? I presume it's a reference to a variable, >> but please write the design document so it doesn't need detailed >> knowledge of implementation details. > > Correct. In this part it should be mentioned that the current format > includes a part of the final kvm command (kvm_cmd), a list of NICs', and > hvparams dict. I will also rephrase that. Try to keep the design separate from the actual implementation if and when possible. It should be rather high-level. >> > +instance, called ``dev_idxs``, holding the last id given to a device. >> Then, why don't you use UUIDs? We already use them for node groups and >> networks and the idea is to eventually use them for all resources >> (which disks are, for example). I know VirtualBox uses UUIDs to manage >> disks as well. > > It is very nice to hear that you are heading towards a model where > everything has its own UUID. However, KVM uses the `id` parameter > in netdev/drive/device options to name devices. Currently, ids are > limited to 32 characters, while UUIDs are 37. I'd consider this an implementation detail. “hv_kvm” can always use a map from in-configuration identifiers to something used by KVM. UUIDs are obviously opaque identifiers, while plain integers aren't. We have already seen this with job IDs, which originally were designed as a opaque identifier (though the actual implementation used an increasing number). Later people ended up using them as numbers (comparing them for ordering, etc.). If you intend to use “dev_idxs” as an identifier, you should use an opaque string. Another idea: If we know hypervisors can only use shorter identifiers, you could use another randomly generated string (or a part of a UUID). Assuming you use “config.TemporaryReservationManager.Generate”, all generated identifiers are unique within the cluster. > I will resend the design doc with the above changes included, once > we are done with the review of its remaining part too. Please do! Michael
