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

Tidy up GO_GC handling? #2839

Open
dtrudg opened this issue Apr 22, 2024 · 3 comments
Open

Tidy up GO_GC handling? #2839

dtrudg opened this issue Apr 22, 2024 · 3 comments
Labels
roadmap Features / changes that are scheduled to be implemented techdebt

Comments

@dtrudg
Copy link
Member

dtrudg commented Apr 22, 2024

Type of issue

technical debt

Description of issue

In #8 we disabled go garbage collection for the starter. This was done to avoid the issue in #7 - where if the timing of a GC cycle was unfortunate, we could end up closing internal Go runtime netpoll fds in StartProcess, causing a crash.

When runc addressed a security issue by closing Fds, they faced the same need to avoid closing internal Go netpoll fds. Rather than disable GC so the fds do not appear, they used an internal Go function to identify the fds and avoid closing them:

opencontainers/runc@a9833ff#diff-6dc5f3f1e98fc4e379c98c5c301256dc1950dcb04fbce8d280bf12c41fadc1aaR73

We could consider using this approach.

@dtrudg
Copy link
Member Author

dtrudg commented Apr 22, 2024

@tri-adam - curious about your take on this? Better to disable GC... or better to link against and use an internal Go function?

Both are pretty hacky...

@tri-adam
Copy link
Member

The approach taken in the runc repo looks like a good one, other than the obvious downside of relying on poll.IsPollDescriptor. Assuming that function doesn't go anywhere, it's probably the better of the two evils? One thing that makes me nervous about it is IsPollDescriptor is only used in tests as far as I can see. On the other hand, the test code hasn't been modified in four years.

With the assumption that a change or removal of poll.IsPollDescriptor in some future version of Go would break our code base in an obvious way, I'd be in favour of this change.

@dtrudg
Copy link
Member Author

dtrudg commented Aug 12, 2024

Dropping this off the milestone... need to consider the apptainer solution to their #2166 and also consider the implications of adopting the runc approach given the moves toward closing of private APIs in go.

We don't have any problems reported related to our current approach... so lets be conservative here... given the underlying issue did cause real problems for some users.

@dtrudg dtrudg added techdebt roadmap Features / changes that are scheduled to be implemented labels Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
roadmap Features / changes that are scheduled to be implemented techdebt
Projects
None yet
Development

No branches or pull requests

2 participants