Skip to content

Conversation

@afbjorklund
Copy link
Collaborator

@afbjorklund afbjorklund commented Jan 10, 2026

When running in docker, there could be a large range of file descriptors causing "faked" to hang at the end...

Apply the same workaround for this as was done in apt, as posted on the mailing list. Debian Bug no: 920913

EDIT: Seems like this issue was reported to buildroot too:

https://gitlab.com/buildroot.org/buildroot/-/issues/115


https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=920913;msg=25

I rebased it to fakeroot 1.36, and minimized the patch by ignoring whitespace.

Fixes the issue when you might have increased the limits, on a real Linux host.

The official "fix" in 1.35 only adds close_range, which still takes too long time...

When running in docker, there could be a large range
of file descriptors causing "faked" to hang at the end.

Apply the same workaround for this as was done in apt,
as posted on the mailing list. Debian Bug no: 920913
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: afbjorklund
Once this PR has been reviewed and has the lgtm label, please assign comradeprogrammer for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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

@k8s-ci-robot k8s-ci-robot requested a review from nirs January 10, 2026 17:25
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 10, 2026
@afbjorklund afbjorklund requested a review from medyagh January 10, 2026 17:25
@medyagh medyagh requested a review from Copilot January 11, 2026 06:47
@medyagh
Copy link
Member

medyagh commented Jan 11, 2026

/ok-to-build-iso

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a patch for fakeroot 1.36 to optimize file descriptor closing in containers where ulimit may be very high. The patch implements an optimization that iterates through /proc/self/fd to close only open file descriptors, instead of blindly iterating through all possible file descriptor numbers up to the ulimit.

Changes:

  • Added a new patch file for fakeroot 1.36 that implements fd-closing optimization based on the approach used in APT
  • The patch adds logic to read actual open file descriptors from /proc/self/fd before falling back to the original approach

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+ if (dir != NULL) {
+ struct dirent *ent;
+ int dfd = dirfd(dir);
+ while (ent = readdir(dir)) {
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The while loop assignment lacks parentheses around the assignment expression. The C compiler will issue a warning for this pattern. The condition should be while ((ent = readdir(dir))) instead of while (ent = readdir(dir)) to make the assignment-as-condition pattern explicit and suppress compiler warnings.

Suggested change
+ while (ent = readdir(dir)) {
+ while ((ent = readdir(dir)) != NULL) {

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +31
+ fl = atoi(ent->d_name);
+ if (fl >= 0 && fl != dfd)
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The use of atoi on directory entry names is problematic. When readdir encounters "." or ".." entries, atoi will return 0, which would incorrectly attempt to close stdin (fd 0). Additionally, fl >= 0 allows closing fd 0. The condition should check fl > 0 to avoid closing stdin, and should verify the entry name consists only of digits before calling atoi to avoid issues with non-numeric directory entries.

Suggested change
+ fl = atoi(ent->d_name);
+ if (fl >= 0 && fl != dfd)
+ const char *name = ent->d_name;
+ int is_numeric = 1;
+ const char *p;
+
+ for (p = name; *p != '\0'; ++p) {
+ if (!isdigit((unsigned char)*p)) {
+ is_numeric = 0;
+ break;
+ }
+ }
+ if (!is_numeric)
+ continue;
+
+ fl = atoi(name);
+ if (fl > 0 && fl != dfd)

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +35
+ if (fl >= 0 && fl != dfd)
+#ifdef FAKEROOT_FAKENET
+ if (fl != sd)
+#endif /* FAKEROOT_FAKENET */
+ close(fl);
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The control flow structure with nested preprocessor conditionals creates ambiguous logic. When FAKEROOT_FAKENET is defined, there are two consecutive if statements without proper bracing, making it unclear which conditions must be met for the close call to execute. This should use proper logical operators (&&) or explicit bracing to clarify that both conditions (fl != dfd AND fl != sd) must be true before closing the file descriptor.

Suggested change
+ if (fl >= 0 && fl != dfd)
+#ifdef FAKEROOT_FAKENET
+ if (fl != sd)
+#endif /* FAKEROOT_FAKENET */
+ close(fl);
+ if (fl >= 0 && fl != dfd
+#ifdef FAKEROOT_FAKENET
+ && fl != sd
+#endif /* FAKEROOT_FAKENET */
+ )
+ close(fl);

Copilot uses AI. Check for mistakes.
@afbjorklund
Copy link
Collaborator Author

afbjorklund commented Jan 11, 2026

This 2021 patch is not yet accepted upstream, when it is it might be more worthwhile to spend time on "fixing" it there.

https://salsa.debian.org/clint/fakeroot (and https://bugs.debian.org/cgi-bin/pkgreport.cgi?src=fakeroot)

@minikube-bot
Copy link
Collaborator

Hi @afbjorklund, building a new ISO failed for Commit b2c401f
See the logs at:
https://storage.cloud.google.com/minikube-builds/logs/22432/b2c401f/iso_build.txt

@afbjorklund
Copy link
Collaborator Author

Fakeroot package built OK, then Jenkins ran out of disk or something:

/home/jenkins/workspace/iso-pr-build/out/buildroot/output-x86_64/host/bin/tar: /home/jenkins/workspace/iso-pr-build/out/buildroot/output-x86_64/images/rootfs.tar: Wrote only 6144 of 10240 bytes

@medyagh
Copy link
Member

medyagh commented Jan 17, 2026

/ok-to-build-iso

@medyagh
Copy link
Member

medyagh commented Jan 17, 2026

@afbjorklund do you mind checking the copilot's comments to see if they are relevant ?

@afbjorklund
Copy link
Collaborator Author

I did (#22432 (comment)), but I'm not sure I want to go about "fixing" whitespace in every patch

@minikube-bot
Copy link
Collaborator

Hi @afbjorklund, we have updated your PR with the reference to newly built ISO. Pull the changes locally if you want to test with them or update your PR further.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2026
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Details

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.

@k8s-ci-robot
Copy link
Contributor

@afbjorklund: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
integration-docker-containerd-linux-x86 a4c91ad link true /test integration-docker-containerd-linux-x86
integration-kvm-crio-linux-x86 a4c91ad link false /test integration-kvm-crio-linux-x86
integration-kvm-containerd-linux-x86 a4c91ad link true /test integration-kvm-containerd-linux-x86
pull-minikube-build a4c91ad link true /test pull-minikube-build
integration-docker-docker-linux-x86 a4c91ad link true /test integration-docker-docker-linux-x86
integration-kvm-docker-linux-x86 a4c91ad link true /test integration-kvm-docker-linux-x86
integration-docker-crio-linux-x86 a4c91ad link false /test integration-docker-crio-linux-x86
integration-none-containerd-linux-x86 a4c91ad link false /test integration-none-containerd-linux-x86
integration-none-docker-linux-x86 a4c91ad link true /test integration-none-docker-linux-x86
integration-docker-containerd-linux-arm a4c91ad link false /test integration-docker-containerd-linux-arm
integration-vfkit-docker-macos-arm a4c91ad link false /test integration-vfkit-docker-macos-arm
integration-docker-docker-linux-arm a4c91ad link true /test integration-docker-docker-linux-arm

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants