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

ygalblum · 2022-12-27T13:50:10Z

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 · 2022-12-29T11:50:08Z

cmd/podman/kube/play.go

@@ -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 {


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 }

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

TomSweeneyRedHat · 2022-12-29T22:03:48Z

LGTM once @rhatdan 's comment is addressed.

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?

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

ygalblum · 2023-01-02T08:12:23Z

@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 · 2023-01-02T15:18:51Z

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.

…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 <[email protected]>

ygalblum · 2023-01-03T08:36:52Z

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

LGTM, thanks!

openshift-ci · 2023-01-03T09:26:41Z

[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:

~~OWNERS~~ [vrothberg]

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

vrothberg · 2023-01-03T09:28:46Z

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 · 2023-01-03T13:48:06Z

/lgtm
/hold

ygalblum · 2023-01-03T14:22:18Z

/hold cancel

openshift-ci bot added the release-note label Dec 27, 2022

rhatdan reviewed Dec 29, 2022

View reviewed changes

ygalblum force-pushed the kube-service-container-logdriver branch from e65985a to 3710c54 Compare January 1, 2023 06:45

vrothberg reviewed Jan 2, 2023

View reviewed changes

ygalblum force-pushed the kube-service-container-logdriver branch from 3710c54 to 68fbebf Compare January 3, 2023 08:34

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

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 bot assigned rhatdan Jan 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2023

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 merged commit 2dd9e08 into containers:main Jan 3, 2023

ygalblum deleted the kube-service-container-logdriver branch January 3, 2023 14:28

edsantiago mentioned this pull request Jan 9, 2023

placeholder issue for quay.io flakes #16973

Closed

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 bot locked as resolved and limited conversation to collaborators Sep 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ygalblum commented Dec 27, 2022 •

edited

Loading

rhatdan Dec 29, 2022

ygalblum Jan 1, 2023

TomSweeneyRedHat commented Dec 29, 2022

vrothberg left a comment

ygalblum commented Jan 2, 2023

edsantiago commented Jan 2, 2023

ygalblum commented Jan 3, 2023

vrothberg left a comment

openshift-ci bot commented Jan 3, 2023

vrothberg commented Jan 3, 2023

rhatdan commented Jan 3, 2023

ygalblum commented Jan 3, 2023

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

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

Conversation

ygalblum commented Dec 27, 2022 • edited Loading

Does this PR introduce a user-facing change?

rhatdan Dec 29, 2022

Choose a reason for hiding this comment

ygalblum Jan 1, 2023

Choose a reason for hiding this comment

TomSweeneyRedHat commented Dec 29, 2022

vrothberg left a comment

Choose a reason for hiding this comment

ygalblum commented Jan 2, 2023

edsantiago commented Jan 2, 2023

ygalblum commented Jan 3, 2023

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 3, 2023

vrothberg commented Jan 3, 2023

rhatdan commented Jan 3, 2023

ygalblum commented Jan 3, 2023

ygalblum commented Dec 27, 2022 •

edited

Loading