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.

Sign up for GitHub

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

Jump to bottom

Kube Play: use passthrough as the default log-driver if service-container is set #16947

Merged
merged 1 commit into from
Jan 3, 2023
Merged

Kube Play: use passthrough as the default log-driver if service-container is set #16947

merged 1 commit into from
Jan 3, 2023

Conversation

ygalblum
Copy link
Contributor

@ygalblum ygalblum commented Dec 27, 2022 •
edited
Loading

If log-driver is not set, set to passthough is service-container is set otherwise use the global default

Update the system tests

  • explicitly set logdriver in sdnotify and play tests
  • podman-kube template test: Verify the default log driver for service-container

Does this PR introduce a user-facing change?

Yes

When using --service-container the default log-driver is now passthrough

Resolves: #16592

rhatdan
rhatdan reviewed Dec 29, 2022
View reviewed changes
cmd/podman/kube/play.go Outdated
@@ -247,6 +247,14 @@ func play(cmd *cobra.Command, args []string) error {
return errors.New("--force may be specified only with --down")
}

if len(playOptions.LogDriver) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

I think a better check would be whether the user overrode the default or not.

Something like:

if !cmd.Flags().Changed("log-driver")  && playOptions.ServiceContainer {			
     playOptions.LogDriver = define.PassthroughLogging
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know this capability of the Flags, thanks. Fixed

@TomSweeneyRedHat
Copy link
Member

LGTM once @rhatdan 's comment is addressed.

@ygalblum ygalblum force-pushed the kube-service-container-logdriver branch from e65985a to 3710c54 Compare January 1, 2023 06:45
vrothberg
vrothberg reviewed Jan 2, 2023
View reviewed changes
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.

I miss an explanation on why. Could you add a code comment and something to the commit message on why the defaults are changed?

Also: Why only for kube play? Wouldn't it make sense to cover all systemd cases?

@ygalblum
Copy link
Contributor Author

ygalblum commented Jan 2, 2023

@vrothberg

I miss an explanation on why. Could you add a code comment and something to the commit message on why the defaults are changed?

The Why is explained in the issue linked to this PR. I can add a comment copying the explanation given there.

Also: Why only for kube play? Wouldn't it make sense to cover all systemd cases?

I made the change only for kube play because this is what the issue referenced to. Is there an equivalent flag for podman run? I've just did a quick look but couldn't find something that stood out

@edsantiago
Copy link
Member

The Why is explained in the issue linked to this PR. I can add a comment copying the explanation given there.

Teachable moment: this is an antipattern. Linking to issues or external resources causes cognitive load on maintenance programmers. Any time a reviewer asks me "why", that's an opportunity to reflect on my code and realize that future maintainers will also wonder, and will appreciate a comment.

@ygalblum
Kube Play: use passthrough as the default log-driver if service-conta…
68fbebf 
…iner is set

Reasoning
---------
When the log-driver is passthrough, the journal socket is passed to the containers as-is which has two advantages:
1. journald can see who the actual sender of the log event is,
    rather than thinking everything comes from the conmon process
2. conmon will not have to copy all the log data

Code Changes
------------
If log-driver was not set by the user and service-container is set use
passthrough as the default log-driver

Update the system tests
- explicitly set logdriver in sdnotify and play tests
- podman-kube template test:  Verify the default log driver for service-container

Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
@ygalblum ygalblum force-pushed the kube-service-container-logdriver branch from 3710c54 to 68fbebf Compare January 3, 2023 08:34
@ygalblum
Copy link
Contributor Author

ygalblum commented Jan 3, 2023

I've added the comment in both the code and the commit message as @vrothberg asked.
@edsantiago I asked again merely because the maintainer, (@vrothberg) who asked why, is the same one who suggested this solution :)

vrothberg
vrothberg approved these changes Jan 3, 2023
View reviewed changes
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, thanks!

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 3, 2023
@vrothberg
Copy link
Member

I made the change only for kube play because this is what the issue referenced to. Is there an equivalent flag for podman run? I've just did a quick look but couldn't find something that stood out

An alternative that would also work for ordinary containers is to check whether the PODMAN_SYSTEMD_UNIT flag is set but it's perfectly fine as is.

I asked again merely because the maintainer, (@vrothberg) who asked why, is the same one who suggested this solution :)

I forgot over the break that I suggested it 🤣

@rhatdan
Copy link
Member

rhatdan commented Jan 3, 2023

/lgtm
/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 3, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2023
@ygalblum
Copy link
Contributor Author

ygalblum commented Jan 3, 2023

/hold cancel

@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 3, 2023
@openshift-merge-robot openshift-merge-robot merged commit 2dd9e08 into containers:main Jan 3, 2023
@ygalblum ygalblum deleted the kube-service-container-logdriver branch January 3, 2023 14:28
@edsantiago edsantiago mentioned this pull request Jan 9, 2023
Closed
@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 16, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rhatdan rhatdan rhatdan left review comments

@vrothberg vrothberg vrothberg approved these changes

Assignees

@rhatdan rhatdan

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

quadlet kube should use logging
6 participants
@ygalblum @TomSweeneyRedHat @edsantiago @vrothberg @rhatdan @openshift-merge-robot