From c01727b0ad396e0ffc471a3b5574455c78d0b1d2 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Wed, 5 Jul 2023 18:32:43 +0200 Subject: [PATCH 1/4] Avoid API queries for some events These events change the state in a predictable way. Avoid the expensive updateContainer() call for these, to avoid multiple calls overlapping each other. See https://github.com/containers/podman/issues/19124 The exact `StartedAt` value for the "start" event is unfortunately not part of the event data, but we can approximate it very well with the event time stamp. This doesn't have to be 100% correct: The only place where we use that is the restart detection in testLifecycleOperations. Adjust that to not require the data-started-at to be identical to the CLI output, just that it changes. --- src/app.jsx | 54 +++++++++++++++++++++++++++++++++++++----- test/check-application | 6 ++--- 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/app.jsx b/src/app.jsx index 13c3353e8..0863a7962 100644 --- a/src/app.jsx +++ b/src/app.jsx @@ -33,6 +33,7 @@ import ContainerHeader from './ContainerHeader.jsx'; import Containers from './Containers.jsx'; import Images from './Images.jsx'; import * as client from './client.js'; +import { debug } from './util.js'; const _ = cockpit.gettext; @@ -276,6 +277,27 @@ class Application extends React.Component { return new_wait; } + /* lightweight version of updateContainer, but still needs the same serialization */ + updateContainerState(id, system, props) { + const idx = id + system.toString(); + + if (!this.state.containers[idx]) + // we got an event for a container which we didn't introspect yet + return this.updateContainer(id, system); + + const wait = this.pendingUpdateContainer[idx] ?? Promise.resolve(); + + wait.then(() => this.setState(prevState => { + const containers = { ...prevState.containers }; + const newContainer = { ...containers[idx], State: { ...containers[idx].State, ...props } }; + containers[idx] = newContainer; + debug(system, "updateContainerState", idx, "to", JSON.stringify(newContainer.State)); + return { containers }; + })); + this.pendingUpdateContainer[idx] = wait; + wait.finally(() => { delete this.pendingUpdateContainer[idx] }); + } + updateImage(id, system) { client.getImages(system, id) .then(reply => { @@ -343,6 +365,15 @@ class Application extends React.Component { case 'unmount': case 'wait': break; + + /* The following events just change the State properties */ + case 'pause': + this.updateContainerState(id, system, { Status: "paused" }); + break; + case 'unpause': + this.updateContainerState(id, system, { Status: "running" }); + break; + /* The following events need only to update the Container list * We do get the container affected in the event object but for * now we 'll do a batch update @@ -353,17 +384,28 @@ class Application extends React.Component { (event.Actor.Attributes.podId ? this.updatePod(event.Actor.Attributes.podId, system) : this.updatePods(system) - ).then(() => this.updateContainer(id, system, event)); + ).then(() => this.updateContainerState( + id, system, + { + Status: "running", + StartedAt: new Date(event.timeNano / 1000000).toISOString() + })); break; - case 'checkpoint': + + case 'died': + case 'stop': + this.updateContainerState(id, system, { Status: "stopped" }); + break; + case 'cleanup': + // don't bother with setting FinishedAt, we don't use it anywhere + this.updateContainerState(id, system, { Status: "exited" }); + break; + + case 'checkpoint': case 'create': - case 'died': case 'exec_died': // HACK: pick up health check runs, see https://github.com/containers/podman/issues/19237 - case 'pause': case 'restore': - case 'stop': - case 'unpause': case 'rename': // rename event is available starting podman v4.1; until then the container does not get refreshed after renaming this.updateContainer(id, system, event); break; diff --git a/test/check-application b/test/check-application index 66f1737e3..f871bafff 100755 --- a/test/check-application +++ b/test/check-application @@ -1228,10 +1228,10 @@ class TestApplication(testlib.MachineCase): # Restart the container; there is no steady state change in the visible UI, so look for # a changed data-started-at attribute old_start = self.getStartTime("swamped-crate", auth=auth) - b.wait_in_text(f'#containers-containers tr[data-started-at="{old_start}"]', "swamped-crate") + old_start_dom = b.attr("#containers-containers tr[data-row-id", "data-started-at") self.performContainerAction(IMG_BUSYBOX, "Force restart") - new_start = self.waitRestart("swamped-crate", old_start, auth=auth) - b.wait_in_text(f'#containers-containers tr[data-started-at="{new_start}"]', "swamped-crate") + self.waitRestart("swamped-crate", old_start, auth=auth) + b.wait_not_attr("#containers-containers tr[data-row-id", "data-started-at", old_start_dom) self.waitContainer(container_sha, auth, name='swamped-crate', image=IMG_BUSYBOX, state='Running') self.waitContainerRow(IMG_BUSYBOX) From faa78feeae6c87d49ec1df16febd9ef5e6c81bba Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Fri, 14 Jul 2023 12:01:17 +0200 Subject: [PATCH 2/4] Revert "Avoid API queries for some events" This reverts commit d3cb07dff88663bd47c07aacdd5e3eb32bbf938f. Let's debug healthcheck first --- src/app.jsx | 54 +++++------------------------------------- test/check-application | 6 ++--- 2 files changed, 9 insertions(+), 51 deletions(-) diff --git a/src/app.jsx b/src/app.jsx index 0863a7962..13c3353e8 100644 --- a/src/app.jsx +++ b/src/app.jsx @@ -33,7 +33,6 @@ import ContainerHeader from './ContainerHeader.jsx'; import Containers from './Containers.jsx'; import Images from './Images.jsx'; import * as client from './client.js'; -import { debug } from './util.js'; const _ = cockpit.gettext; @@ -277,27 +276,6 @@ class Application extends React.Component { return new_wait; } - /* lightweight version of updateContainer, but still needs the same serialization */ - updateContainerState(id, system, props) { - const idx = id + system.toString(); - - if (!this.state.containers[idx]) - // we got an event for a container which we didn't introspect yet - return this.updateContainer(id, system); - - const wait = this.pendingUpdateContainer[idx] ?? Promise.resolve(); - - wait.then(() => this.setState(prevState => { - const containers = { ...prevState.containers }; - const newContainer = { ...containers[idx], State: { ...containers[idx].State, ...props } }; - containers[idx] = newContainer; - debug(system, "updateContainerState", idx, "to", JSON.stringify(newContainer.State)); - return { containers }; - })); - this.pendingUpdateContainer[idx] = wait; - wait.finally(() => { delete this.pendingUpdateContainer[idx] }); - } - updateImage(id, system) { client.getImages(system, id) .then(reply => { @@ -365,15 +343,6 @@ class Application extends React.Component { case 'unmount': case 'wait': break; - - /* The following events just change the State properties */ - case 'pause': - this.updateContainerState(id, system, { Status: "paused" }); - break; - case 'unpause': - this.updateContainerState(id, system, { Status: "running" }); - break; - /* The following events need only to update the Container list * We do get the container affected in the event object but for * now we 'll do a batch update @@ -384,28 +353,17 @@ class Application extends React.Component { (event.Actor.Attributes.podId ? this.updatePod(event.Actor.Attributes.podId, system) : this.updatePods(system) - ).then(() => this.updateContainerState( - id, system, - { - Status: "running", - StartedAt: new Date(event.timeNano / 1000000).toISOString() - })); + ).then(() => this.updateContainer(id, system, event)); break; - - case 'died': - case 'stop': - this.updateContainerState(id, system, { Status: "stopped" }); - break; - - case 'cleanup': - // don't bother with setting FinishedAt, we don't use it anywhere - this.updateContainerState(id, system, { Status: "exited" }); - break; - case 'checkpoint': + case 'cleanup': case 'create': + case 'died': case 'exec_died': // HACK: pick up health check runs, see https://github.com/containers/podman/issues/19237 + case 'pause': case 'restore': + case 'stop': + case 'unpause': case 'rename': // rename event is available starting podman v4.1; until then the container does not get refreshed after renaming this.updateContainer(id, system, event); break; diff --git a/test/check-application b/test/check-application index f871bafff..66f1737e3 100755 --- a/test/check-application +++ b/test/check-application @@ -1228,10 +1228,10 @@ class TestApplication(testlib.MachineCase): # Restart the container; there is no steady state change in the visible UI, so look for # a changed data-started-at attribute old_start = self.getStartTime("swamped-crate", auth=auth) - old_start_dom = b.attr("#containers-containers tr[data-row-id", "data-started-at") + b.wait_in_text(f'#containers-containers tr[data-started-at="{old_start}"]', "swamped-crate") self.performContainerAction(IMG_BUSYBOX, "Force restart") - self.waitRestart("swamped-crate", old_start, auth=auth) - b.wait_not_attr("#containers-containers tr[data-row-id", "data-started-at", old_start_dom) + new_start = self.waitRestart("swamped-crate", old_start, auth=auth) + b.wait_in_text(f'#containers-containers tr[data-started-at="{new_start}"]', "swamped-crate") self.waitContainer(container_sha, auth, name='swamped-crate', image=IMG_BUSYBOX, state='Running') self.waitContainerRow(IMG_BUSYBOX) From 168dfe74566347afe12a35c0991517c38c817dbd Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Mon, 3 Jul 2023 10:21:06 +0200 Subject: [PATCH 3/4] XXX debug/amplify tests --- packit.yaml | 22 --------------- test/check-application | 63 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 22 deletions(-) diff --git a/packit.yaml b/packit.yaml index e7c9c9602..1af4f24de 100644 --- a/packit.yaml +++ b/packit.yaml @@ -16,28 +16,6 @@ srpm_build_deps: - npm jobs: - - job: copr_build - trigger: pull_request - targets: - - fedora-37 - - fedora-38 - - fedora-latest-aarch64 - - fedora-development - - centos-stream-9-x86_64 - - centos-stream-9-aarch64 - - centos-stream-8-x86_64 - - - job: tests - trigger: pull_request - targets: - - fedora-37 - - fedora-38 - - fedora-latest-aarch64 - - fedora-development - - centos-stream-9-x86_64 - - centos-stream-9-aarch64 - - centos-stream-8-x86_64 - - job: copr_build trigger: release owner: "@cockpit" diff --git a/test/check-application b/test/check-application index 66f1737e3..4dc4392aa 100755 --- a/test/check-application +++ b/test/check-application @@ -401,6 +401,18 @@ class TestApplication(testlib.MachineCase): b.click("#table-pod-3-title .pod-details-volumes-btn") b.wait_in_text(".pf-v5-c-popover__content", "/tmp ↔ /app") + def testPods2(self): + self.testPods() + + def testPods3(self): + self.testPods() + + def testPods4(self): + self.testPods() + + def testPods5(self): + self.testPods() + def testBasicSystem(self): self._testBasic(True) @@ -559,6 +571,8 @@ class TestApplication(testlib.MachineCase): expected_ws += "ws" self.login(auth) + b.eval_js('window.debugging = "podman"') + b.cdp.trace = True # Check all containers if auth: @@ -1510,6 +1524,15 @@ class TestApplication(testlib.MachineCase): def testCreateContainerSystem(self): self._testCreateContainer(True) + def testCreateContainerSystem2(self): + self._testCreateContainer(True) + + def testCreateContainerSystem3(self): + self._testCreateContainer(True) + + def testCreateContainerSystem4(self): + self._testCreateContainer(True) + def testCreateContainerUser(self): self._testCreateContainer(False) @@ -1537,6 +1560,9 @@ class TestApplication(testlib.MachineCase): b = self.browser container_name = "busybox-downloaded" + b.eval_js('window.debugging = "podman"') + b.cdp.trace = True + b.click("#containers-containers button.pf-v5-c-button.pf-m-primary") b.set_input_text("#run-image-dialog-name", container_name) @@ -1634,6 +1660,8 @@ class TestApplication(testlib.MachineCase): self.execute(False, "podman rmi --all") self.login(auth) + b.eval_js('window.debugging = "podman"') + b.cdp.trace = True b.click("#containers-images button.pf-v5-c-expandable-section__toggle") @@ -2278,6 +2306,8 @@ class TestApplication(testlib.MachineCase): self.execute(False, f"podman rmi {IMG_BUSYBOX}") self.login(auth) + b.eval_js('window.debugging = "podman"') + b.cdp.trace = True b.click("#containers-images button.pf-v5-c-expandable-section__toggle") @@ -2399,9 +2429,27 @@ class TestApplication(testlib.MachineCase): def testHealthcheckSystem(self): self._testHealthcheck(True) + def testHealthcheckSystem2(self): + self._testHealthcheck(True) + + def testHealthcheckSystem3(self): + self._testHealthcheck(True) + + def testHealthcheckSystem4(self): + self._testHealthcheck(True) + def testHealthcheckUser(self): self._testHealthcheck(False) + def testHealthcheckUser2(self): + self._testHealthcheck(False) + + def testHealthcheckUser3(self): + self._testHealthcheck(False) + + def testHealthcheckUser4(self): + self._testHealthcheck(False) + # Ubuntu 2204 lacks user systemd units # https://github.com/containers/podman/commit/9312d458b4254b48e331d1ae40cb2f6d0fec9bd0 @testlib.skipImage("podman-restart not available for user", "ubuntu-2204") @@ -2578,6 +2626,18 @@ class TestApplication(testlib.MachineCase): def testCreatePodUser(self): self._createPod(False) + def testCreatePodUser2(self): + self._createPod(False) + + def testCreatePodUser3(self): + self._createPod(False) + + def testCreatePodUser4(self): + self._createPod(False) + + def testCreatePodUser5(self): + self._createPod(False) + def _createPod(self, auth): b = self.browser m = self.machine @@ -2625,6 +2685,9 @@ class TestApplication(testlib.MachineCase): b.set_file_autocomplete_val("#create-pod-dialog-volume-1 .pf-v5-c-select", rwdir) b.set_input_text('#create-pod-dialog-volume-1-container-path', '/tmp/rw') + b.eval_js('window.debugging = "podman"') + b.cdp.trace = True + b.click("#create-pod-create-btn") b.set_val("#containers-containers-filter", "all") self.waitPodContainer(pod_name, []) From 35512e6bec5ed3cf00af786234570e48ccb85934 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Fri, 14 Jul 2023 17:15:19 +0200 Subject: [PATCH 4/4] test: Add hack to avoid losing focus on #containers-filter For some reason, .focus() sometimes does not take effect, and the element immediately loses the focus again. This causes set_input_text() to select the whole page text instead of the input element, and failing to set the intended value. --- test/check-application | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/check-application b/test/check-application index 4dc4392aa..649510b3e 100755 --- a/test/check-application +++ b/test/check-application @@ -373,6 +373,12 @@ class TestApplication(testlib.MachineCase): b.click(".pf-v5-c-modal-box button:contains('Force delete')") self.waitPodRow("pod-1", False) + # HACK: there is some race here which steals the focus from the filter input and selects the page text instead + for _ in range(3): + b.focus('#containers-filter') + time.sleep(1) + if b.eval_js('document.activeElement == document.querySelector("#containers-filter")'): + break b.set_input_text('#containers-filter', '') self.machine.execute("podman pod create --infra=false --name pod-2") self.waitPodContainer("pod-2", [])