Jailer configuration API cleanup and improved logging with Debug log level

[dreadl0ck]

Jailer configuration API cleanup

This pull request

1. Removes a superfluous parameter accepted by the firecracker.NewNaiveChrootStrategy function:

func NewNaiveChrootStrategy(rootfs, kernelImagePath string) NaiveChrootStrategy

The rootfs parameter is superfluous, because all the information required to assemble it is already contained in the configuration structure.

Furthermore its name is misleading: rootfs indicates that the root filesystem image should be passed (which will obviously fail), whereas actually the path to the jailed filesystem root on the host is desired.

I think it is cleaner to build this path when it's needed, then to duplicate the configuration information by generating it there.

This is an example JailerConfig initialisation with the updated API:

JailerCfg: &firecracker.JailerConfig{
    GID:            firecracker.Int(opts.Gid),
    UID:            firecracker.Int(opts.Uid),
    ID:             opts.Id,
    NumaNode:       firecracker.Int(0),
    ExecFile:       opts.ExecFile,
    JailerBinary:   "/usr/bin/jailer",
    ChrootBaseDir:  "/srv/jailer",
    Daemonize:      false,
    ChrootStrategy: firecracker.NewNaiveChrootStrategy(opts.FcKernelImage), // only pass the KernelImage
    Stdout:         os.Stdout,
    Stderr:         os.Stderr,
    Stdin:          os.Stdin,
},

The following code is used in the LinkFilesHandler to create the rootfs path that points to the correct place in the jail:

// assemble the path to the jailed root folder on the host
rootfs := filepath.Join(
    m.Cfg.JailerCfg.ChrootBaseDir,
    filepath.Base(m.Cfg.JailerCfg.ExecFile),
    m.Cfg.JailerCfg.ID,
    rootfsFolderName,
)

2. Removes unnecessary function linkFileToRootFS()

The following function has been removed and replaced with a direct invocation of os.Link():

func linkFileToRootFS(cfg *JailerConfig, dst, src string) error {
	if err := os.Link(src, dst); err != nil {
		return err
	}

	return nil
}

The cfg parameter is always unused and therefore superfluous.

The order of parameters is flipped in the function signature: dst, src VS os.Link(src, dst)

I therefore propose to simplify this call by invoking os.Link(src, dst) and returning the resulting error directly.

3. Logs the arguments executed on the shell in debug mode

When using the Debug log level, the final arguments passed to shell are now logged for easier troubleshooting.

This has been implemented in startVMM:

// startVMM starts the firecracker vmm process and configures logging.
func (m *Machine) startVMM(ctx context.Context) error {
	m.logger.Printf("Called startVMM(), setting up a VMM on %s", m.Cfg.SocketPath)
	startCmd := m.cmd.Start

	m.logger.Debug("[startVMM] arguments:", m.cmd.Args)
        ...

cc @ppartarr

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

[kzys]

Thanks! The code change itself looks good. Please add "Signed-off-by:" line on each commit. You can add the line by doing "git commit -s".

Also it will make sense to combine some commits (e.g. e4b889f + 61d91a7) if they belong to the same change.

[dreadl0ck]

Squashed commits e4b889f + 61d91a7 into a single one, preserving both commit messages:

git checkout 61d91a7a9164d1cb147dd6bbe7a20f4b29158561
git rebase --interactive 8549cd3a27a508d9ac1191ae611799dfa5c1da5b
git pull origin jailer-fixes
git push origin jailer-fixes

On the branch are now 8 commits: the 6 previous ones + 1 merge + 1 new squashed commit.
I rewrote the last 8 commits to be Signed-off-by you:

git checkout jailer-fixes
git config trailer.sign.key "Signed-off-by"
export SIGNOFF="Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>"
git filter-branch -f --msg-filter \
    "git interpret-trailers --trailer \"$SIGNOFF\"" \
    HEAD~8..HEAD
git push

It seems like something has gone wrong though, now there are 23 commits on the branch?

[kzys]

Sorry. The "signed-off" line is called Developers Certificate of Origin (DCO) and you should sign with your name, not mine. Helm's blog post would be helpful to understand the meaning.

I'm not so sure how did you get the 23 commits. You could clean up your branch by git rebase --interactive master and remove unnecessary/duplicate commits.

[dreadl0ck]

Thanks for clarifying!

I've cleaned up the pull request and changed the "signed-off" lines.

[kzys]

Awesome! Can you take a look this test failure? I think you may need to update the test.

[dreadl0ck]

Sure, I've updated the tests - seems fine now.

[kzys]

Sorry for the late review. The overall change looks good!

Regarding commits, I think you could organize them better. For example, 1ebbada undos a258d73. How about combining both of them together? The last three changes are all about tests. If you have a logical distinction between them, it would be better to combine them together.

http://gitforteams.com/resources/commit-granularity.html would be good read regarding the granularity of Git commits.

[dreadl0ck]

No problemo, I'll take this as a free git rebase masterclass ;-)

Combined the log message and test update commits into a single one!

[kzys]

Looks great. Thanks for the contribution!