libpod/events: Use unix timestamp for event time, and add a timeNano field for event time in nanoseconds #18549

ch33hau · 2023-05-12T00:54:21Z

Does this PR introduce a user-facing change?

Yes

The `Time` field in the output of `podman events --format '{{json .}}'` has been changed to `time`, and its value is now a Unix timestamp in seconds. 
Apart from that, a `timeNano` field is also introduced, which is the same as the time field but in nanoseconds precision.

rhatdan · 2023-05-12T13:11:45Z

pkg/domain/entities/events.go

 	libpodEvents "github.com/containers/podman/v4/libpod/events"
 	dockerEvents "github.com/docker/docker/api/types/events"
+	"strconv"


Please keep the golang native separate from the githup imports.

rhatdan · 2023-05-12T13:12:04Z

Thanks @ch33hau
Linter is not happy

TomSweeneyRedHat · 2023-05-12T19:04:47Z

Changes LGTM once @rhatdan 's comment is addressed and the tests are made happy.

ch33hau · 2023-05-14T20:39:08Z

Thanks for reviewing, @rhatdan @TomSweeneyRedHat !

@rhatdan I think your comment about the imports was the reason why Linter is unhappy.

I have fixed that linter complaint as well as some bugs (should use time.Unix(0, nsec) instead of time.Unix(sec, 0)) that caught by apiv2test, and also updated a doc.
It would be nice if there is at least an additional review :)

By the way, I have a side question which is not directly related to this issue:
Actually, the linter issue is pretty challenging for me to be figured out, if not from your comment, because I am unable to run make lint and make validate on my machine (Fedora 37, Go 1.19.7).
Running both comments will see the below output on my machine:

make validate
VERSION=1.51.1 ./hack/install_golangci.sh
golangci-lint has version 1.51.1 built from b87d2c19 on 2023-02-05T13:06:33Z
Using existing ./bin/golangci-lint has version 1.51.1 built from b87d2c19 on 2023-02-05T13:06:33Z
hack/golangci-lint.sh run

Running golangci-lint for tunnel
Build Tags tunnel: apparmor,seccomp,selinux,remote
Skipped directories tunnel: pkg/api
WARN [linters_context] rowserrcheck is disabled because of generics. You can track the evolution of the generics support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters_context] wastedassign is disabled because of generics. You can track the evolution of the generics support by following the https://github.com/golangci/golangci-lint/issues/2649. 
pkg/domain/infra/tunnel/images.go:21:2: "github.com/containers/podman/v4/pkg/errorhandling" imported and not used (typecheck)
	"github.com/containers/podman/v4/pkg/errorhandling"
	^
pkg/domain/infra/tunnel/network.go:12:2: "github.com/containers/podman/v4/pkg/errorhandling" imported and not used (typecheck)
	"github.com/containers/podman/v4/pkg/errorhandling"
	^
pkg/domain/infra/tunnel/pods.go:11:2: "github.com/containers/podman/v4/pkg/errorhandling" imported and not used (typecheck)
	"github.com/containers/podman/v4/pkg/errorhandling"
	^
... more similar errors

I will keep investigating how I can make it works but just wanted to ask you if you by chance have any ideas to deal with these problems. (I tried to search from Github issues but seems no other people have similar issues)

Thanks a lot for your time!

ch33hau · 2023-05-15T00:42:45Z

/retest

openshift-ci · 2023-05-15T00:43:01Z

@ch33hau: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

- Use unix timestamp for event time - Add a timeNano field for event time in nanoseconds Fixes: containers#14993 Signed-off-by: Chee Hau Lim <[email protected]>

edsantiago · 2023-05-15T12:15:37Z

This seems to break API?

ch33hau · 2023-05-15T13:09:00Z

Still working on fixing localsystem tests.

vrothberg · 2023-05-15T13:55:46Z

This seems to break API?

Yes. The Event struct is used in the REST API, so we need to keep it stable.

Looking at #14993, I am not we can guarantee compatibility on the raw JSON level. Was there a concrete issue or bug caused by the differences or is it more of a cosmetic nature?

ch33hau · 2023-05-15T19:12:15Z

This seems to break API?

Yes. The Event struct is used in the REST API, so we need to keep it stable.

Looking at #14993, I am not we can guarantee compatibility on the raw JSON level. Was there a concrete issue or bug caused by the differences or is it more of a cosmetic nature?

@edsantiago @vrothberg thanks for mentioning the API changes. That sounds like I should on-hold the changes until a decision has been made, right?

vrothberg · 2023-05-16T07:01:28Z

Thanks, @ch33hau. Yes, let's discuss how to proceed.

@ch33hau, was there a concrete issue or bug caused by the differences of the raw JSON or is it more of a cosmetic nature?

Luap99 · 2023-05-16T09:03:22Z

Thanks, @ch33hau. Yes, let's discuss how to proceed.

@ch33hau, was there a concrete issue or bug caused by the differences of the raw JSON or is it more of a cosmetic nature?

The issue links https://github.com/stepchowfun/docuum

Looking at #14993 the docker json output is all lowercase, so technically we could add new fields as lowercase. Converting the timestamp can be done on the client side. Breaking the internal event type must be avoided, this would result in a newer client unable to read the events from the event file.

openshift-merge-robot · 2023-05-16T09:55:58Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ch33hau · 2023-05-16T11:04:16Z

Thanks, @ch33hau. Yes, let's discuss how to proceed.

@ch33hau, was there a concrete issue or bug caused by the differences of the raw JSON or is it more of a cosmetic nature?

@vrothberg Sorry that I am pretty new to Podman so I might not totally get your question correctly.

What I learned from #14993 was that a user is asking if Podman is trying to match Docker's outputs, because the user is facing some issues when using a tool that is working fine with Docker but not Podman.

And then this comment says that time and timeNano fields should show the Unix timestamp. (at the moment podman events --format '{{json .}}' returns the Time field that displays a human-readable format and starts with an uppercase, and has no timeNano field.)

vrothberg

Apologies, I was on PTO for two weeks.

@Luap99, what's your take?

vrothberg · 2023-06-13T07:53:11Z

cmd/podman/machine/machine.go

@@ -147,8 +147,10 @@ func eventSockDir() (string, error) {
 func newMachineEvent(status events.Status, event events.Event) {
 	openEventSock.Do(initMachineEvents)

+	now := time.Now()
+	event.Time = now.Unix()
+	event.TimeNano = now.UnixNano()


Alternatively use events.new(status).

Luap99 · 2023-06-13T08:49:29Z

The unix time can be added to the json but only in the frontend, i.e. cmd/podman/system/events.go. We cannot break our internal structure here, this is also used via our libpod rest API.

You could embed the event.Time struct there in a new struct and use that to overwrite some field to include the docker fields as well. In any case we should keep existing fields to not cause regressions for users parsing existing fields.

github-actions · 2023-07-31T00:07:06Z

A friendly reminder that this PR had no activity for 30 days.

vrothberg · 2023-09-22T11:00:33Z

Friendly ping. @ch33hau, do you have cycles to complete this PR?

vrothberg

Closing this PR due to inactivity. Feel free to reopen or others to take over and create a new PR.

openshift-ci · 2023-10-18T09:46:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ch33hau, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the release-note label May 12, 2023

rhatdan reviewed May 12, 2023

View reviewed changes

ch33hau force-pushed the event-unix-time-and-time-nano branch 4 times, most recently from 8cdb0f3 to ead1cf4 Compare May 14, 2023 20:14

ch33hau force-pushed the event-unix-time-and-time-nano branch from ead1cf4 to 66395eb Compare May 15, 2023 00:05

ch33hau force-pushed the event-unix-time-and-time-nano branch from 66395eb to 1473bef Compare May 15, 2023 01:02

libpod/events: Update event time format and add timeNano

ec0668a

- Use unix timestamp for event time - Add a timeNano field for event time in nanoseconds Fixes: containers#14993 Signed-off-by: Chee Hau Lim <[email protected]>

ch33hau force-pushed the event-unix-time-and-time-nano branch from 1473bef to ec0668a Compare May 15, 2023 09:09

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2023

vrothberg reviewed Jun 13, 2023

View reviewed changes

joachimBurket mentioned this pull request Jul 3, 2023

Podman support stepchowfun/docuum#226

Open

github-actions bot added the stale-pr label Jul 31, 2023

vrothberg approved these changes Oct 18, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2023

vrothberg closed this Oct 18, 2023

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jan 17, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libpod/events: Use unix timestamp for event time, and add a timeNano field for event time in nanoseconds #18549

libpod/events: Use unix timestamp for event time, and add a timeNano field for event time in nanoseconds #18549

ch33hau commented May 12, 2023

rhatdan May 12, 2023

rhatdan commented May 12, 2023

TomSweeneyRedHat commented May 12, 2023

ch33hau commented May 14, 2023

ch33hau commented May 15, 2023

openshift-ci bot commented May 15, 2023

edsantiago commented May 15, 2023

ch33hau commented May 15, 2023

vrothberg commented May 15, 2023

ch33hau commented May 15, 2023

vrothberg commented May 16, 2023

Luap99 commented May 16, 2023

openshift-merge-robot commented May 16, 2023

ch33hau commented May 16, 2023

vrothberg left a comment

vrothberg Jun 13, 2023

Luap99 commented Jun 13, 2023

github-actions bot commented Jul 31, 2023

vrothberg commented Sep 22, 2023

vrothberg left a comment

openshift-ci bot commented Oct 18, 2023

libpod/events: Use unix timestamp for event time, and add a timeNano field for event time in nanoseconds #18549

libpod/events: Use unix timestamp for event time, and add a timeNano field for event time in nanoseconds #18549

Conversation

ch33hau commented May 12, 2023

Does this PR introduce a user-facing change?

rhatdan May 12, 2023

Choose a reason for hiding this comment

rhatdan commented May 12, 2023

TomSweeneyRedHat commented May 12, 2023

ch33hau commented May 14, 2023

ch33hau commented May 15, 2023

openshift-ci bot commented May 15, 2023

edsantiago commented May 15, 2023

ch33hau commented May 15, 2023

vrothberg commented May 15, 2023

ch33hau commented May 15, 2023

vrothberg commented May 16, 2023

Luap99 commented May 16, 2023

openshift-merge-robot commented May 16, 2023

ch33hau commented May 16, 2023

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Jun 13, 2023

Choose a reason for hiding this comment

Luap99 commented Jun 13, 2023

github-actions bot commented Jul 31, 2023

vrothberg commented Sep 22, 2023

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 18, 2023