Do not acquire lock for file.Sync() fsync call #404

1978629634 · 2024-04-02T14:10:58Z

What this PR does / why we need it:

For file.Sync() (the fsync system call that flushes the kernel buffer to disk), there is no need to acquire a lock; let the kernel handle it.

Which issue(s) this PR fixes:

Fixes #403

Release note:

Do not acquire lock for file.Sync() fsync call

linux-foundation-easycla · 2024-04-02T14:11:02Z

The committers listed above are authorized under a signed CLA.

✅ login: 1978629634 / name: Chen Pu (79575d8)

k8s-ci-robot · 2024-04-02T14:11:07Z

Welcome @1978629634!

It looks like this is your first PR to kubernetes/klog 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/klog has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

1978629634 · 2024-04-03T06:18:32Z

Hi @pohly @harshanarayana @mengjiao-liu , Could you please take a look at this pull request when you have a chance? I'd appreciate your review and feedback.

klog.go

dims · 2024-04-08T12:54:08Z

xref: golang#68

1978629634 · 2024-04-08T13:30:31Z

xref: golang#68

Could you please review it again?

yuzhiquan · 2024-04-09T08:08:14Z

/lgtm

1978629634 · 2024-04-11T12:41:38Z

xref: golang#68

Could you please review it again?

The changes to the glog have a potential issue. Besides the FlushDaemon, there is also an exit() call that invokes flushAll. If we release the lock and call file.Sync() in the exit code, the program may continue to write logs, which could lead to the program running longer than expected.
In the exit(), we should acquire the lock and call file.Sync() to ensure a clean shutdown.
cc @dims

dims · 2024-04-11T13:29:17Z

/assign @pohly

1978629634 · 2024-04-18T11:50:38Z

@pohly I'd appreciate it if you could take a look at this pull request and offer any suggestions you may have. Your feedback would be invaluable. Thank you so much!

pohly · 2024-05-02T07:16:53Z

I have it on my TODO list, but it may take a while before I get to it.

logicalhan

/triage accepted
/assign @pohly

logicalhan · 2024-05-02T16:56:42Z

/assign @shiva1333

k8s-ci-robot · 2024-05-02T16:56:45Z

@logicalhan: GitHub didn't allow me to assign the following users: shiva1333.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @shiva1333

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.

klog.go

pohly

/lgtm
/approve

k8s-ci-robot · 2024-06-11T08:07:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 1978629634, pohly, yuzhiquan

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~~ [pohly]

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

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [k8s.io/klog/v2](https://togithub.com/kubernetes/klog) | `v2.120.1` -> `v2.130.0` | [![age](https://developer.mend.io/api/mc/badges/age/go/k8s.io%2fklog%2fv2/v2.130.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/go/k8s.io%2fklog%2fv2/v2.130.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/go/k8s.io%2fklog%2fv2/v2.120.1/v2.130.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/k8s.io%2fklog%2fv2/v2.120.1/v2.130.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>kubernetes/klog (k8s.io/klog/v2)</summary> ### [`v2.130.0`](https://togithub.com/kubernetes/klog/releases/tag/v2.130.0): Prepare klog release for Kubernetes v1.31 (Take 1) [Compare Source](https://togithub.com/kubernetes/klog/compare/v2.120.1...v2.130.0) #### What's Changed - chore(\*): fix spelling of Intel Corporation by [@jsoref](https://togithub.com/jsoref) in [https://github.com/kubernetes/klog/pull/399](https://togithub.com/kubernetes/klog/pull/399) - build: fix some linter warnings by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/402](https://togithub.com/kubernetes/klog/pull/402) - examples: fix linter warning by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/406](https://togithub.com/kubernetes/klog/pull/406) - Do not acquire lock for file.Sync() fsync call by [@1978629634](https://togithub.com/1978629634) in [https://github.com/kubernetes/klog/pull/404](https://togithub.com/kubernetes/klog/pull/404) - ktesting: tone down warning about leaked test goroutine by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/401](https://togithub.com/kubernetes/klog/pull/401) #### New Contributors - [@jsoref](https://togithub.com/jsoref) made their first contribution in [https://github.com/kubernetes/klog/pull/399](https://togithub.com/kubernetes/klog/pull/399) - [@1978629634](https://togithub.com/1978629634) made their first contribution in [https://github.com/kubernetes/klog/pull/404](https://togithub.com/kubernetes/klog/pull/404) **Full Changelog**: kubernetes/klog@v2.120.1...v2.130.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/open-telemetry/opentelemetry-go-contrib).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

pohly · 2024-06-17T18:51:35Z

klog.go

+	// Flush from fatal down, in case there's trouble flushing.
+	for _, file := range files {
+		if file != nil {
+			_ = file.Sync() // ignore error


As noted in #403 (comment), syncBuffer is not thread-safe and here we call syncBuffer.Sync.

Will follow-up in #407.

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [k8s.io/klog/v2](https://togithub.com/kubernetes/klog) | `v2.120.1` -> `v2.130.0` | [![age](https://developer.mend.io/api/mc/badges/age/go/k8s.io%2fklog%2fv2/v2.130.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/go/k8s.io%2fklog%2fv2/v2.130.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/go/k8s.io%2fklog%2fv2/v2.120.1/v2.130.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/k8s.io%2fklog%2fv2/v2.120.1/v2.130.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>kubernetes/klog (k8s.io/klog/v2)</summary> ### [`v2.130.0`](https://togithub.com/kubernetes/klog/releases/tag/v2.130.0): Prepare klog release for Kubernetes v1.31 (Take 1) [Compare Source](https://togithub.com/kubernetes/klog/compare/v2.120.1...v2.130.0) #### What's Changed - chore(\*): fix spelling of Intel Corporation by [@jsoref](https://togithub.com/jsoref) in [https://github.com/kubernetes/klog/pull/399](https://togithub.com/kubernetes/klog/pull/399) - build: fix some linter warnings by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/402](https://togithub.com/kubernetes/klog/pull/402) - examples: fix linter warning by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/406](https://togithub.com/kubernetes/klog/pull/406) - Do not acquire lock for file.Sync() fsync call by [@1978629634](https://togithub.com/1978629634) in [https://github.com/kubernetes/klog/pull/404](https://togithub.com/kubernetes/klog/pull/404) - ktesting: tone down warning about leaked test goroutine by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/401](https://togithub.com/kubernetes/klog/pull/401) #### New Contributors - [@jsoref](https://togithub.com/jsoref) made their first contribution in [https://github.com/kubernetes/klog/pull/399](https://togithub.com/kubernetes/klog/pull/399) - [@1978629634](https://togithub.com/1978629634) made their first contribution in [https://github.com/kubernetes/klog/pull/404](https://togithub.com/kubernetes/klog/pull/404) **Full Changelog**: kubernetes/klog@v2.120.1...v2.130.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).  --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: opentelemetrybot <[email protected]>

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 2, 2024

k8s-ci-robot requested review from harshanarayana and mengjiao-liu April 2, 2024 14:11

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Apr 2, 2024

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 2, 2024

1978629634 force-pushed the fsync-freelock branch from 5c6f209 to 425d338 Compare April 2, 2024 14:11

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 2, 2024

dims reviewed Apr 8, 2024

View reviewed changes

klog.go Outdated Show resolved Hide resolved

1978629634 force-pushed the fsync-freelock branch from 425d338 to a3c1718 Compare April 8, 2024 13:27

k8s-ci-robot assigned yuzhiquan Apr 9, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2024

1978629634 requested a review from dims April 9, 2024 15:05

1978629634 force-pushed the fsync-freelock branch from a3c1718 to ccda4e7 Compare April 11, 2024 08:54

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2024

k8s-ci-robot assigned pohly Apr 11, 2024

logicalhan reviewed May 2, 2024

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 2, 2024

pohly mentioned this pull request Jun 10, 2024

ktesting: tone down warning about leaked test goroutine #401

Merged

pohly reviewed Jun 10, 2024

View reviewed changes

klog.go Outdated Show resolved Hide resolved

Do not acquire lock for file.Sync() fsync call

79575d8

1978629634 force-pushed the fsync-freelock branch from ccda4e7 to 79575d8 Compare June 11, 2024 03:31

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 11, 2024

pohly approved these changes Jun 11, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 11, 2024

k8s-ci-robot merged commit 2ee202a into kubernetes:main Jun 11, 2024
2 checks passed

pohly mentioned this pull request Jun 17, 2024

dependencies: klog v2.130.1 kubernetes/kubernetes#125531

Merged

pohly reviewed Jun 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not acquire lock for file.Sync() fsync call #404

Do not acquire lock for file.Sync() fsync call #404

1978629634 commented Apr 2, 2024 •

edited

Loading

linux-foundation-easycla bot commented Apr 2, 2024 •

edited

Loading

k8s-ci-robot commented Apr 2, 2024

1978629634 commented Apr 3, 2024 •

edited

Loading

dims commented Apr 8, 2024

1978629634 commented Apr 8, 2024 •

edited

Loading

yuzhiquan commented Apr 9, 2024

1978629634 commented Apr 11, 2024

dims commented Apr 11, 2024

1978629634 commented Apr 18, 2024

pohly commented May 2, 2024

logicalhan left a comment

logicalhan commented May 2, 2024

k8s-ci-robot commented May 2, 2024

pohly left a comment

k8s-ci-robot commented Jun 11, 2024

pohly Jun 17, 2024

Do not acquire lock for file.Sync() fsync call #404

Do not acquire lock for file.Sync() fsync call #404

Conversation

1978629634 commented Apr 2, 2024 • edited Loading

linux-foundation-easycla bot commented Apr 2, 2024 • edited Loading

k8s-ci-robot commented Apr 2, 2024

1978629634 commented Apr 3, 2024 • edited Loading

dims commented Apr 8, 2024

1978629634 commented Apr 8, 2024 • edited Loading

yuzhiquan commented Apr 9, 2024

1978629634 commented Apr 11, 2024

dims commented Apr 11, 2024

1978629634 commented Apr 18, 2024

pohly commented May 2, 2024

logicalhan left a comment

Choose a reason for hiding this comment

logicalhan commented May 2, 2024

k8s-ci-robot commented May 2, 2024

pohly left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 11, 2024

pohly Jun 17, 2024

Choose a reason for hiding this comment

1978629634 commented Apr 2, 2024 •

edited

Loading

linux-foundation-easycla bot commented Apr 2, 2024 •

edited

Loading

1978629634 commented Apr 3, 2024 •

edited

Loading

1978629634 commented Apr 8, 2024 •

edited

Loading