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

Cirrus: preserve podman-server logs #17153

Merged

Conversation

edsantiago
Copy link
Member

@edsantiago edsantiago commented Jan 18, 2023

Output from podman system service, on system tests, is
being saved... it just hasn't been collected as an artifact.
Start collecting it. And, remove obsolete-unused-misleading
code that made me think it was being collected.

Also: log system-service output for bud tests

Signed-off-by: Ed Santiago [email protected]

None

@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 18, 2023
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you!

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, 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 [edsantiago,vrothberg]

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

@Luap99
Copy link
Member

Luap99 commented Jan 18, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2023
@edsantiago
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2023
@edsantiago
Copy link
Member Author

I don't think it's working (logs missing) but I'll look after meeting

@edsantiago
Copy link
Member Author

Yes, but only for system tests. I thought it would work for bud and int tests too, and it's not. I don't think there's any way I can log int, but I think it's not too hard to log bud. Am working on it.

@edsantiago edsantiago force-pushed the preserve_server_logs branch from 1e98ec5 to a572f37 Compare January 18, 2023 18:37
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2023
@edsantiago
Copy link
Member Author

Yay logs:

...and now bud. Unfortunately, the bud log is empty apart from test names. All the other logs have stuff like "pulling images", "storing signatures". Does anyone know why there's nothing at all in the system-service logs for bud? Did I do something wrong?

+ # static CONTAINERS_CONF needed for capabilities test. As of 2021-07-01
+ # no tests in bud.bats override this; if at some point any test does
+ # so, it will probably need to be skip_if_remote()d.
+ env CONTAINERS_CONF=${CONTAINERS_CONF:-$(dirname ${BASH_SOURCE})/containers.conf} $PODMAN_NATIVE system service --timeout=0 &
+ env CONTAINERS_CONF=${CONTAINERS_CONF:-$(dirname ${BASH_SOURCE})/containers.conf} $PODMAN_NATIVE system service --timeout=0 &>>${PODMAN_SERVER_LOG:-/dev/stderr} &
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ env CONTAINERS_CONF=${CONTAINERS_CONF:-$(dirname ${BASH_SOURCE})/containers.conf} $PODMAN_NATIVE system service --timeout=0 &>>${PODMAN_SERVER_LOG:-/dev/stderr} &
+ env CONTAINERS_CONF=${CONTAINERS_CONF:-$(dirname ${BASH_SOURCE})/containers.conf} $PODMAN_NATIVE system service --timeout=0 &>>${$PODMAN_SERVER_LOG:-/dev/stderr} &

@edsantiago could this fix the issue? Not sure though ... bash isn't my strong suit.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually a syntax error. The ${ VAR colon-minus default} means to use stderr if PODMAN_SERVER_LOG is unset:

$ echo ${PODMAN_SERVER_LOG:-/dev/stderr}
/dev/stderr
$ PODMAN_SERVER_LOG=/tmp/foo
$ echo ${PODMAN_SERVER_LOG:-/dev/stderr}
/tmp/foo

@Luap99
Copy link
Member

Luap99 commented Jan 19, 2023

...and now bud. Unfortunately, the bud log is empty apart from test names. All the other logs have stuff like "pulling images", "storing signatures". Does anyone know why there's nothing at all in the system-service logs for bud? Did I do something wrong?

the log level is warning by default, so it could be very wall that are no warning/error logged. The fact the we see pull progress on the server side also sounds like a bug to me and not a feature, why would anybody want that. I assume bud test do not require any pulls because the images are already present in the store.

@edsantiago
Copy link
Member Author

we see pull progress on the server side also sounds like a bug to me

Oh, yeah. But it's existed so long that it's probably a feature now.

I assume bud test do not require any pulls because the images are already present in the store.

I haven't actually checked, but I used to be kinda-sure that some of the bud tests did image pulls? Like, isn't that why we see the quay03 flake in podman bud tests?

@Luap99
Copy link
Member

Luap99 commented Jan 19, 2023

I haven't actually checked, but I used to be kinda-sure that some of the bud tests did image pulls? Like, isn't that why we see the quay03 flake in podman bud tests?

Could be, maybe going through buildah just suppresses this output in the remote context for some reason.

If you want to make sure it works you could add --log-level=info, this might actually make more sense anyway because it would log the actual requests which will be useful for debugging.

Output from podman system service, on system tests, is
being saved... it just hasn't been collected as an artifact.
Start collecting it. And, remove obsolete-unused-misleading
code that made me think it _was_ being collected.

Also: log system-service output for bud tests, and set
log-level to info per suggestion from @Luap99

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago force-pushed the preserve_server_logs branch from a572f37 to 07d297c Compare January 19, 2023 13:28
@edsantiago
Copy link
Member Author

you could add --log-level=info,

Good idea. Tested locally, I like the results (informative, not TOO noisy). May help someone in the future. Done and pushed. Thank you.

@edsantiago
Copy link
Member Author

...and here's the podman server log for bud tests, with log-level info

/hold cancel

Ready to merge, if it meets team approval. Thanks for the advice.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 19, 2023
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

Nice work, @edsantiago and @Luap99 !

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2023
@openshift-merge-robot openshift-merge-robot merged commit 29a90c3 into containers:main Jan 19, 2023
@edsantiago edsantiago deleted the preserve_server_logs branch January 19, 2023 16:02
@github-actions 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 Sep 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants