Code review comment for ~mpontillo/maas:add-deploy-kvm-ui--bug-1795013

Revision history for this message
Mike Pontillo (mpontillo) wrote :

Andres, your review brings up some good points, many of which are still undecided, I think.

== Interactions Between KVM Pod Installation and OS Selection ==

I didn't implement any special functionality in the UI for causing the "Install KVM" option to modify the OS selection. I think it should probably gray out the OS selection (and possibly select Bionic automatically) to make it clear that the user doesn't get a choice.

Instead, what happens in this branch is, if you select "Install KVM", on the back-end it forces the OS selection to become "ubuntu/bionic".

This needs to be revisited in a future branch to provide a complete solution, because when you specify install_kvm from the API, no such forced-OS-selection occurs. For supportability purposes, we should always deploy KVM with the latest LTS we've tested it on (in this case, bionic).

In summary, this branch is more of an initial cut than a polished version of this UI action. It just gets the basics done.

== KVM Pod Labels and Terminology ==

I kind of disagree with putting (pod) in parentheses. It's not a (pod), it's a Pod, according to our current UI. I proposed this branch without mentioning "pod" in anticipation of the idea that the "Pod" terminology might be going away in the near future. I feel that if it's not going away, we might as well say "KVM Pod".

The API/model terminology of "install_kvm" is similarly vague. To many people, including data center engineers, KVM might mean "Keyboard/Video/Mouse".

More generally, this needs to be more consistent in other places in the UI. In other branches I've seen proposed (and in the actual UI itself), we often say "Virsh pod", which is also weird. (Likely because the power type for a KVM pod is literally "virsh", but we can correct that in the UI, to some degree).

« Back to merge proposal