bcicen / ctop

Top-like interface for container metrics
https://ctop.sh
MIT License
15.6k stars 528 forks source link

Negative Uptime #275

Closed mshedsilegx closed 2 years ago

mshedsilegx commented 3 years ago

Linux RHEL8.4 Docker version 20.10.8, build 3967b7d ctop 0.76

Uptime is displayed negatively for some containers, see attachments of docker ps -a compared to ctop image image .

stokito commented 3 years ago

The uptime calculated here https://github.com/bcicen/ctop/blob/b48a5cf2f9638d4bf74d0563d6cd5726d565efcd/connector/docker.go#L189 It looks into two fields from inspect described here: https://docs.docker.com/engine/api/v1.41/#operation/ContainerInspect

Maybe the FinishedTime is zero so ctop takes a time from the local machine and then subtracts StartedTime which is a server time. If local time is lags on few seconds then we get a negative time, Another possible reason is that the StartedTime is in fact time when the container was completely started. Maybe some other reasons. Could you please compare time between docker server and computer? Are they are running on the same machine?

mshedsilegx commented 3 years ago

Correct, both are running on the same machine.

mshedsilegx commented 3 years ago

image

mshedsilegx commented 3 years ago

docker inspect bfe9e1da34ed | grep "2021" returns: "Created": "2021-08-16T19:30:06.734181271Z", "StartedAt": "2021-08-16T19:30:24.233332093Z", "FinishedAt": "2021-08-16T19:30:21.645054825Z"

mshedsilegx commented 3 years ago

Why not the following logic:

func calcUptime(insp *api.Container) string {
  endTime = time.Now()
  uptime := endTime.Sub(insp.State.StartedAt)
  return uptime.Truncate(time.Second).String()
}

Uptime is my mind if the difference between current time and started time

stokito commented 3 years ago

so the bug is somewhere is docker: it returns finished time before started, Looks like it failed to start or maybe it currently restarting. What is a full inspect output?

mshedsilegx commented 3 years ago

Patch below fixes the issue for me image

image

stokito commented 3 years ago

not sure if this will be fine for stopped or paused containers

mshedsilegx commented 3 years ago

good point, what about a condition calculating uptime only if the container is running (ie not paused or stopped), and display N/A otherwise ?

stokito commented 3 years ago

well, even for stopped container users may want to see how long it was executed. But I'm not an author so can't assume an expected behavior. Another docker List Containers api call returns uptime in Status field and only if the container is running

mshedsilegx commented 3 years ago

I really appreciate all your comments. I settled for this patch for now, it covers my immediate issues. I'm sure there is a much better way to resolve this, but that will do for now.

image

mshedsilegx commented 3 years ago

behavior after the patch is inline with what I'm expecting image

dricottone commented 3 years ago

I have several containers returning negative or 0 uptime. All of them are running locally, all of them are currently running (i.e. not stopped or restarting), and all of them have a FinishedAt time earlier than their StartedAt time (as shown by docker inspect CONTAINER).

I applied the latest patch suggested by mshedsilegx and the output seems 'more correct' now. At the very least, it matched the output of docker ps for my containers.

My (very limited) understanding of these fields is that FinishedAt stores the last stop time, so if a container was stopped and subsequently restarted, it would have a FinishedAt time earlier than the StartedAt time. So I think the 'most correct' patch might be to add to the condition rather than rewrite the logic entirely.

diff --git a/connector/docker.go b/connector/docker.go
index ff0be28..fb2e727 100644
--- a/connector/docker.go
+++ b/connector/docker.go
@@ -209,7 +209,7 @@ func (cm *Docker) inspect(id string) (insp *api.Container, found bool, failed bo

 func calcUptime(insp *api.Container) string {
        endTime := insp.State.FinishedAt
-       if endTime.IsZero() {
+       if endTime.IsZero() || insp.State.Running {
                endTime = time.Now()
        }
        uptime := endTime.Sub(insp.State.StartedAt)

With that said, my understanding of docker internals is very limited, so maybe this is an upstream bug after all. This works for me but your mileage may vary.

Flova commented 2 years ago

@dricottone your fix worked fine in my testing. The update rate seems a bit slow and the uptime is shown in seconds which can be confusing for running containers.

ovizii commented 2 years ago

I just upgraded to 0.7.7 and the uptime is still wrong. If I restart a container showing negative uptime it then switches to show 0s.

ctop is showing the correct and updated uptime for exactly 3 out of 39 containers.

ctop -v                                                                                                                      
ctop version 0.7.7, build 11a1cb1 go1.18    

image

Flova commented 2 years ago

287 is not included in the latest release as it is not merged. It includes a fix for the uptime calculation as well as some formatting improvements. Feel free to check it out and test it.

Flova commented 2 years ago

The issue with the calculation is that the end time is set from the last time the container was stopped but it is not updated if the container is currently running. This results in negative uptimes. Containers that have never been stopped work fine as their stop time is replaced with the current time.

Flova commented 2 years ago

This can be closed because #287 is merged. @bcicen