TestMachinesHostDevs.testHostDevAddSingleDevice flake #1292
Assignees
Labels
Comments
Closed
|
The same happens on rawhide. I reproduced this locally after (to pick up the new libvirt 9.9). I got a single failure of testHostDevAddSingleDevice out of about 10 runs, and no local failure of testHostDevAddSessionConnection. However, there are two different SELinux failures: I wonder why it's |
|
It's not just a simple re-render. I tried this interactively, and it does not make the dialog get hidden: --- src/components/vm/vmDetailsPage.jsx
+++ src/components/vm/vmDetailsPage.jsx
@@ -17,7 +17,7 @@
* along with Cockpit; If not, see <http://www.gnu.org/licenses/>.
*/
import PropTypes from 'prop-types';
-import React, { useEffect } from 'react';
+import React, { useState, useEffect } from 'react';
import cockpit from 'cockpit';
import { Breadcrumb, BreadcrumbItem } from "@patternfly/react-core/dist/esm/components/Breadcrumb";
@@ -63,6 +63,10 @@ export const VmDetailsPage = ({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
+ const [counter, setCounter] = useState(0);
+
+ window.setTimeout(() => { setCounter(counter + 1) }, 1000);
+
const vmActionsPageSection = (
<PageSection className="actions-pagesection" variant={PageSectionVariants.light} isWidthLimited>
<div className="vm-top-panel" data-vm-transient={!vm.persistent}>
@@ -163,7 +167,7 @@ export const VmDetailsPage = ({
{
id: `${vmId(vm.name)}-hostdevs`,
className: "hostdevs-card",
- title: _("Host devices"),
+ title: `Host devices ${counter}`,
actions: <VmHostDevActions vm={vm} />,
body: <VmHostDevCard vm={vm} nodeDevices={nodeDevices} />,
}
@@ -237,6 +241,8 @@ export const VmDetailsPage = ({
);
});
+ console.log("XXX VmDetailsPage render", counter);
+
return (
<WithDialogs>
<Page id="vm-details"Neither when I do the change "more properly" via the store: --- src/components/vm/vmDetailsPage.jsx
+++ src/components/vm/vmDetailsPage.jsx
@@ -44,6 +44,9 @@ import { VmSnapshotsCard, VmSnapshotsActions } from './snapshots/vmSnapshotsCard
import VmActions from './vmActions.jsx';
import { VmNeedsShutdown } from '../common/needsShutdown.jsx';
+import { updateVm } from '../../actions/store-actions.js';
+import store from "../../store.js";
+
import './vmDetailsPage.scss';
const _ = cockpit.gettext;
@@ -56,6 +59,7 @@ export const VmDetailsPage = ({
useEffect(() => {
// Anything in here is fired on component mount.
onUsageStartPolling();
+ window.setTimeout(() => { store.dispatch(updateVm({ tick: Date.now() }))}, 1000);
return () => {
// Anything in here is fired on component unmount.
onUsageStopPolling();@jelly how sure are you that this happens because of a re-rendering? |
|
I added logging to the close events: --- src/components/vm/hostdevs/hostDevAdd.jsx
+++ src/components/vm/hostdevs/hostDevAdd.jsx
@@ -225,12 +225,12 @@ const AddHostDev = ({ idPrefix, vm }) => {
isLoading={addHostDevInProgress}
isDisabled={addHostDevInProgress}
variant="primary"
- onClick={attach}>
+ onClick={() => { console.log("XXX closing HostDevAdd dialog via Attach button"); attach() }}>
{_("Add")}
</Button>
<Button id={`${idPrefix}-cancel`}
variant="link"
- onClick={Dialogs.close}>
+ onClick={() => { console.log("XXX closing HostDevAdd dialog via cancel button"); Dialogs.close() }}>
{_("Cancel")}
</Button>
</>
@@ -240,7 +240,7 @@ const AddHostDev = ({ idPrefix, vm }) => {
<Modal position="top"
variant="medium"
id={`${idPrefix}-dialog`}
- onClose={Dialogs.close}
+ onClose={() => { console.log("XXX closing HostDevAdd dialog via X button"); Dialogs.close() }}
title={_("Add host device")}
footer={footer}
isOpen>But when this happens, neither of the three run: With
With a normal interactive session, the X button is neither focused, nor does it even work -- clicking on it does nothing. Conclusion: I'm pretty sure that there is no "active" closing of the Dialog, as none of the event handlers are called. Marius says, the other way of hiding the dialog is if the component that contains the --- src/app.jsx
+++ src/app.jsx
@@ -323,9 +323,12 @@ class AppActive extends React.Component {
)
: undefined;
// If vm.isUi is set we show a dummy placeholder until libvirt gets a real domain object for newly created V
- const expandedContent = (vm.isUi && !vm.id)
- ? null
- : (
+ if (vm.isUi && !vm.id) {
+ console.log("XXX AppActive render; no vm.id, HIDING VmDetailsPage");
+ return null;
+ }
+ console.log("XXX AppActive render; rendering VmDetailsPage");
+ return (
<>
{vmNotifications}
<VmDetailsPage vm={vm} vms={combinedVms} config={config}
@@ -340,7 +343,6 @@ class AppActive extends React.Component {
/>
</>
);
- return expandedContent;
}
return (But the HIDING short-circuiting never happens, i.e. VmDetailsPage is always rendered. Nevertheless, I tried to move the
There is some contradicting documentation in https://github.com/cockpit-project/cockpit/blob/main/pkg/lib/dialogs.jsx : The example puts WithDialogs outside of
That's what c-machines does ATM. The docs also say
I tried that, but it doesn't fix the problem. It may still be a good idea to do that, though. |
|
I added some debug logging directly to WithDialogs: --- ../cockpit/main/pkg/lib/dialogs.jsx 2023-10-17 14:18:19.946822723 +0200
+++ pkg/lib/dialogs.jsx 2023-11-09 10:27:46.748451925 +0100
@@ -105,9 +105,11 @@
export const WithDialogs = ({ children }) => {
const [dialog, setDialog] = useState(null);
+ console.log("XXX WithDialogs render", JSON.stringify(dialog));
+
const Dialogs = {
- show: setDialog,
- close: () => setDialog(null),
+ show: v => { console.log("XXX WithDialogs.show", v); setDialog(v) },
+ close: () => { console.log("XXX WithDialogs.close"); setDialog(null) },
isActive: () => dialog !== null
};This should tell us whether the component survived ( This is a bit unexpected -- so something calls .close() (which then sets the state to And when adding logging to src/components/common/deleteResource.jsx, voilà: |
|
So after discovering this, the fix is actually simple and obvious: --- test/check-machines-hostdevs
+++ test/check-machines-hostdevs
@@ -226,6 +226,7 @@ class TestMachinesHostDevs(VirtualMachinesCase):
b.wait_in_text("#delete-resource-modal-vendor", self._vendor)
b.click('.pf-v5-c-modal-box__footer button:contains("Remove")')
+ b.wait_not_present("#delete-resource-modal")
b.wait_not_present(f"#vm-subVmTest1-hostdev-{self.vm_dev_id}-product")
# Check the error if selecting no devices when the VM is runningBut I wonder, can we make this less excruciating the next time? We never want to have a situation where we try to open a dialog when another one is already active. |
martinpitt
added a commit
to martinpitt/cockpit-machines
that referenced
this issue
Nov 9, 2023
Wait for the "Remove host device from VM?" dialog to disappear before opening the "Add host device" dialog in the next test step. Otherwise it often happend that the latter dialog just got opened, while deleteResource.jsx' dialog shutdown handler called `Dialogs.close()`, which would then close the new instead of the old dialog. This is purely a test race, as a human user/browser won't allow interaction with the main page as long as the modal dialog is still open. But our CDP driver allows that. Fixes cockpit-project#1292
martinpitt
added a commit
to martinpitt/cockpit
that referenced
this issue
Nov 9, 2023
With human user/browser interaction there can only ever be one Modal dialog at a time. However, our tests/CDP driver does not have that restriction, tests can happily click on main page buttons while a dialog is open. This can lead to situations where a test opens a dialog, and then opens a second one (which will overwrite `WithDialogs.Dialog.dialog` with the new component) without waiting for the first one to close. Then when the first dialog handler finally calls `.close()`, it will unexpectedly close the *second* dialog instead. These races are a pain to debug [1]. Make them obvious with an error message which shows both the old and the new dialog element. As our tests now fail on console.errors(), that will detect/prevent similar race conditions in our tests. [1] cockpit-project/cockpit-machines#1292
|
I have a dialogs.jsx improvement which will detect this situation. Unfortunately representing the component in our CDP log isn't very obvious: It works nicely in the interactive browser though, there I can clearly see which dialog is open. But with I sent cockpit-project/cockpit#19595 about this. |
martinpitt
added a commit
that referenced
this issue
Nov 9, 2023
Wait for the "Remove host device from VM?" dialog to disappear before opening the "Add host device" dialog in the next test step. Otherwise it often happend that the latter dialog just got opened, while deleteResource.jsx' dialog shutdown handler called `Dialogs.close()`, which would then close the new instead of the old dialog. This is purely a test race, as a human user/browser won't allow interaction with the main page as long as the modal dialog is still open. But our CDP driver allows that. Fixes #1292
martinpitt
added a commit
to martinpitt/cockpit
that referenced
this issue
Nov 9, 2023
With human user/browser interaction there can only ever be one Modal dialog at a time. However, our tests/CDP driver does not have that restriction, tests can happily click on main page buttons while a dialog is open. This can lead to situations where a test opens a dialog, and then opens a second one (which will overwrite `WithDialogs.Dialog.dialog` with the new component) without waiting for the first one to close. Then when the first dialog handler finally calls `.close()`, it will unexpectedly close the *second* dialog instead. These races are a pain to debug [1]. Make them obvious with an error message which shows both the old and the new dialog element. As our tests now fail on console.errors(), that will detect/prevent similar race conditions in our tests. [1] cockpit-project/cockpit-machines#1292
martinpitt
added a commit
to martinpitt/cockpit
that referenced
this issue
Nov 10, 2023
With human user/browser interaction there can only ever be one Modal dialog at a time. However, our tests/CDP driver does not have that restriction, tests can happily click on main page buttons while a dialog is open. This can lead to situations where a test opens a dialog, and then opens a second one (which will overwrite `WithDialogs.Dialog.dialog` with the new component) without waiting for the first one to close. Then when the first dialog handler finally calls `.close()`, it will unexpectedly close the *second* dialog instead. These races are a pain to debug [1]. Make them obvious with an error message which shows both the old and the new dialog element. As our tests now fail on console.errors(), that will detect/prevent similar race conditions in our tests. [1] cockpit-project/cockpit-machines#1292
martinpitt
added a commit
to cockpit-project/cockpit
that referenced
this issue
Nov 10, 2023
With human user/browser interaction there can only ever be one Modal dialog at a time. However, our tests/CDP driver does not have that restriction, tests can happily click on main page buttons while a dialog is open. This can lead to situations where a test opens a dialog, and then opens a second one (which will overwrite `WithDialogs.Dialog.dialog` with the new component) without waiting for the first one to close. Then when the first dialog handler finally calls `.close()`, it will unexpectedly close the *second* dialog instead. These races are a pain to debug [1]. Make them obvious with an error message which shows both the old and the new dialog element. As our tests now fail on console.errors(), that will detect/prevent similar race conditions in our tests. [1] cockpit-project/cockpit-machines#1292
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Flake logs: https://cockpit-logs.us-east-1.linodeobjects.com/pull-5481-20231104-052435-87e823ba-arch-cockpit-project-cockpit-machines/log.html
What seems to happen is that the
Add host devicedialog is closed while we have it open, probably due to a re-render, after adding some debug logs:The text was updated successfully, but these errors were encountered: