Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ParentMain/nsenter: do not fail if getwd errors #3329

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

apostasie
Copy link
Contributor

To fix #3328

It is not clear to me if there would be side-effects to this.

nsenter specialists opinion highly welcome.

@apostasie apostasie changed the title ParentMain/nsenter: do not fail if getwd errors [BROKEN] ParentMain/nsenter: do not fail if getwd errors Aug 18, 2024
@apostasie apostasie marked this pull request as draft August 18, 2024 18:38
@apostasie apostasie force-pushed the b-dev-3328-getwd-nsenter branch from 276b75f to d3a8c48 Compare August 18, 2024 22:35
@apostasie apostasie changed the title [BROKEN] ParentMain/nsenter: do not fail if getwd errors ParentMain/nsenter: do not fail if getwd errors Aug 18, 2024
@apostasie apostasie marked this pull request as ready for review August 18, 2024 23:06
@apostasie apostasie force-pushed the b-dev-3328-getwd-nsenter branch 2 times, most recently from 40f4bc0 to ad6d20c Compare August 18, 2024 23:39
@apostasie apostasie force-pushed the b-dev-3328-getwd-nsenter branch from ad6d20c to e0d0079 Compare August 20, 2024 04:18
@apostasie
Copy link
Contributor Author

Comment addressed.
CI is green.

@apostasie apostasie force-pushed the b-dev-3328-getwd-nsenter branch from e0d0079 to a1bd476 Compare August 21, 2024 06:04
@apostasie apostasie requested a review from djdongjin August 21, 2024 06:04
Copy link
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. btw did you verify if this resolves the issue? i.e. the repro in #3328 works now:

mkdir foo
cd foo
rmdir ../foo
nerdctl container run -d --name foo debian sleep Inf
FATA[0000] getwd: no such file or directory

@apostasie
Copy link
Contributor Author

LGTM, thanks. btw did you verify if this resolves the issue? i.e. the repro in #3328 works now:

mkdir foo
cd foo
rmdir ../foo
nerdctl container run -d --name foo debian sleep Inf
FATA[0000] getwd: no such file or directory

Yes, it does fix the issue.
I am just mildly concerned about the behavior of nsenter in such a condition, but then... this is (somewhat) abnormal to start with.

if err != nil {
log.L.WithError(err).Warn("unable to determine working directory")
} else {
args = append(args, "-w"+wd)
Copy link
Member

Choose a reason for hiding this comment

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

IMO we may even set the working directory after nsenter with --wdns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To what path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah - you mean, in case getwd does NOT error, right?

Honestly, I would rather focus efforts on re-exec-ing instead of tweaking nsenter.

Copy link
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

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

@AkihiroSuda AkihiroSuda merged commit ed37a4d into containerd:main Aug 22, 2024
21 checks passed
@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getwd: should we really hard fail?
4 participants