systemd: Display uptime on overview page

[martin-schiffner]

Implementing issue #13541 for displaying the system uptime in the System Information card on the Overview page.

[marusak]

Nice, thanks!
I have some comments about codestyle, nothing serious really.
The main problem this PR has is that the uptime will never update while the page is loaded. It needs to follow up and update itself. (When it shows 15 minutes, it needs to update to 16, 17... automatically).

[martin-schiffner]

Thanks for the comments. I fixed them and implemented automatic update of the uptime via a separate timer.

[martinpitt]

Thanks for working on this feature!

This needs a test as well, in check-system-info. I suggest one that uses the real uptime and merely asserts that it shows something (text is not empty), and then another one that fakes it with a known value. I. e. create a temporary file with a fixed uptime value, bind-mount it to /proc/uptime, log into Cockpit, and assert the human-formatted value. If that sounds too daunting for you, I'm happy to help with the test.

[martinpitt]

Thanks! This looks and works nicely now:

@garrett, any objections/comments?

So from my POV what remains is an integration test. Do you want to have a stab at this, or want me to write one?

[garrett]

First impressions from what I see in this PR:

1. Awesome! Thanks for implementing this, @martin-schiffner!
2. Drop "System" from "System uptime", so it'd be "Uptime".

I'll check it out locally now.

[garrett]

My machine has been up for 2 hours and 26 minutes, but it just says "2 hours" in the system information area. It should say "2 hours 26 minutes".

Otherwise, it's looking good (once the string is just "Uptime", without the "System" part, as it's already under "System information".)

[martinpitt]

There is a lot of bantering about humanize() precision on StackOverflow and moment issue. If you prefer a more precise result, then we can't use humanize(), and you need to go back to a custom implementation. (But you may want to extend it to cover months and years as well?)

[garrett]

@martinpitt: That's fine then, if we're using humanize() as-is. We can optimize it later (or not).

[martin-schiffner]

I changed to label to contain "Uptime" only.
Moreover, happy to help. I like this project. We use it in our company as it makes it easier for us managing virtual machines via libvirt on our lab servers.

[martinpitt]

Thanks @martin-schiffner! I added a small code cleanup and an integration test. I didn't force-push, the whole thing will be squashed into a single commit on landing anyway. @marusak, as I meddled with this PR quite a bit now, do you mind reviewing this? Thanks!

I also updated the screenshot.

Happy with the code now, but don't want to self-review.

[marusak]

Nice thanks!
The test is a bit simpler than I expected, but I think good enough :)

[martinpitt]

The test is a bit simpler than I expected

I'm happy to extend it a bit, what do you have in mind?

[marusak]

I'm happy to extend it a bit, what do you have in mind?

I was thinking something like:

1. Check it has some reasonable value (as some OSes may have different format, unlikely but..). Now you check that we can parse it, which is mostly good. I know this one is kinda tricky and can be hard to do robustly.
2. Check some values like "minutes", "days", "months".
3. Check that it follows (you do it now by checking that it will have "33 minutes", maybe better would be checking it has 33 minutes and it will move to 34 minutes).

[martinpitt]

Check it has some reasonable value

We could check that the string contains "second", "minute", "hour", "day", or "month", and then hope that nobody runs the test on a machine that runs for longer. But that gets dangerously close to testing moment itself, which I'd like to avoid.

Check some values like "minutes", "days", "months".

Yes, I'll add one more case (although it's similar to above -- we don't really want to test moment).

Check that it follows

The test already checks that it updates automatically (if that is what you mean by "follows"). We just need to take into account that each new case will increase the test time by a full minute.

[martinpitt]

@marusak: I added a second case.

[marusak]

thanks!

[digitalformula]

I see this was merged into master on 22 April - does that mean it should automatically show system uptime in the "System Information" panel? Asking as mine doesn't and I ended up making a plugin to show the system uptime and last boot time, before finding this issue.

[marusak]

I see this was merged into master on 22 April - does that mean it should automatically show system uptime in the "System Information" panel?

yes, if you use cockpit-218 or newer

[digitalformula]

I see this was merged into master on 22 April - does that mean it should automatically show system uptime in the "System Information" panel?

yes, if you use cockpit-218 or newer

Ah, CentOS 8 default repos are only cockpit-211. Bummer. Thanks for the response! 👍