Skip to content

Conversation

@apostasie
Copy link
Contributor

@apostasie apostasie commented Jan 25, 2025

From reading code, it seems to me like:

  1. XDGRuntimeDir() should check the value of ROOTLESSKIT_PARENT_EUID (like ParentEUID does)
  2. getXDGRuntimeDir() in bypass4netns is essentially duplicating XDGRuntimeDir()
  3. getRuntimeVariableDataDir and CNIRuntimeDir should not ignore errors, and they seem to have faulty logic (relying on Geteuid regardless of conditions), or calling rootlessutil.ParentEUID which will redo stuff done already in XDGRuntimeDir()

PR addresses these.

@apostasie apostasie force-pushed the cleanup-xdg branch 2 times, most recently from b1c9415 to 890d40e Compare January 25, 2025 08:22
@apostasie
Copy link
Contributor Author

Was wondering if we should even incorporate into XDGRuntimeDir the extra logic

			if rootlessutil.IsRootlessChild() {
				return "", err
			}
			run = fmt.Sprintf("/run/user/%d", os.Geteuid())

but (for now) elected for pure refactoring with no change of behavior.

@apostasie
Copy link
Contributor Author

apostasie commented Jan 25, 2025

Not clear why the failures (some of it is definitely the usual stargz / layer not found), but the rest is weird (eg: platform timeout).

If we can poke it in case it is a fluke...

@apostasie apostasie marked this pull request as draft January 25, 2025 09:28
@apostasie apostasie marked this pull request as ready for review January 27, 2025 18:12
@apostasie
Copy link
Contributor Author

CI failure seems to be a timeout.

@apostasie apostasie force-pushed the cleanup-xdg branch 2 times, most recently from 116f492 to 57046a1 Compare February 21, 2025 18:32
@apostasie
Copy link
Contributor Author

Failures are a bunch of timeouts. Looks like the import cache manifest from github cache are extremely slow during the initial build phase:

Wether we should increase the overall timeout over 30 minutes or not is something we can talk about - at least these type of events would not fail the builds - but the underlying issue seem to be github action cache.

@apostasie
Copy link
Contributor Author

Rebasing to force CI re-run.

@AkihiroSuda AkihiroSuda added this to the v2.x.x (tentative) milestone Feb 22, 2025
run = fmt.Sprintf("/run/user/%d", rootlessutil.ParentEUID())
if rootlessutil.IsRootlessChild() {
return "", err
}
Copy link
Member

Choose a reason for hiding this comment

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

Warn should be still printed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is necessary, since err will be bubbled up and printed out as an error anyhow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eg: if we leave the warn, we will just print the error twice (once as a warn, and then as an error)

Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@apostasie
Copy link
Contributor Author

CI failure looks like #3556

I am not sure why we are suddenly hitting that one a lot more.

Did we recently change something wrt cni?

@AkihiroSuda AkihiroSuda merged commit 46be3a7 into containerd:main Feb 23, 2025
30 checks passed
@apostasie apostasie changed the title Cleanup XDGRuntime Refactor: cleanup XDGRuntime Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants