DO NOT MERGE: try to figure out the preserve-fds bug

[edsantiago]

Add "ls -l /proc/self/fd" before running the test.
Might be useful to also run "lsof -p $$", what's the
right Go incantation for getting my own pid?

Signed-off-by: Ed Santiago [email protected]

none

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

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 [edsantiago]

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

[mheon]

@edsantiago https://pkg.go.dev/os#Getpid

[edsantiago]

Yay, the flake triggered! Boo, there are no extra open fds. Unless exec.Command() closes fds??? That would be weird. I'll continue playing with this tomorrow.

[edsantiago]

Sigh. Failed, with both ls and lsof results... but I'm darned if I can figure out a useful difference between the failed ubuntu log and the passing one on fedora.

[Luap99]

@edsantiago Yes exec.Command closes all fds, see my comment on the issue:

We use os/exec to start podman in the tests, it should not matter how many fds we leak in the test since os/exec will always unset all fds.
https://cs.opensource.google/go/go/+/refs/tags/go1.19.1:src/os/exec/exec.go;l=506-517
That makes me think the leak or bug must be somewhere in podman itself.

#15943 (comment)

It does not matter how many open fds the test has, in fact we cannot control this at all since they run in parallel.
So either the golang lib has a bug and does not close the fds in all cases or we have a bug in podman.

[edsantiago]

Ugh, you're right. I guess closing fds is user-friendly in some respects, it's just counterintuitive for someone who grew up on UNIX and C. Thanks for the reminder.

Do you have any suggestions on how else to track this down? Since this is consistently ubuntu-only, could it be something that ubuntu changed in the Go libraries, as a distro patch? Or a too-old version of Go?

Again, this is easy to fix in the test, I just don't want to hide a real bug.

[Luap99]

I don't really know, all I see is that we have some custom c code which checks the fds:

podman/pkg/rootless/rootless_linux.c

Lines 299 to 338 in 84c85f4

	/* Store how many FDs were open before the Go runtime kicked in. */
	d = opendir ("/proc/self/fd");
	if (d)
	{
	struct dirent *ent;
	size_t size = 0;
	for (ent = readdir (d); ent; ent = readdir (d))
	{
	int fd;
	if (ent->d_name[0] == '.')
	continue;
	fd = atoi (ent->d_name);
	if (fd == dirfd (d))
	continue;
	if (fd >= size * FD_SETSIZE)
	{
	int i;
	size_t new_size;
	new_size = (fd / FD_SETSIZE) + 1;
	open_files_set = realloc (open_files_set, new_size * sizeof (fd_set));
	if (open_files_set == NULL)
	_exit (EXIT_FAILURE);
	for (i = size; i < new_size; i++)
	FD_ZERO (&(open_files_set[i]));
	size = new_size;
	}
	if (fd > open_files_max_fd)
	open_files_max_fd = fd;
	FD_SET (fd % FD_SETSIZE, &(open_files_set[fd / FD_SETSIZE]));
	}
	}

podman/pkg/rootless/rootless_linux.c

Lines 274 to 281 in 84c85f4

	int
	is_fd_inherited(int fd)
	{
	if (open_files_set == NULL || fd > open_files_max_fd || fd < 0)
	return 0;
	return FD_ISSET(fd % FD_SETSIZE, &(open_files_set[fd / FD_SETSIZE])) ? 1 : 0;
	}

[edsantiago]

I notice there's no matching closedir() for the opendir() on line 300. Should there be?

[edsantiago]

Unable to reproduce the flake if I use -focus on the ginkgo tests.

[openshift-merge-robot]

@edsantiago: PR needs rebase.

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.

[github-actions]

A friendly reminder that this PR had no activity for 30 days.

[rhatdan]

@edsantiago is this still valid, needs a rebase.

[edsantiago]

This is an Ubuntu-only flake. We haven't run CI on Ubuntu in a month, so I'll just close this and reopen if we ever get Ubuntu back.