machines: Add create vm dialog #7820
Conversation
You can get memory a few different ways. Can you use metrics to get the memory metadata. (http://cockpit-project.org/guide/latest/cockpit-metrics.html). You can also of course run and parse commands like free or vmstat or even read and parse /proc/meminfo.
|
I'd suggest adding the feature to lib/cockpit-components-select.jsx. I'd also suggest using the file selector here: https://github.com/cockpit-project/cockpit/blob/master/pkg/lib/cockpit-components-file-autocomplete.css |
It is dependent mostly on the version of qemu/emulated machine and OS. There would need to be some backend which gets these values for us, because we do not want to delegate this functionality to frontend. Maybe we could add it in the future, but I would just use the input field for now.
I will look at it again and see if the refactoring makes sense. But for now the StatelessSelect has different use case (delegates selected state to parent)
thanks, I will try that |
In the absence of something better is there a reason we can't set it based on the machine's available memory. Still seems better to me than an text field. |
I think it is mostly about the priorities what to do. I'd rather leave it until we can know the value correctly. |
|
|
||
|
|
||
| const _ = cockpit.gettext; | ||
| const OTHER_OS = "Other Os" |
There was a problem hiding this comment.
Capitalization error. Make this "Other OS"
| <tr> | ||
| <td className="top"> | ||
| <label className="control-label" for="location"> | ||
| {_("Installation Location")} |
There was a problem hiding this comment.
At first I thought this referred to what directory it would install the OS into.
I would make this "Installation Source" to be more consistent with the Fedora/RHEL installer.
See https://access.redhat.com/webassets/avalon/d/Red_Hat_Enterprise_Linux-7-Installation_Guide-en-US/images/2f2c6dc8424ea84cc783f1d26d645839/installationsource.png
|
Thanks for implementing this! It's looking good so far. ...But I do have a lot to say about things that need changing still. I'll keep this particular comment confined to the OS vendors & OS versions dropdowns, including the subtopics of widget implementation and dropdown content. Dropdown heightDropdowns extend past page height, as they do not take into account their starting position. A fix in /* Make the max-height based on the screen hight - offset from the top */
#vendor-select .dropdown-menu {
max-height: ~"calc(90vh - 50px - 8em)"
}
#system-select .dropdown-menu {
max-height: ~"calc(90vh - 50px - 10em)"
}Vendor dropdown order & groupingThe vendor list does not make sense.
Suggested list reorder
OS version lists are not ordered properlyScreenshots of dropdowns:
Suggestions for OS version listsAll the OS version lists have issues. This stems from the fact that the current ordering doesn't work at all. The common solution would be to sort them in release order, from newest to oldest (so the most likely ones would be at the top). Extremely old releases probably should be kicked out from the list — certainly this is the case for Red Hat Linux, but we might want to consider it for others as well. Generic versions without release versions (such as "Fedora") should be renamed to "Generic" and placed at the bottom of the lists with a divider line above them. For some OS versions that contain distinct products, we might want to consider grouping here as well. For example: Windows desktop versus Windows server. In the case of Windows specifically, this grouping would help make the releases make more sense, as release order would alternate between the distinct products. In this case, we probably would list Server versions before Desktop versions, because that's much more likely to be used in a server environment. (I would also suggest that we would only show server versions, but I do know people have odd setups where they use a desktop OS in a server environment for whatever reasons, so leaving it out entirely for Windows might not be a good idea.) |
|
For reference, here's what the dialog currently looks like on my computer:
Further questions on the current implementation in this PR:
|
Using the cli, if you sort them with |
|
For reference, I've ran the following command, had it parsed by splitting at
Google Docs output: https://docs.google.com/spreadsheets/d/1Vdfo4m2mXcmhgEh6XabBU2txr7TxxjE4dWD6lIJ776Y/edit?usp=sharing What I'm proposing is:
|
|
I'm not sure which languages are used, but I've included some sorting examples in JavaScript. Simple number-only sorting comparison: [95, 3.11, 5, 9.4, 3, 12, 10].sort(function(a, b) { return b - a; });Here's a more correct version that handles both strings and numbers: [95, 3.11, 5, 9.4, "foo", "bar", 3, 12, 10].sort(function(a, b) {
return (b + "").localeCompare(a, undefined, {
numeric: true,
sensitivity: 'base'
});
});(In most cases that have both numbers and strings, the strings are rolling releases and more current, so this works.) |
|
I fixed the strings and dropdown height. Thanks for the css @garrett
Sorry for the mistake - yes it is Installation Source. Btw should we have the switch as shown in your link? For URL and file or just one input with Upload button?
0 memory would not work
These are only suggestions for libvirt for optimizations. They do not have any other meaning. You can have different ISO than selected OS. I don't know If I misunderstood but I think these values are up to the user. |
So the selects stay same with the same meaning. And the vendor select will have family headers + better sorted right? I would group just the win* family otherwise you will not have many labels. Still not sure if msdos should belong there.
sounds good
the sorting in javascript should not be a problem. script itself is in shell
I would rather not introduce any rules based on specific distros/families. How do we decide which distro goes out and which does not? I do not think user will be bothered by that much if we implement sorting well. virt-install expects one of these values and I think the user should have the option to do so as well |
atiratree
force-pushed
the
virt-create
branch
from
b019bbc to
bba4cbc
Compare
me and @vojtechszocs refactored the selects. There are still two selects because they have different use cases, but they make much more sense now. Please let me know if I should keep the naming or make the first one Select and the second one StatefulSelect. I also used @mareklibra devices.listing from #5562 because I needed the functionality to open created vm in the list |
As an user, I would not expect any change happening on the system before pressing the Create button. This includes the iso image upload. For this reason, there's probably no need to render the |
|
there is still third option - to use ISO already present in the cockpit machine and locate it with already mentioned |
atiratree
force-pushed
the
virt-create
branch
3 times, most recently
from
a24c890 to
7a1779e
Compare
|
@garrett sorting fixed, can you please check if it is ok now? I also added SelectDivider and SelectHeader to the components |
atiratree
force-pushed
the
virt-create
branch
from
7a1779e to
4f7b638
Compare
HeadersThanks for adding headers! It's much better. However, it's hard to differentiate between groups, so I'd suggest adding in some CSS like such: #vendor-select .dropdown-header,
#vendor-select .divider {
margin-top: 2ex;
}Red HatThere's still a huge problem with Red Hat. It merges Red Hat Linux (really old predecessor to both Fedora and RHEL) and Red Hat Enterprise Linux, and worse yet, it sorts purely by version number. This means RHL 9 (from 2003) is sorted before Red Hat Enterprise Linux 7.4 (current release, a few months ago, in 2017)! These are two distinct OSes — one is ancient (discontinued after 2003) and the other is current and one of the main OSes most people will want to choose (especially in Cockpit). Here's the list:
Again, aside from nostalgia, I think we really should not even have the ability to have old, unsupported operating systems. We need a blocklist to make sure some OSes (especially with random Linux distros) do not show up. GNOME?Why is GNOME in the vendor dropdown? GNOME is a desktop, not a distro, and this create new desktop UI in Cockpit is for servers, not desktops. (Yeah, it's probably included in osinfo for running GNOME continuous via libvirt in other ways, but that's a completely different use case than what's needed in Cockpit.) WinWin should be Windows The sorting of Windows is still incorrect, similar to Red Hat. It needs to be sorted by release date, not by version. (It should be sorted by version after release date, for those OSes without release dates.) OS groupsThe OS groups (that is: the groups of vendors) should not be sorted by alphabetical order, but by most likely to be used. That would be Linux, Windows, then the rest in alphabetical order. (The items within these groups should be sorted alphabetically still.) Improper encodingThe distro names should be in UTF-8, but it seems there's improper encoding somewhere between the DB to the browser. You can see the result of improper encoding in Fedora release 19: "Schrödinger's Cat". In the OS dropdown, it's listed as "Schr?dinger's Cat" Old, insecure OSes are still includedOf course, some people may need to run old OSes behind a firewall (Windows XP, for example, or even DOS) — this is a bad practice, but sometimes necessary for some ancient corporate software. However, when it comes to Linux distributions, it's a bit different. Sure, people are sometimes lax in upgrading to a supported distro. The most recent version of ALTLinux is from 2012. The most recent version of Mandriva is from 2011. (Mandriva even went bankrupt in 2012 and was dissolved in 2015 — as of 2010, the distro was forked and lives on as Mageia, which is already included as a vendor choice.) Red Hat Linux (not Red Hat Enterprise Linux) ended in 2003 and was replaced with Red Hat Enterprise Linux and Fedora. Red Hat Linux is old and should be removed. The severely outdated distros should be removed outright. As far as cutoff points for current distributions, it would make sense to have something like > 1 or 2 years after the EOL date to have a very generous a buffer time. |
Grouping & sorting, continuedWhile playing with all the data, I figured out how it the releases should be be sorted into the two dropdowns and tested it out with a mockup @ http://garrettlesage.com/cockpit-mockweb/osinfo/ — however, this is generated in Jekyll and Liquid (the templating language that Jekyll uses) is really barebones, so it doesn't have a way to sort things properly, so when things are sorted by version, it's just plain wrong. I've commented in the source at https://github.com/garrett/cockpit-mockweb/blob/master/osinfo/index.md#group-by-os-name-more-or-less Basically, the way it works is this:
With all of the above, it shows a generous list for Linux distros (quite old releases) and happens to programatically filter out the distros I mentioned in the previous content without having to special case any distros. Please note:
Thanks! Hopefully all this makes sense. Feel free to ask me to clarify anything if I haven't phrased it properly and don't hesitate to contact me if you have any questions. |
|
Can you please rebase to resolve the conflicts? I'll trigger the tests then. |
atiratree
force-pushed
the
virt-create
branch
2 times, most recently
from
79ee5b2 to
9c23767
Compare
atiratree
force-pushed
the
virt-create
branch
2 times, most recently
from
c8c7469 to
feccf33
Compare
|
@martinpitt Do you know if anything is wrong with that?
|
|
@martinpitt |
|
@martinpitt thanks |
Heavily modified and finished by suomiy Closes cockpit-project#7820
martinpitt
force-pushed
the
virt-create
branch
from
feccf33 to
bd98748
Compare
|
This again collided with the polyfill fix (PR #8523), so I rebased to current master and also squashed the current commits together. This really needs to land at some point :-) Some questions for @suomiy :
Thanks! |
|
I just tested this with a Fedora Atomic Workstation ISO and the graphical VNC console, this is really cool! Do you also plan a mode where one doesn't supply an installer ISO, but can import an already existing .qcow2 image, and it then just creates the libvirt metadata? Also, this is green at last, so let's land this before the next conflict comes along :-) |
|
@martinpitt don't worry about the credits
Yup, that is a current one.
It is not planned at the moment. I guess it depends on the direction and priorities.
Great! :) |
The video is needed for the release notes, so it will reflect the current state of the release. |
|
I can make the video, but it will take me at least a week to get to that, if that is fine? Though, I would rather wait at least for #8441 |
|
@suomiy: We will release tomorrow (end of current sprint). I can make a current screenshot or two, and then we'll put the video into the next release notes, when this got some more cleanup? |
|
oki |
|
Screenshot from current master:
|
|
I made a series of screenshots for an internal presentation. Adding them here in case they get useful for anything else again:
|
|
@martinpitt Can I still make YT video on this? |
|
@suomiy: Of course :-) |
|
@martinpitt please give me feedback, if it should be done differently |
|
@suomiy: Really nice job with the video, also with the unintrusive cut over the installation. Great to see URL with install tree covered, I wasn't aware of that. That will use virt-install?
|
|
@martinpitt thanks |
|
@suomiy: This is great! I have a couple of questions with regard to some minor formatting things (as seen in the video; screengrabs included below):
And on this second screenshot:
|
|
@garrett thanks
you are right. It behaves weirdly and also depends on a browser
|
|
I can't reproduce the styling issue with When verifying, the VNC session "blinks" blank for a moment when installing Centos. But since this is when the VNC session is already established and VM is booting, I guess it's what the VNC server sends. Anyway, it disappears quickly, so I don't see it as a big issue. Please note, the component is about to be replaced by patternfly-react VncConsole - the higher-level styling will be changed slightly so potential issue reported here might be fixed. Recent screenshots from my testing:
|
|
Since this PR is merged, does't it make more sense to split new things off in new issues and PRs? |
Hello, I overtook #7138
creating vm works now but it was not tested properly yet
there are few changes in the proposed UI
also I had to extend Select in pkg/lib/cockpit-components-select.jsx because it does not have any way to select items programmatically. Please let me know if I should refactor it or just use my StatelessSelect
todo: