Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show pod details #1037

Merged
merged 3 commits into from
Sep 26, 2022
Merged

Show pod details #1037

merged 3 commits into from
Sep 26, 2022

Conversation

jelly
Copy link
Member

@jelly jelly commented Jul 15, 2022

Show the port details of a pod using a popover.


Pod CPU, memory, port and volume details

Podman now shows the CPU and memory usage of a pod. The amount port and volume mappings of a pod are now shown with a popover showing the exact detailed mapping.

image

image

@jelly jelly added the no-test label Jul 15, 2022
src/Containers.jsx Outdated Show resolved Hide resolved
src/Containers.jsx Outdated Show resolved Hide resolved
@marusak
Copy link
Member

marusak commented Jul 18, 2022

based on the last image, when there are no ports, the height is somewhat weird, isn't it?

@jelly
Copy link
Member Author

jelly commented Jul 18, 2022

based on the last image, when there are no ports, the height is somewhat weird, isn't it?

Yes, that's why it's still draft :)

src/Containers.jsx Outdated Show resolved Hide resolved
@jelly
Copy link
Member Author

jelly commented Aug 10, 2022

@garrett pushed a new version which looks like this:

image

@marusak
Copy link
Member

marusak commented Aug 22, 2022

Nice, thanks! I am not super happy with the design. It seems to create one sentence with pod group CPU Memory. I think that making labels CPU and Memory bold would help with reading of it. Also memory can be empty, that is not a good practice. Can we either not show the label either or show 0MB?

Screenshot from 2022-08-22 09-53-04

@garrett
Copy link
Member

garrett commented Sep 5, 2022

Here's how it'd work right now, as-is:

account-create-desktop excalidraw

(Note the larger text for the pod group name, which I've used in every mockup so far, but isn't in this PR.)

And it would flow on mobile to be something like this:

account-create-mobile excalidraw

Closest to a summary I've seen in PF is using icons + numbers in tables like this demo:
https://www.patternfly.org/v4/demos/primary-detail/react-demos/primary-detail-full-page/

image

We'd have to have the right icons if we use icons. But the icons might not be self-explanitory and they're not consistent with the lists of the containers — or anywhere else in Cockpit — so I'm hesitant to suggest them.

@garrett
Copy link
Member

garrett commented Sep 5, 2022

There seems to be too much padding on the right and top of the pod group? It's odd.

I've trimmed it a little and modified the screenshot a little (including bumping up the pod's heading) and have this:

image

Here's what it would look like with icons (the ones PF specifies for each type of item):

image

Obviously, they'd have to have some kind of hover tooltip to explain what each represents. And the linked ones with a popover would probably say "click to expand" or something as well. And on mobile, we'd need to expand the name and not show the tooltips, as hover isn't really possible on mobile.

(The icons actually don't look too bad?)

@jelly
Copy link
Member Author

jelly commented Sep 12, 2022

Ok, let's implement the icons design, I'm not 100% sure how we would implement the changing of components for the mobile view. Currently I can only think of adding an resize eventlistener or using a special class which hides/shows components depending on the using display: block and PF's breakpoints.

@garrett
Copy link
Member

garrett commented Sep 12, 2022

Why not the hidden utility class in a breakpoint, just for the text? (Or perhaps we might need the opposite, depending on what the breakpoint means.)

Mobile doesn't support hover, so we could just ignore the hover tooltips, as they won't show. But the text will.

@jelly
Copy link
Member Author

jelly commented Sep 13, 2022

image

Some progress, yes icons need to be aligned...

@jelly jelly force-pushed the issues/489 branch 2 times, most recently from d8aaf70 to bed7a1b Compare September 13, 2022 13:26
@jelly
Copy link
Member Author

jelly commented Sep 14, 2022

Unrelated issue:

image

@garrett
Copy link
Member

garrett commented Sep 14, 2022

Which issue? I see a few.

  1. Title is truncated, with no way to see what it is.
    • This can be always set to wrap by changing the white-space: nowrap; to white-space: break-spaces;
    • line-clamp could help here by letting it be a specified number of lines before truncating (instead of just 1), but it's still under a -webkit- prefix and requires a bunch of wacky other bits of CSS to have it work. However, it does work cross browser in a reliable, supported manner. https://css-tricks.com/almanac/properties/l/line-clamp/ (This is incompatible with the above suggestion, which is simpler and fine as long as the line doesn't run too long.)
  2. Modal is missing content to explain what it will do and to what it will do it to.
  3. There's no need to have "Confirm ..." in the modal. That's redundant.

@jelly jelly force-pushed the issues/489 branch 2 times, most recently from f808831 to 3bf42e9 Compare September 15, 2022 14:31
@jelly jelly requested a review from marusak September 15, 2022 15:06
@jelly jelly marked this pull request as ready for review September 16, 2022 09:27
src/Containers.jsx Outdated Show resolved Hide resolved
jelly and others added 2 commits September 19, 2022 17:26
A pod consists out of one or more containers which consume CPU, memory
which we would like to show combined in the pod's header. The CPU usage
is reported per container, so we do not add them all up but show the
highest CPU usage of one of the containers in the pod. For memory usage
we do add up all containers memory usage.

Pods can also have a port and volume mapping which is shared with every
container, the port/volume mapping can be obtained from the
infrastructure container which delegates these mappings.
@jelly
Copy link
Member Author

jelly commented Sep 20, 2022

# testCommitUser (__main__.TestApplication)
cp: cannot stat '/var/lib/containers/storage/overlay/compat4181880111': No such file or directory
Traceback (most recent call last):
  File "/usr/lib64/python3.10/unittest/case.py", line 546, in _callSetUp
    self.setUp()
  File "/work/bots/make-checkout-workdir/test/check-application", line 56, in setUp
    self.restore_dir("/var/lib/containers", reboot_safe=True)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 1904, in restore_dir
    self.machine.execute("mkdir -p %(vm_tmpdir)s && cp -a %(path)s/ %(backup)s/" % {
  File "/work/bots/make-checkout-workdir/bots/machine/machine_core/ssh_connection.py", line 307, in execute
    res = subprocess.run(command_line,
  File "/usr/lib64/python3.10/subprocess.py", line 524, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['env', '-u', 'LANGUAGE', 'LC_ALL=C', 'ssh', '-p', '2401', '-o', 'StrictHostKeyChecking=no', '-o', 'UserKnownHostsFile=/dev/null', '-o', 'IdentitiesOnly=yes', '-o', 'BatchMode=yes', '-o', 'PKCS11Provider=none', '-o', 'LogLevel=ERROR', '-l', 'root', '127.0.0.2', '-o', 'ControlPath=/tmp/.cockpit-test-resources/ssh-%h-%p-%r-11716', 'set -e;', 'mkdir -p /var/lib/cockpittest && cp -a /var/lib/containers/ /var/lib/cockpittest/_var_lib_containers/']' returned non-zero exit status 1.
Journal extracted to TestApplication-testCommitUser-debian-testing-127.0.0.2-2401-FAIL.log.gz
Traceback (most recent call last):
  File "/work/bots/make-checkout-workdir/test/check-application", line 56, in setUp
    self.restore_dir("/var/lib/containers", reboot_safe=True)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 1904, in restore_dir
    self.machine.execute("mkdir -p %(vm_tmpdir)s && cp -a %(path)s/ %(backup)s/" % {
  File "/work/bots/make-checkout-workdir/bots/machine/machine_core/ssh_connection.py", line 307, in execute
    res = subprocess.run(command_line,
  File "/usr/lib64/python3.10/subprocess.py", line 524, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['env', '-u', 'LANGUAGE', 'LC_ALL=C', 'ssh', '-p', '2401', '-o', 'StrictHostKeyChecking=no', '-o', 'UserKnownHostsFile=/dev/null', '-o', 'IdentitiesOnly=yes', '-o', 'BatchMode=yes', '-o', 'PKCS11Provider=none', '-o', 'LogLevel=ERROR', '-l', 'root', '127.0.0.2', '-o', 'ControlPath=/tmp/.cockpit-test-resources/ssh-%h-%p-%r-11716', 'set -e;', 'mkdir -p /var/lib/cockpittest && cp -a /var/lib/containers/ /var/lib/cockpittest/_var_lib_containers/']' returned non-zero exit status 1.

# Result testCommitUser (__main__.TestApplication) failed
# 1 TEST FAILED [2s on 1-cockpit-11]
not ok 6 /work/bots/make-checkout-workdir/test/check-application TestApplication.testCommitUser [ND@2]

# testPruneUnusedImagesSystem (__main__.TestApplication)
cp: cannot stat '/var/lib/containers/storage/overlay/opaque-bug-check3855566078': No such file or directory

# testBasicUser (__main__.TestApplication)
cp: cannot stat '/var/lib/containers/storage/overlay/compat828855375': No such file or directory
Traceback (most recent call last):

Debian testing is a flakerino

marusak
marusak previously approved these changes Sep 20, 2022
Copy link
Member

@marusak marusak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!

On Debian-testing restore_dir fails as it tries to copy a file which has
come away. So it seems that systemctl stop did not fully succeed in
stopping podman, ensure it does.
Copy link
Member

@marusak marusak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@jelly jelly merged commit 00a2cf3 into cockpit-project:main Sep 26, 2022
@jelly jelly deleted the issues/489 branch September 26, 2022 09:44
@jelly jelly removed the release-note label Nov 2, 2022
@jelly jelly mentioned this pull request Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants