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.

Sign up for GitHub

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

Jump to bottom

Updating jailer to be compatible with v0.16.0 #86

Merged
merged 1 commit into from
May 20, 2019
Merged

Updating jailer to be compatible with v0.16.0 #86

merged 1 commit into from
May 20, 2019

Conversation

xibz
Copy link
Contributor

@xibz xibz commented Mar 7, 2019 •
edited
Loading

Fixes #92

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

@xibz
Copy link
Contributor Author

xibz commented Mar 7, 2019 •
edited
Loading

Test will fail until v0.16.0 of firecracker is added to the CI system

@samuelkarp
Copy link
Contributor

Test will fail until v0.16.0 of firecracker is added to the CI system

Let me know if you need help with this.

@xibz xibz closed this Apr 16, 2019
@xibz xibz reopened this May 6, 2019
@xibz xibz force-pushed the jailer_update branch from 64002df to 6306733 Compare May 6, 2019 20:12
nmeyerhans
nmeyerhans reviewed May 8, 2019
View reviewed changes
CHANGELOG.md Outdated
@@ -1,6 +1,7 @@
# [ Unreleased ]

* Fixes bug where default socketpath would always be used when not using jailer (#84)
* Updates jailer's socket path to point to the root drive.
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not make sense. Socket path should not point to a root drive.

Copy link
Contributor Author

@xibz xibz May 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'll reword to Updates the jailer's socket path to point to the unix socket in the jailer's workspace

jailer.go Outdated
@@ -318,6 +318,9 @@ func jail(ctx context.Context, m *Machine, cfg *Config) error {
WithSeccompLevel(cfg.JailerCfg.SeccompLevel).
WithStdout(stdout).
WithStderr(stderr).
WithStdout(os.Stdout).
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this replace the WithStdout call on line 319?

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 this was a rebase issue. I'll go ahead and fix that.

jailer.go Outdated
@@ -318,6 +318,9 @@ func jail(ctx context.Context, m *Machine, cfg *Config) error {
WithSeccompLevel(cfg.JailerCfg.SeccompLevel).
WithStdout(stdout).
WithStderr(stderr).
WithStdout(os.Stdout).
WithStderr(os.Stderr).
Copy link
Contributor

Choose a reason for hiding this comment

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

We're already calling WithStderr on line 320.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor

@jeromegn jeromegn May 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can we add support for Stdin while we're at it? I mean, for setting Stdin from the JailerCfg

jailer.go Outdated
@@ -290,9 +290,9 @@ func (b JailerCommandBuilder) Build(ctx context.Context) *exec.Cmd {
func jail(ctx context.Context, m *Machine, cfg *Config) error {
rootfs := ""
if len(cfg.JailerCfg.ChrootBaseDir) > 0 {
rootfs = filepath.Join(cfg.JailerCfg.ChrootBaseDir, "firecracker", cfg.JailerCfg.ID)
rootfs = filepath.Join(cfg.JailerCfg.ChrootBaseDir, "firecracker", cfg.JailerCfg.ID, rootfsFolderName)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's only tangentially related to the change you're making here, but I think rootfsFolderName is the wrong name for this variable. It implies that the contents of this directory make up a root filesystem, but that's not the case. It's where we link the kernel and root drive image, but that's not the same thing a rootfs.

Feel free to change this here if you want, otherwise we should do this in a later PR.

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'll go ahead and change it. Maybe jailerWorkspacePath?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about jailerWorkspaceDir?

Copy link
Contributor

Choose a reason for hiding this comment

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

This confused me a lot when I was trying to figure out how things worked. I passed in the wrong path entirely to the NaiveChrootStrategy

@xibz xibz force-pushed the jailer_update branch 3 times, most recently from d5c4bb9 to edaca58 Compare May 10, 2019 19:45
@jeromegn
Copy link
Contributor

The tests are not passing because logging is setup before the linking handler. The logging handler also attempts to client.PutLogger which means it'll send the request to the socket before the logs and metrics fifos are linked in the root directory.

The BootstrapLoggingHandler handler likely needs to be refactored in 2 steps where one creates the fifos and the other one sends the request. The NaiveChrootStrategy can then inject a handler between the 2 to link logs files properly.

@jeromegn
Copy link
Contributor

I've addressed some of my concerns in #96

@xibz xibz force-pushed the jailer_update branch 11 times, most recently from 8c0afd7 to 8270bd8 Compare May 20, 2019 21:54
@xibz
Copy link
Contributor Author

xibz commented May 20, 2019

@jeromegn - I've went ahead and removed the fix for fifos here. It seems like there is an issue with the jailer and put logger API which causes linking of the fifos to fail. I opened an issue here

@xibz xibz force-pushed the jailer_update branch 2 times, most recently from 524cd1b to e4e9d14 Compare May 20, 2019 22:01
@xibz
Updating jailer to be compatible with v0.16.0
0917f7f 
Signed-off-by: xibz <impactbchang@gmail.com>
@xibz xibz force-pushed the jailer_update branch from e4e9d14 to 0917f7f Compare May 20, 2019 22:03
@xibz xibz merged commit c49a624 into firecracker-microvm:master May 20, 2019
@xibz xibz deleted the jailer_update branch May 20, 2019 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeromegn jeromegn jeromegn left review comments

@nmeyerhans nmeyerhans nmeyerhans left review comments

@samuelkarp samuelkarp samuelkarp approved these changes

@mxpv mxpv Awaiting requested review from mxpv

@sipsma sipsma Awaiting requested review from sipsma

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How to use metrics and logs fifo with the jailer?
4 participants
@xibz @samuelkarp @jeromegn @nmeyerhans