Fix flaky sysctl completion by handling /proc/sys race conditions #27262
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Honny1 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Honny1
force-pushed
the
flake-investigation
branch
from
719fe13 to
1c8664d
Compare
Honny1
changed the title
openshift-ci
bot
added
release-note-none
and removed
do-not-merge/release-note-label-needed
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Ignore ENOENT errors during /proc/sys traversal as entries can disappear between directory enumeration and file access. Continue logging other errors for investigation. Fixes: containers#27252 Signed-off-by: Jan Rodák <[email protected]>
Honny1
force-pushed
the
flake-investigation
branch
from
1c8664d to
c4ade21
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
| var completions []string | ||
| sysPath := "/proc/sys" | ||
|
|
||
| err := filepath.Walk(sysPath, func(path string, info os.FileInfo, err error) error { |
There was a problem hiding this comment.
unrelated but we should use WalkDir() as it is more efficient
| }) | ||
|
|
||
| if err != nil { | ||
| logrus.Errorf("failed to scan sysctl parameters in %q: %v", sysPath, err) |
There was a problem hiding this comment.
Let's not log with logrus in completions, it is somewhat inconsistent and end users will never get to seem than anyway so I am not sure if that does add much value
For completion "logging" cobra provides its own function helper so we should use for consistency if you really want to log this: cobra.CompErrorln() which is what some other function here do
| // /proc/sys is a volatile virtual filesystem whose contents can change rapidly. | ||
| // Handle race conditions where entries disappear between directory enumeration | ||
| // and file access attempts - this is expected behavior in busy CI environments. | ||
| // See: https://github.com/containers/podman/issues/27252 | ||
| if errors.Is(err, fs.ErrNotExist) { | ||
| return nil | ||
| } | ||
| return err |
There was a problem hiding this comment.
It could happen on any system not just in CI, this makes it a bit sounds like we only ignore this because of CI which is not the case as the walking race can always happen.
Also while ignoring ErrNotExist is certainly right do we get anything by reporting other errors, that would still abort the completion and offer no suggestion to end users.
Maybe there is some proc/sys dir not readable as rootless user then this would fail. So I think it would be best to return the special error filepath.SkipDir here to just skip the directory where the error happened and keep searching for more
Ignore ENOENT errors during /proc/sys traversal as entries can disappear
between directory enumeration and file access. Continue logging other
errors for investigation.
Fixes: #27252
Does this PR introduce a user-facing change?