Skip to content

Conversation

@RoyceDavison
Copy link
Contributor

@RoyceDavison RoyceDavison commented May 11, 2021

To remove hard links for FIFOs(LogFifo and MetricsFifo) in runc jailer, as described in #456

Signed-off-by: Royce Zhao qiqinzha@amazon.com

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@RoyceDavison RoyceDavison changed the title remove hard links in runc jailer Remove hard links in runc jailer May 11, 2021
@RoyceDavison RoyceDavison force-pushed the saved-bind-mount branch 4 times, most recently from 3cefc56 to 07661f6 Compare May 14, 2021 00:25
@RoyceDavison RoyceDavison marked this pull request as ready for review May 14, 2021 01:37
@RoyceDavison RoyceDavison marked this pull request as draft May 17, 2021 04:25
@RoyceDavison RoyceDavison force-pushed the saved-bind-mount branch 2 times, most recently from 4a49b48 to 16bd9b3 Compare May 17, 2021 23:47
@RoyceDavison RoyceDavison marked this pull request as ready for review May 17, 2021 23:48
@RoyceDavison RoyceDavison requested review from alakesh and kzys May 17, 2021 23:48
}

j.logger.Info("Successfully ran jailer handler")
j.started = true
Copy link

Choose a reason for hiding this comment

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

Why do you want to move j.started = true from here to the handler above? I'd like to keep this line here, since we may reorder the handlers in future.

Copy link
Contributor Author

@RoyceDavison RoyceDavison May 20, 2021

Choose a reason for hiding this comment

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

j.started = true in the BuildJailedRootHandler will block BuildBindMountFifoHandler, since RootHandler is always the first handler. Once RootHandler sets up j.started=true which means the runcJailer is started, no bind mount is allowed as this line of code is shown. BuildBindMountFifoHandler is triggered after firecracker.CreateLogFilesHandlerName.

Copy link

Choose a reason for hiding this comment

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

Hmm, how does that work? bindMountFileToJail is modifying runc's configuration. It has to be executed before starting runc.

Copy link
Contributor Author

@RoyceDavison RoyceDavison May 20, 2021

Choose a reason for hiding this comment

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

If j.started=true is placed inside BuildBindMountFifoHandler, it will work. I thought the runc started after all the defaultHandlers. So I just moved the line inside BuildBindMountFifoHandler. hmm..I am not quite sure when the runc is started now. Will look into it.

@RoyceDavison RoyceDavison force-pushed the saved-bind-mount branch 2 times, most recently from 015e73b to 9eebaea Compare May 20, 2021 23:19
firecracker-containerd uses hard links in runc jailer. However making a hard link doesn't work if its destination and its source are in different partitions.

Signed-off-by: Royce Zhao <qiqinzha@amazon.com>
@RoyceDavison RoyceDavison marked this pull request as draft May 27, 2021 22:29
@RoyceDavison RoyceDavison deleted the saved-bind-mount branch August 10, 2021 00:52
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.

2 participants