fix: check if virtualization is disabled and show warning #980

subhoghoshX · 2023-03-14T13:45:28Z

Machines: Show an alert when virtualization is disabled in BIOS/EFI

Cockpit-Machines displays a full-page alert when virtualization is disabled in the BIOS/EFI settings, preventing potential issues when running a VM.

Thanks to Subho Ghosh for this contribution.

jelly

Thanks for the PR, I didn't test it yet but here are some general improvements which can be made.

src/components/vms/hostvmslist.jsx

KKoukiou

After we agree on the fixes with the people I involved let's add a test for it. I will guide you in that.

KKoukiou · 2023-03-14T14:30:16Z

src/components/vms/hostvmslist.jsx

@@ -42,6 +42,7 @@ import { AggregateStatusCards } from "../aggregateStatusCards.jsx";
 import store from "../../store.js";

 import "./hostvmslist.scss";
+import { Alert, List, ListComponent, ListItem, OrderType } from '@patternfly/react-core';


please move a few lines up and change to use full path import (@patternfly/react-core/dist/esm/components .*), as we import other Patternfly components. There is a reason for that, check the git blame to figure out why if you want :)

Got it.

I wonder how reliable the bundler's tree shaking capabilities are.

This is the exact reason we moved to using tihs import style. Because importing from react-core would leave some imported but unused CSS in place.

KKoukiou · 2023-03-14T14:45:36Z

src/components/vms/hostvmslist.jsx

@@ -126,6 +141,13 @@ const HostVmsList = ({ vms, config, ui, storagePools, actions, networks, onAddEr
            <Page>
                <PageSection>
                    <Gallery className="ct-cards-grid" hasGutter>
+                        {!virtualizationEnabled && <Alert variant="warning" title="Warning: Virtualization is disabled in the firmware" component="h5">


replace title with: _("Hardware virtualization extensions are disabled in BIOS")

I am not sure if we want to add the steps for fixing this inline in cockpit. @garrett @martinpitt what do you think?
It's quite of an edge scenario, as all new servers will come with virtualization enabled. And it's a lot of text to translate.

Perhaps we just link to https://cockpit-project.org/faq.html#virtual-machines ? That has more detailed instructions. But I don't have a strong opinion about it, showing it inline is okay. It would be good to test this though, perhaps the test can rmmod kvm_intel or so?

Thanks for the feedbacks.

Linking it to the FAQ sounds better to me because it avoids repeating instructions already present in the FAQ.

I tested it by rebooting and toggling the settings in BIOS a few times. It was not ideal. I'll try rmmod kvm_intel . Thanks.

Implemented the suggested changes.

@martinpitt rmmod kvm_intel doesn't seem to work. At least, it doesn't have an immediate effect.

rmmod kvm_intel would't help as the kernel populates /proc/cpuinfo and kvm_intel is just a way to use the virtualisation extensions of the CPU, it could as well be Xen or Virtualbox.

src/components/vms/hostvmslist.jsx

subhoghoshX · 2023-03-15T14:59:56Z

Now it looks like this:

KKoukiou · 2023-03-20T14:27:20Z

src/components/vms/hostvmslist.jsx

@@ -126,6 +140,14 @@ const HostVmsList = ({ vms, config, ui, storagePools, actions, networks, onAddEr
            <Page>
                <PageSection>
                    <Gallery className="ct-cards-grid" hasGutter>
+                        {!virtualizationEnabled &&
+                        (<Alert variant="warning" title={_("Hardware virtualization extensions are disabled in BIOS")} component="h5">
+                            <Text>


@garrett can you help a bit with wording here?

I'd drop "in BIOS", as it's a bit confusing with UEFI machines. Also, one just needs one extension. Maybe just "Hardware virtualization is disabled", to keep this short and comprehensible?

Something like this?

Virtualization support is not available

Enable virtualization support in BIOS/EFI settings.

Instructions vary per manufacturer, but usually involves pressing a hot key such as ESC, F1, F12, or Del during boot. In the settings screen, look for "virtualization", "VM", "VMX', "SVM", "VTX", "VTD". It will be different on every computer. Enable any of these options. Consult your computer's manual for details.

However, I thought this would be a blocker for using Cockpit-Machines, so it would be an empty state pattern, not an alert at the top of the page?

I understand if you do have already-installed VMs if you're doing development, but someone not doing development and using Cockpit-Machines won't have this experience. If virtualization support is off in their BIOS/EFI settings, then they wouldn't have been able to create and install VMs. Therefore, it really blocks functionality.

Oh, I suggested an alert at the top of the page on the issue. 😅 #891 (comment)

Since a machine may have multiple VMs set up (due to the toggle changing, which happens during BIOS/EFI updates sometimes, or multiple attempts at installing VMs), I think the right place might be to have a big error at the top of the page (in an alert or banner) and still show the VMs otherwise. It should provide details about how to fix the error (rebooting, hitting a key, changing a value).

An alternate approach is to have an empty state, so it's in your face and have a bypass button, such as "Ignore" to be able to get to the machines to delete it.

WDYT?

martinpitt

Thanks!

martinpitt · 2023-03-22T08:58:14Z

src/components/vms/hostvmslist.jsx

+                                {_("For instructions on how to enable virtualization, please take a look ")}
+                                <a href='https://cockpit-project.org/faq.html#virtual-machines'>here</a>
+                                {_(" if your virtual machines fail to start or operate too slowly.")}


This doesn't work for translations -- you can't split a sentence in the middle and assume some ordering there, and that leading and trailling spaces are kept intact, and such. It is really difficult to translate a sentence with an embedded link. Please look at the "Read more" external link on the hwinfo page for how to model this.

martinpitt · 2023-03-22T08:59:00Z

src/components/vms/hostvmslist.jsx

+    useEffect(() => {
+        async function checkVirtualization() {
+            const cpuinfo = await cockpit.file("/proc/cpuinfo").read();
+            const flags = /vmx|svm|0xc0f/;


This magic number needs a comment with a link to some documentation or article which explains it.

martinpitt · 2023-03-22T09:00:21Z

src/components/vms/hostvmslist.jsx

@@ -126,6 +140,14 @@ const HostVmsList = ({ vms, config, ui, storagePools, actions, networks, onAddEr
            <Page>
                <PageSection>
                    <Gallery className="ct-cards-grid" hasGutter>
+                        {!virtualizationEnabled &&
+                        (<Alert variant="warning" title={_("Hardware virtualization extensions are disabled in BIOS")} component="h5">
+                            <Text>


I'd drop "in BIOS", as it's a bit confusing with UEFI machines. Also, one just needs one extension. Maybe just "Hardware virtualization is disabled", to keep this short and comprehensible?

martinpitt · 2023-03-22T09:02:33Z

src/components/vms/hostvmslist.jsx

+            const cpuinfo = await cockpit.file("/proc/cpuinfo").read();
+            const flags = /vmx|svm|0xc0f/;
+
+            setVirtualizationEnabled(flags.test(cpuinfo));


Oh, and this needs a fallback if flags is null -- that can happen if /proc/cpuinfo does not exist. Think containers, exotic architectures, etc.

garrett

I suggested test @ #980 (comment)

(Happy to have feedback on that.)

And I suggested changing this to an empty state pattern instead of an alert, despite what I said previously. #980 (comment)

I don't think most people would have made VMs prior, especially if they come across this warning first (and fix things)... Unless it's possible to actually use Cockpit-Machines without VM extensions (but then it would be exceedingly slow, right?)

If we're going to keep this as an alert (if Cockpit-Machines is actually useful without VM being enabled in the BIOS/EFI), it would be better to do so with an inline style, as it's technically an inline alert that's embedded in the page, not an alert that's popping up above something (and then going away). Making it inline is not only more proper, but it would also draw more attention to it due to the background color. It would need to have the card shadow around it to be consistent, however. This may require some custom CSS, such as adding a class to the element or using --pf-c-card--BoxShadow: var(--pf-global--BoxShadow--sm); (which is what pf-c-card uses).

But I think in almost every typical case, the empty state pattern (with a "Dismiss" or "Ignore" action) would probably be better here.

test/check-machines-virtualization

jelly · 2023-03-28T10:05:38Z

All packit tests seem to fail, can you retry to rebase your branch and re-push?

subhoghoshX · 2023-03-28T14:18:13Z

All packit tests seem to fail, can you retry to rebase your branch and re-push?

It appears the tests failed again :(

test/check-machines-virtualization

KKoukiou · 2023-03-28T20:38:28Z

src/app.jsx

+            <EmptyState>
+                <EmptyStateIcon icon={WrenchIcon} />
+                <Title headingLevel="h4" size="lg">
+                    {_("Virtualization support is not available")}


'Hardware virtualization is disabled' is more correct. is not available sounds like you can't undo it. THis was the suggestion from Martin and i like it.

Yes, it looks better. Changed it.

Hardware virtualization is disabled

Good call. I like that too.

KKoukiou · 2023-03-28T20:40:33Z

src/app.jsx

+                <EmptyStateBody>
+                    <Text>{_("Enable virtualization support in BIOS/EFI settings.")}</Text>
+                    <Text>
+                        {_(`Instructions vary per manufacturer, but usually involves pressing a hot key such as ESC, F1, F12, or Del during boot.


The ` will not let the string be translated. It also works with ".

If you need to use nested ", then you need to escape these like ".

The ` will not let the string be translated.

` is just like ", right?

Ah it includes the line break and the indentation spaces. Maybe that prevents it from translation?

Using " wouldn't allow the string to be broken in multiple lines. Escaping the line break \ would be same as `. Maybe I should use + to concatenate the broken strings.

KKoukiou · 2023-03-28T20:46:48Z

src/app.jsx

+                <EmptyStateBody>
+                    <Text>{_("Enable virtualization support in BIOS/EFI settings.")}</Text>
+                    <Text>
+                        {_(`Instructions vary per manufacturer, but usually involves pressing a hot key such as ESC, F1, F12, or Del during boot.


@garrett @martinpitt I feel that the suggestion text starting from 'Instructions' is too wordy, causing a lot of work for translations without even giving a very clear guidance.
I would suggest to skip it and just stick with the 'Enable virtualization support...`

KKoukiou · 2023-03-28T20:49:23Z

src/app.jsx

+                    </Text>
+                </EmptyStateBody>
+                <EmptyStatePrimary>
+                    <Button variant="link" onClick={() => setEmptyStateIgnored(true)}>{_("Ignore")}</Button>


Since we don't use any cookie and we store that in react state that 'ignore' wil last only till the user refreshes the page. I think it's fine as first pass, but we probably want to either use a cookie for remembering the option or reconsider the design and not hide the whole app behind this empty state. But fine for follow up.

We could also use localStorage.

We probably would want to have "Dismiss" instead of "Ignore", until we can remember it via LocalStorage.

Thanks for the feedback. I implemented saving the preference in localStorage.

test/check-machines-virtualization

src/app.jsx

garrett · 2023-03-29T12:37:06Z

I'm not sure how to test this without rebooting and messing with my EFI settings (and then rebooting back). Is there a better way?

Also, could you post an updated screenshot and/or small screencast video (audio wouldn't be necessary)?

jelly · 2023-03-29T13:12:51Z

I'm not sure how to test this without rebooting and messing with my EFI settings (and then rebooting back). Is there a better way?

Also, could you post an updated screenshot and/or small screencast video (audio wouldn't be necessary)?

This PR should have a test which mounts over /proc/cpuinfo and thus hiding the virtualization extensions. You should be able to do the same locally.

touch /tmp/cpuinfo.txt && mount --bind /tmp/cpuinfo.txt /proc/cpuinfo
umount /proc/cpuinfo

jelly · 2023-03-29T13:13:05Z

Sorry, misclick!

KKoukiou

Unfortunately packit test environment seems to not have virtualization enabled in bios? All test fail on the first page load where now this new screen appears. I suggest to just click ignore for all tests if the selector exists:

https://github.com/cockpit-project/cockpit-machines/blob/main/test/machineslib.py#L28

So write something like;

if b.is_visible("h4:contains('Hardware virtualization is disabled')"):
b.click('button:contains(Ignore)')

garrett

Three things (edit: actually four... look at the very bottom about the icon):

Ignore should be a button. I suggest a secondary button (even though it's the primary action... I'd suggest the implied actual primary action is to fix the BIOS, making this secondary).
- We should never use links or link styled buttons, except when they go somewhere (then it should be a link, not a button), are to downplay importance in an otherwise cluttered interface (such as in table rows, and they should be a link-styled button, unless they go somewhere as they'd be links instead), or are specifically the "Cancel" (link-styled) button on modals. ("Close" and other buttons should still look like buttons.) Everything else should be a button that looks like a button (either primary, secondary, warning, or danger).
For some odd reason, there's not enough vertical spacing between paragraphs when there are multiple paragraphs in the empty state. Are we using it incorrectly here, or is this a PF4 bug that we should workaround? (Perhaps something like .pf-c-empty-state p + p { margin-top: var(--pf-global--spacer--sm) }?
The text is far too wide.

This text should be narrower... ideally a around 60 characters or so, except on mobile. We can do that without breakpoints.

We'd want something like this:

.pf-c-empty-state.ct--empty-state--constrain {
  // Use a max width of 60 0-characters across and let it size for mobile too
   --pf-c-empty-state__content--MaxWidth: min(60ch, 100%);
}

(I'm not sure why there isn't a constraint by default, and even mystified that it doesn't properly auto-center by default. 🤷)

And we'd want to add that at a top-level for all of Cockpit in a file that other pages can use and other projects import, probably in lib/ somewhere.

The reason why I'm saying we should make this a custom ct-* class is that cockpit-project/cockpit#18592 is struggling with the same thing right now. We probably want this across all the empty states.

We could even use PF overrides and make this the default for Cockpit. If that's the case, we'd want to drop the class:

.pf-c-empty-state {
  // Use a max width of 60 0-characters across and let it size for mobile too
   --pf-c-empty-state__content--MaxWidth: min(60ch, 100%);
}

With this bit of CSS, it'd look like this:

We probably always want to constrain the width (when there's width that's greater than the width shown above, so I'm in favor of it if @KKoukiou agrees with that too.

If that's the case, we might want to make it a separate PR and rebase this and the other PR on top of it. Or keep it as a separate commit and cherry-pick it from one PR to the other (and have it first in each PRs... so whatever lands first wins and it would fall out from the other, yet both PRs would work as expected... practically the same thing, really).

When all the changes I outlined above are implemented, it would look like this:

Additional point:

We might also want to consider changing the icon from a wrench as well. Perhaps pf-icon-virtual-machine instead? For reference, it looks like this:

Co-authored-by: Garrett LeSage <garrettl@gmail.com>

All requested changed applied

garrett · 2023-04-05T12:22:15Z

Please update the screenshot; it's still using the old wrench icon and doesn't have a summary yet. Thanks!

subhoghoshX · 2023-04-06T04:58:46Z

Please update the screenshot; it's still using the old wrench icon and doesn't have a summary yet. Thanks!

Oops, my bad. I missed that point. I hope I'm not too late to make the change.

garrett · 2023-04-06T12:26:55Z

@subhoghoshX: No worries. It's easy to miss things like this sometimes. We've all done it from time to time. It's why we plan to have a day or two to get everything in and have follow-up quick fixes if we notice anything small like this.

KKoukiou self-requested a review March 14, 2023 14:05

jelly requested changes Mar 14, 2023

View reviewed changes

src/components/vms/hostvmslist.jsx Outdated Show resolved Hide resolved

src/components/vms/hostvmslist.jsx Outdated Show resolved Hide resolved

KKoukiou reviewed Mar 14, 2023

View reviewed changes

KKoukiou reviewed Mar 20, 2023

View reviewed changes

KKoukiou requested a review from garrett March 22, 2023 08:53

martinpitt previously requested changes Mar 22, 2023

View reviewed changes

martinpitt reviewed Mar 22, 2023

View reviewed changes

garrett requested changes Mar 22, 2023

View reviewed changes

jelly previously requested changes Mar 27, 2023

View reviewed changes

test/check-machines-virtualization Show resolved Hide resolved

subhoghoshX force-pushed the check-virtualization branch from d80acf5 to 5afb2c0 Compare March 28, 2023 10:50

KKoukiou requested changes Mar 28, 2023

View reviewed changes

garrett reviewed Mar 29, 2023

View reviewed changes

src/app.jsx Outdated Show resolved Hide resolved

jelly closed this Mar 29, 2023

jelly reopened this Mar 29, 2023

KKoukiou reviewed Mar 31, 2023

View reviewed changes

KKoukiou force-pushed the check-virtualization branch 2 times, most recently from baaed1f to de84a2a Compare April 3, 2023 12:15

garrett previously requested changes Apr 3, 2023

View reviewed changes

subhoghoshX added 6 commits April 4, 2023 11:33

fix: check if virtualization is disabled and show warning

a5e4a27

apply suggested changes

c52d3f7

fix: VMs listing not taking full width in small screens

b700adf

implement empty state

64efbc9

add tests

77d1735

remove console logs

b67a48f

subhoghoshX and others added 7 commits April 4, 2023 11:33

use cleanup

ea8b888

save empty state preference in localStorage

763bb3d

Update src/app.jsx

2e82d02

Co-authored-by: Garrett LeSage <garrettl@gmail.com>

fix: text translation

92ae1cd

fix: test text

ad3820b

Adjust tests to also run when HW virtualization is disabled

6ed222c

Make ignore button primary

d12f026

KKoukiou force-pushed the check-virtualization branch from de84a2a to 24623a7 Compare April 4, 2023 09:34

Apply garret's design suggestions

2e8929c

KKoukiou force-pushed the check-virtualization branch from 24623a7 to 2e8929c Compare April 4, 2023 11:01

KKoukiou approved these changes Apr 4, 2023

View reviewed changes

KKoukiou merged commit 4471e93 into cockpit-project:main Apr 4, 2023
8 checks passed

KKoukiou added the release-note label Apr 5, 2023

subhoghoshX mentioned this pull request Apr 6, 2023

Change empty state icon from Wrench to VirtualMachine #1011

Merged

martinpitt removed the release-note label Apr 18, 2023

fix: check if virtualization is disabled and show warning #980

fix: check if virtualization is disabled and show warning #980

Conversation

subhoghoshX commented Mar 14, 2023 • edited by garrett

Machines: Show an alert when virtualization is disabled in BIOS/EFI

jelly left a comment

Choose a reason for hiding this comment

KKoukiou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhoghoshX commented Mar 15, 2023

KKoukiou Mar 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrett left a comment

Choose a reason for hiding this comment

jelly commented Mar 28, 2023

subhoghoshX commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrett commented Mar 29, 2023

jelly commented Mar 29, 2023

jelly commented Mar 29, 2023

KKoukiou left a comment

Choose a reason for hiding this comment

garrett left a comment • edited

Choose a reason for hiding this comment

garrett commented Apr 5, 2023

subhoghoshX commented Apr 6, 2023

garrett commented Apr 6, 2023

subhoghoshX commented Mar 14, 2023 •

edited by garrett

KKoukiou Mar 20, 2023 •

edited

garrett left a comment •

edited