Allow disabling fsync on log rotate and pod shutdown #8363

rtreffer · 2024-07-09T20:49:00Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

On log rotate or container exit the logs are closed and fsync is used to flush the data to disk.
This results in good crash recovery behavior but it may cause IO locks that can cause pods to wait on logging to stdout/stderr. #6743 describes the issue.

conmon supports --no-sync-log to avoid this issue. This PR adds the same (pass through) no_sync_log option to cri-o.

logging: remove unuseful fsync containers/conmon#512

Which issue(s) this PR fixes:

Fixes: #6743

Special notes for your reviewer:

This is my first PR to CRI-O. I have been able to test this locally (manually), conmon starts up with --no-sync-log.

Does this PR introduce a user-facing change?

Add `no_sync_log` option to disable fsync on container log rotation and container exit. This can improve performance at the cost of potential data loss on machine crashes.

Signed-off-by: René Treffer <[email protected]>

openshift-ci · 2024-07-09T20:49:15Z

Hi @rtreffer. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

haircommander · 2024-07-09T20:52:18Z

pkg/config/config.go


+	// NoSyncLog if enabled will disable fsync on log rotation and container exit.
+	// This can improve performance but may result in date loss on hard system crashes.
+	NoSyncLog bool `toml:"no_sync_log"`


@cri-o/cri-o-maintainers I wonder if this should be a member of the runtime config structure, as it doesn't work for runtime pod or vm right now

I just checked other log related settings in this struct and they seem to have mixed scopes as well

LogSizeMax seems to apply to oci and pod

LogToJournald seems to apply to oci only

Would it be acceptable to document this correctly?
E.g. appending Applies to oci containers only to all doc strings for the setting?

We have, indeed, overloaded RuntimeConfig a little bit. I still am of a belief that LogLevel should not be there, for example. I suppose it was also done with some of the options for convenience since it's easier to access some of these depending on which objects are being passed around...

@haircommander, I think it would be fine... for now. We then can figure out how to clean this up.

Thoughts?

sorry by runtime config I mean crio.runtime.runtimes runtime (talk about overloaded!) . this only works for runtime type OCI, and I'd love to see it as a member there instead

Ah, ok, so

Move it to RuntimeHandler

Add validation that it only applies to OCI

The thing I haven't figured out is what to do about the cli option in that case.
But I'll update this PR once I get that sorted.

I personaly don't think the cli option is needed. Do you really want it @rtreffer ?

Well, it was convinient, but looks like it might be annoying to expose it as a flag, so I am in favor of dropping it for simplicity.

kwilczynski · 2024-07-10T00:30:39Z

/ok-to-test

codecov · 2024-07-10T00:40:40Z

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 49.43%. Comparing base (0450024) to head (9ee9157).
Report is 138 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8363      +/-   ##
==========================================
- Coverage   49.49%   49.43%   -0.07%     
==========================================
  Files         153      153              
  Lines       17084    17166      +82     
==========================================
+ Hits         8456     8486      +30     
- Misses       7564     7610      +46     
- Partials     1064     1070       +6

Signed-off-by: René Treffer <[email protected]>

rtreffer-rddt · 2024-07-10T14:37:24Z

/retest-required

rtreffer · 2024-07-10T14:45:12Z

/retest-required

rtreffer · 2024-07-10T16:43:10Z

/test ci-fedora-critest

rtreffer · 2024-07-10T18:54:32Z

The remaining test failures (rawhide rpm build) are in line with other PRs / builds and as far as I can tell unrelated.

kwilczynski · 2024-07-11T03:00:45Z

The remaining test failures (rawhide rpm build) are in line with other PRs / builds and as far as I can tell unrelated.

@rtreffer, that is fine to ignore for now. We know why this is failing. See:

So nothing to worry about for now 😄

pkg/config/config.go

Signed-off-by: René Treffer <[email protected]>

rtreffer · 2024-07-11T20:09:28Z

/retest-required

rtreffer · 2024-07-11T20:20:42Z

/retest-required

kwilczynski · 2024-07-19T09:29:03Z

/retest

rtreffer · 2024-07-27T18:38:58Z

/retest

Signed-off-by: René Treffer <[email protected]>

kwilczynski · 2024-07-29T03:49:32Z

/retest

internal/oci/runtime_oci.go

Signed-off-by: René Treffer <[email protected]>

rtreffer · 2024-08-07T08:51:57Z

/retest

rtreffer · 2024-08-07T19:08:06Z

/retest

rtreffer · 2024-08-09T07:54:09Z

/retest

kwilczynski · 2024-08-09T12:02:40Z

/retest

rtreffer · 2024-08-09T14:48:50Z

/restest

rtreffer · 2024-08-11T16:07:29Z

/retest

rtreffer · 2024-08-13T08:23:59Z

/retest

kwilczynski · 2024-08-14T18:12:06Z

/approve
/lgtm

haircommander · 2024-08-14T18:29:15Z

/approve

thanks @rtreffer

kwilczynski · 2024-08-15T03:22:57Z

/retest

saschagrunert · 2024-08-15T06:59:59Z

/override ci/prow/ci-fedora-integration

openshift-ci · 2024-08-15T07:00:06Z

@saschagrunert: Overrode contexts on behalf of saschagrunert: ci/prow/ci-fedora-integration

In response to this:

/override ci/prow/ci-fedora-integration

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-sigs/prow repository.

openshift-ci · 2024-08-15T07:00:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, kwilczynski, rtreffer, saschagrunert

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~~ [haircommander,saschagrunert]

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

Allow disabling fsync on log rotate and pod shutdown

5a2e10e

Signed-off-by: René Treffer <[email protected]>

rtreffer requested a review from mrunalp as a code owner July 9, 2024 20:49

openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 9, 2024

openshift-ci bot requested review from klihub and littlejawa July 9, 2024 20:49

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 9, 2024

haircommander reviewed Jul 9, 2024

View reviewed changes

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 10, 2024

kwilczynski assigned rtreffer Jul 10, 2024

rtreffer added 2 commits July 10, 2024 09:09

Update manpage for --no-sync-log

6e3685c

Signed-off-by: René Treffer <[email protected]>

Update crio.conf manpage for no_sync_log

8eed3d8

Signed-off-by: René Treffer <[email protected]>

kwilczynski assigned haircommander and rtreffer-rddt and unassigned rtreffer-rddt Jul 11, 2024

ccmtaylor reviewed Jul 11, 2024

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

Fix "data loss" description of no_sync_log setting

b8d9714

Signed-off-by: René Treffer <[email protected]>

openshift-ci bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 28, 2024

Fix trailing whitespace in docs

a048e15

Signed-off-by: René Treffer <[email protected]>

rtreffer force-pushed the rtreffer/no-sync-log branch from 5dcace4 to a048e15 Compare July 28, 2024 15:40

openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jul 28, 2024

haircommander reviewed Aug 6, 2024

View reviewed changes

internal/oci/runtime_oci.go Outdated Show resolved Hide resolved

Move config warning for NoSyncLog

9ee9157

Signed-off-by: René Treffer <[email protected]>

openshift-ci bot assigned kwilczynski Aug 14, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2024

saschagrunert approved these changes Aug 15, 2024

View reviewed changes

openshift-ci bot assigned saschagrunert Aug 15, 2024

openshift-merge-bot bot merged commit 6b446f2 into cri-o:main Aug 15, 2024

github-actions bot mentioned this pull request Sep 25, 2024

Bump crio from v1.30.3 to v1.31.0 kokyhm/kubespray#42

Open

Allow disabling fsync on log rotate and pod shutdown #8363

Allow disabling fsync on log rotate and pod shutdown #8363

Uh oh!

Conversation

rtreffer commented Jul 9, 2024 • edited by kwilczynski Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

openshift-ci bot commented Jul 9, 2024

Uh oh!

haircommander Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

rtreffer Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

kwilczynski Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

haircommander Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

rtreffer Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

haircommander Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

rtreffer Jul 27, 2024

Choose a reason for hiding this comment

Uh oh!

kwilczynski commented Jul 10, 2024

Uh oh!

codecov bot commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rtreffer-rddt commented Jul 10, 2024

Uh oh!

rtreffer commented Jul 10, 2024

Uh oh!

rtreffer commented Jul 10, 2024

Uh oh!

rtreffer commented Jul 10, 2024

Uh oh!

kwilczynski commented Jul 11, 2024

Uh oh!

Uh oh!

rtreffer commented Jul 11, 2024

Uh oh!

rtreffer commented Jul 11, 2024

Uh oh!

kwilczynski commented Jul 19, 2024

Uh oh!

rtreffer commented Jul 27, 2024

Uh oh!

kwilczynski commented Jul 29, 2024

Uh oh!

Uh oh!

rtreffer commented Aug 7, 2024

Uh oh!

rtreffer commented Aug 7, 2024

Uh oh!

rtreffer commented Aug 9, 2024

Uh oh!

kwilczynski commented Aug 9, 2024

Uh oh!

rtreffer commented Aug 9, 2024

Uh oh!

rtreffer commented Aug 11, 2024

Uh oh!

rtreffer commented Aug 13, 2024

Uh oh!

kwilczynski commented Aug 14, 2024

Uh oh!

haircommander commented Aug 14, 2024

rtreffer commented Jul 9, 2024 •

edited by kwilczynski

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading