Skip to content

Commit

Permalink
Factorize and clean up state indexing
Browse files Browse the repository at this point in the history 
Clean up the often repeated "id + uid" index key calculation for the
app's containers/images/pods states, and centralize it into a
`makeKey()` helper. Put the uid in front, so that they have more natural
sorting (in the UI we sort by owner first), and add a `-` in between so
that they are easier to read.
  • Loading branch information
@martinpitt
martinpitt committed Feb 6, 2025
1 parent 66708c2 commit 3289f0c
Show file tree
Hide file tree
Showing 7 changed files with 63 additions and 58 deletions.
16 changes: 8 additions & 8 deletions src/Containers.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ class Containers extends React.Component {
}

renderRow(containersStats, container, localImages) {
const containerStats = containersStats[container.Id + (container.uid ?? "user").toString()];
const containerStats = containersStats[container.key];
const image = container.ImageName;
const isToolboxContainer = container.Config?.Labels?.["com.github.containers.toolbox"] === "true";
const isDistroboxContainer = container.Config?.Labels?.manager === "distrobox";
Expand Down Expand Up @@ -398,7 +398,7 @@ class Containers extends React.Component {
{
title: (container.uid === 0) ? _("system") : <div><span className="ct-grey-text">{_("user:")} </span>{this.props.user}</div>,
props: { modifier: "nowrap" },
sortKey: (container.uid ?? "user").toString()
sortKey: container.key,
},
{ title: proc, props: { modifier: "nowrap" }, sortKey: containerState === "Running" ? containerStats?.CPU ?? -1 : -1 },
{ title: mem, props: { modifier: "nowrap" }, sortKey: containerStats?.MemUsage ?? -1 },
Expand Down Expand Up @@ -458,8 +458,8 @@ class Containers extends React.Component {
columns,
initiallyExpanded: document.location.hash.substring(1) === container.Id,
props: {
key: container.Id + (container.uid ?? "user").toString(),
"data-row-id": container.Id + (container.uid ?? "user").toString(),
key: container.key,
"data-row-id": container.key,
"data-started-at": container.State?.StartedAt,
},
};
Expand All @@ -482,7 +482,7 @@ class Containers extends React.Component {
let cpu = 0;
let mem = 0;
for (const container of pod.Containers) {
const containerStats = containersStats[container.Id + (pod.uid ?? "user").toString()];
const containerStats = containersStats[utils.makeKey(pod.uid, container.Id)];
if (!containerStats)
continue;

Expand All @@ -502,7 +502,7 @@ class Containers extends React.Component {

renderPodDetails(pod, podStatus) {
const podStats = this.podStats(pod);
const infraContainer = this.props.containers[pod.InfraId + (pod.uid ?? "user").toString()];
const infraContainer = this.props.containers[utils.makeKey(pod.uid, pod.InfraId)];
const numPorts = Object.keys(infraContainer?.NetworkSettings?.Ports ?? {}).length;

return (
Expand Down Expand Up @@ -607,7 +607,7 @@ class Containers extends React.Component {
const lcf = this.props.textFilter.toLowerCase();
filtered = filtered.filter(id => this.props.containers[id].Name.toLowerCase().indexOf(lcf) >= 0 ||
(this.props.containers[id].Pod &&
this.props.pods[this.props.containers[id].Pod + (this.props.containers[id].uid ?? "user").toString()].Name.toLowerCase().indexOf(lcf) >= 0) ||
this.props.pods[utils.makeKey(this.props.containers[id].uid, this.props.containers[id].Pod)].Name.toLowerCase().indexOf(lcf) >= 0) ||
this.props.containers[id].ImageName.toLowerCase().indexOf(lcf) >= 0
);
}
Expand Down Expand Up @@ -641,7 +641,7 @@ class Containers extends React.Component {
filtered.forEach(id => {
const container = this.props.containers[id];
if (container)
(partitionedContainers[container.Pod ? (container.Pod + (container.uid ?? "user").toString()) : 'no-pod'] || []).push(container);
(partitionedContainers[container.Pod ? utils.makeKey(container.uid, container.Pod) : 'no-pod'] || []).push(container);
});

// Append downloading containers
Expand Down
1 change: 1 addition & 0 deletions src/ImageRunModal.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ export class ImageRunModal extends React.Component {
// Assign temporary properties to allow rendering
tempImage.Id = tempImage.name;
tempImage.uid = uid;
tempImage.key = utils.makeKey(tempImage.uid, tempImage.Id);
tempImage.State = { Status: _("downloading") };
tempImage.Created = new Date();
tempImage.Name = tempImage.name;
Expand Down
10 changes: 5 additions & 5 deletions src/Images.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class Images extends React.Component {
if (imageContainerList === null) {
return { title: _("unused"), count: 0 };
}
const containers = imageContainerList[image.Id + (image.uid ?? "user").toString()];
const containers = imageContainerList[image.key];
if (containers !== undefined) {
const title = cockpit.format(cockpit.ngettext("$0 container", "$0 containers", containers.length), containers.length);
return { title, count: containers.length };
Expand Down Expand Up @@ -119,7 +119,7 @@ class Images extends React.Component {
imageStats.imagesTotal += 1;
imageStats.imagesSize += image.Size;

const usedBy = imageContainerList[image.Id + (image.uid ?? "user").toString()];
const usedBy = imageContainerList[image.key];
if (usedBy === undefined) {
imageStats.unusedTotal += 1;
imageStats.unusedSize += image.Size;
Expand Down Expand Up @@ -157,7 +157,7 @@ class Images extends React.Component {
renderer: ImageDetails,
data: {
image,
containers: this.props.imageContainerList !== null ? this.props.imageContainerList[image.Id + (image.uid ?? "user").toString()] : null,
containers: this.props.imageContainerList !== null ? this.props.imageContainerList[image.key] : null,
showAll: this.props.showAll,
}
});
Expand All @@ -174,8 +174,8 @@ class Images extends React.Component {
tabRenderers={tabs} />,
columns,
props: {
key: image.Id + (image.uid ?? "user").toString(),
"data-row-id": image.Id + (image.uid ?? "user").toString(),
key: image.key,
"data-row-id": image.key,
},
};
}
Expand Down
6 changes: 3 additions & 3 deletions src/PruneUnusedContainersModal.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const getContainerRow = (container, userSystemServiceAvailable, user, selected)
if (userSystemServiceAvailable)
columns.push({
title: container.system ? _("system") : <div><span className="ct-grey-text">{_("user:")} </span>{user}</div>,
sortKey: (container.uid ?? "user").toString(),
sortKey: container.key,
props: {
className: "ignore-pixels",
modifier: "nowrap"
Expand All @@ -40,13 +40,13 @@ const getContainerRow = (container, userSystemServiceAvailable, user, selected)

const PruneUnusedContainersModal = ({ close, unusedContainers, onAddNotification, userSystemServiceAvailable, user }) => {
const [isPruning, setPruning] = useState(false);
const [selectedContainerIds, setSelectedContainerIds] = React.useState(unusedContainers.map(u => u.id + '-' + (u.uid ?? "user")));
const [selectedContainerIds, setSelectedContainerIds] = React.useState(unusedContainers.map(u => u.key));

const handlePruneUnusedContainers = () => {
setPruning(true);

const actions = unusedContainers
.filter(u => selectedContainerIds.includes(u.id + '-' + (u.uid ?? "user")))
.filter(u => selectedContainerIds.includes(u.key))
.map(u => client.delContainer(u.uid, u.id, true));

Promise.all(actions).then(close)
Expand Down
68 changes: 34 additions & 34 deletions src/app.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import Containers from './Containers.jsx';
import Images from './Images.jsx';
import * as client from './client.js';
import rest from './rest.js';
import { WithPodmanInfo } from './util.js';
import { makeKey, WithPodmanInfo } from './util.js';

const _ = cockpit.gettext;

Expand Down Expand Up @@ -148,10 +148,10 @@ class Application extends React.Component {
}
}

updateState(state, id, newValue) {
updateState(state, key, newValue) {
this.setState(prevState => {
return {
[state]: { ...prevState[state], [id]: newValue }
[state]: { ...prevState[state], [key]: newValue }
};
});
}
Expand All @@ -161,7 +161,7 @@ class Application extends React.Component {
if (reply.Error != null) // executed when container stop
console.warn("Failed to update container stats:", JSON.stringify(reply.message));
else {
reply.Stats.forEach(stat => this.updateState("containersStats", stat.ContainerID + (uid ?? "user").toString(), stat));
reply.Stats.forEach(stat => this.updateState("containersStats", makeKey(uid, stat.ContainerID), stat));
}
}).catch(ex => {
if (ex.cause == "no support for CGroups V1 in rootless environments" || ex.cause == "Container stats resource only available for cgroup v2") {
Expand All @@ -186,7 +186,8 @@ class Application extends React.Component {
});
for (const detail of containerDetails) {
detail.uid = uid;
copyContainers[detail.Id + (uid ?? "user").toString()] = detail;
detail.key = makeKey(uid, detail.Id);
copyContainers[detail.key] = detail;
}

return {
Expand All @@ -212,7 +213,8 @@ class Application extends React.Component {
});
Object.entries(reply).forEach(([Id, image]) => {
image.uid = uid;
copyImages[Id + (uid ?? "user").toString()] = image;
image.key = makeKey(uid, Id);
copyImages[image.key] = image;
});

return {
Expand All @@ -239,7 +241,8 @@ class Application extends React.Component {
});
for (const pod of reply || []) {
pod.uid = uid;
copyPods[pod.Id + (uid ?? "user").toString()] = pod;
pod.key = makeKey(uid, pod.Id);
copyPods[pod.key] = pod;
}

return {
Expand All @@ -256,31 +259,33 @@ class Application extends React.Component {
updateContainer(id, uid, event) {
/* when firing off multiple calls in parallel, podman can return them in a random order.
* This messes up the state. So we need to serialize them for a particular container. */
const idx = id + (uid ?? "user").toString();
const wait = this.pendingUpdateContainer[idx] ?? Promise.resolve();
const key = makeKey(uid, id);
const wait = this.pendingUpdateContainer[key] ?? Promise.resolve();

const new_wait = wait.then(() => client.inspectContainer(uid, id))
.then(details => {
details.uid = uid;
details.key = key;
// HACK: during restart State never changes from "running"
// override it to reconnect console after restart
if (event?.Action === "restart")
details.State.Status = "restarting";
this.updateState("containers", idx, details);
this.updateState("containers", key, details);
})
.catch(e => console.warn("updateContainer uid", uid, "inspectContainer failed:", e.toString()));
this.pendingUpdateContainer[idx] = new_wait;
new_wait.finally(() => { delete this.pendingUpdateContainer[idx] });
this.pendingUpdateContainer[key] = new_wait;
new_wait.finally(() => { delete this.pendingUpdateContainer[key] });

return new_wait;
}

updateImage(id, uid) {
client.getImages(uid, id)
.then(reply => {
const immage = reply[id];
immage.uid = uid;
this.updateState("images", id + (uid ?? "user").toString(), immage);
const image = reply[id];
image.uid = uid;
image.key = makeKey(uid, id);
this.updateState("images", image.key, image);
})
.catch(ex => {
console.warn("Failed to do updateImage for uid", uid, ":", JSON.stringify(ex));
Expand All @@ -294,7 +299,8 @@ class Application extends React.Component {
const pod = reply[0];

pod.uid = uid;
this.updateState("pods", pod.Id + (uid ?? "user").toString(), pod);
pod.key = makeKey(uid, id);
this.updateState("pods", pod.key, pod);
}
})
.catch(ex => {
Expand Down Expand Up @@ -370,14 +376,14 @@ class Application extends React.Component {
case 'remove':
this.setState(prevState => {
const containers = { ...prevState.containers };
delete containers[id + (uid ?? "user").toString()];
delete containers[makeKey(uid, id)];
let pods;

if (event.Actor.Attributes.podId) {
const podIdx = event.Actor.Attributes.podId + (uid ?? "user").toString();
const newPod = { ...prevState.pods[podIdx] };
const podKey = makeKey(uid, event.Actor.Attributes.podId);
const newPod = { ...prevState.pods[podKey] };
newPod.Containers = newPod.Containers.filter(container => container.Id !== id);
pods = { ...prevState.pods, [podIdx]: newPod };
pods = { ...prevState.pods, [podKey]: newPod };
} else {
// HACK: with podman < 4.3.0 we don't get a pod event when a container in a pod is removed
// https://github.com/containers/podman/issues/15408
Expand Down Expand Up @@ -411,7 +417,7 @@ class Application extends React.Component {
case 'remove':
this.setState(prevState => {
const pods = { ...prevState.pods };
delete pods[event.Actor.ID + (uid ?? "user").toString()];
delete pods[makeKey(uid, event.Actor.ID)];
return { pods };
});
break;
Expand Down Expand Up @@ -608,19 +614,13 @@ class Application extends React.Component {
if (this.state.containers !== null) {
Object.keys(this.state.containers).forEach(c => {
const container = this.state.containers[c];
const uid_str = (container.uid ?? "user").toString();
const image = container.Image + uid_str;
if (imageContainerList[image]) {
imageContainerList[image].push({
container,
stats: this.state.containersStats[container.Id + uid_str],
});
} else {
imageContainerList[image] = [{
container,
stats: this.state.containersStats[container.Id + uid_str]
}];
}
const imageKey = makeKey(container.uid, container.Image);
if (!imageContainerList[imageKey])
imageContainerList[imageKey] = [];
imageContainerList[imageKey].push({
container,
stats: this.state.containersStats[makeKey(container.uid, container.Id)],
});
});
} else
imageContainerList = null;
Expand Down
4 changes: 4 additions & 0 deletions src/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ export function debug(...args) {
console.debug("podman", ...args);
}

// containers, pods, images states are indexed by these keys, to make the container IDs
// globally unique across users
export const makeKey = (uid, id) => `${uid ?? "user"}-${id}`;

export function truncate_id(id) {
if (!id) {
return "";
Expand Down
16 changes: 8 additions & 8 deletions test/check-application
View file
Original file line number Diff line number Diff line change
Expand Up @@ -628,8 +628,8 @@ WantedBy=multi-user.target default.target
images[f"{items[0]}:{items[1]}"] = items[2].split(":")[-1]

# show image listing toggle
suffix = '0' if auth else 'user'
hello_sel = f"#containers-images tbody tr[data-row-id=\"{images[IMG_HELLO_LATEST]}{suffix}\"]".lower()
uid = '0' if auth else 'user'
hello_sel = f"#containers-images tbody tr[data-row-id=\"{uid}-{images[IMG_HELLO_LATEST]}\"]".lower()
b.wait_visible(hello_sel)
b.click(hello_sel + " td.pf-v5-c-table__toggle button")
b.click(hello_sel + " .pf-v5-c-menu-toggle")
Expand Down Expand Up @@ -794,7 +794,7 @@ WantedBy=multi-user.target default.target
self.execute(auth, f"podman tag {IMG_BUSYBOX} {IMG_BUSYBOX}:3")
self.execute(auth, f"podman tag {IMG_BUSYBOX} {IMG_BUSYBOX}:4")

busybox_sel = f"#containers-images tbody tr[data-row-id=\"{images[IMG_BUSYBOX_LATEST]}{suffix}\"]".lower()
busybox_sel = f"#containers-images tbody tr[data-row-id=\"{uid}-{images[IMG_BUSYBOX_LATEST]}\"]".lower()
b.click(busybox_sel + " td.pf-v5-c-table__toggle button")

b.wait_in_text(busybox_sel + " + tr", f"{IMG_BUSYBOX}:1")
Expand Down Expand Up @@ -849,7 +849,7 @@ WantedBy=multi-user.target default.target
self.waitContainer(sha, auth, name="test-sh4", state='Exited')
if auth:
b.assert_pixels('#app', "overview", ignore=[".ignore-pixels"], skip_layouts=["rtl", "mobile"])
alpine_sel = f"#containers-images tbody tr[data-row-id=\"{images[IMG_ALPINE_LATEST]}{suffix}\"]".lower()
alpine_sel = f"#containers-images tbody tr[data-row-id=\"{uid}-{images[IMG_ALPINE_LATEST]}\"]".lower()
b.wait_visible(alpine_sel)
b.click(alpine_sel + " td.pf-v5-c-table__toggle button")
clickDeleteImage(alpine_sel)
Expand Down Expand Up @@ -950,7 +950,7 @@ WantedBy=multi-user.target default.target
b.wait_not_in_text("#containers-containers tbody tr:contains('no-cmd')", "Command")

sha = self.execute(auth, f"podman inspect --format '{{{{.Id}}}}' {IMG_ENTRYPOINT}").strip()
entrypoint_sel = f"#containers-images tbody tr[data-row-id=\"{sha}{suffix}\"]".lower()
entrypoint_sel = f"#containers-images tbody tr[data-row-id=\"{uid}-{sha}\"]".lower()
clickDeleteImage(entrypoint_sel)
self.confirm_modal("Delete")
self.confirm_modal("Force delete")
Expand Down Expand Up @@ -1164,7 +1164,7 @@ WantedBy=multi-user.target default.target
# Select the image row

# show image listing toggle
imageId = f"{self.imageSha}{'0' if self.user == 'system' else 'user'}"
imageId = f"{'0' if self.user == 'system' else 'user'}-{self.imageSha}"
sel = f"#containers-images tbody tr[data-row-id=\"{imageId}\"]"
b.wait_visible(sel)
b.click(sel + " td.pf-v5-c-table__toggle button")
Expand Down Expand Up @@ -2281,8 +2281,8 @@ WantedBy=multi-user.target default.target
"image" can be substring, "state" might be string or array of possible states, other are
checked for exact match.
"""
suffix = '0' if auth else 'user'
sel = "#containers-containers #table-" + pod + f" tbody tr[data-row-id=\"{row_id}{suffix}\"]".lower()
uid = '0' if auth else 'user'
sel = "#containers-containers #table-" + pod + f" tbody tr[data-row-id=\"{uid}-{row_id}\"]".lower()
b = self.browser
if name:
b.wait_text(sel + " .container-name", name)
Expand Down

0 comments on commit 3289f0c

Please sign in to comment.