Skip to content

Backport #4436 #4440

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
the diff snapshot on top of the base snapshot's memory file. This can be
done by setting the `mem_file_path` to the path of the pre-existing full
snapshot.
- [#4259](https://github.com./firecracker-microvm/firecracker/pull/4259):
Added a double fork mechanism in the Jailer to avoid setsid failures
occurred while running Jailer as the process group leader.
This changed the behaviour of Jailer and now the Firecracker process
will always have a different pid than the Jailer process.
[#4440](https://github.com./firecracker-microvm/firecracker/pull/4440)
Added a "Known Limitations" section in the Jailer docs to highlight
the above change in behaviour.

### Deprecated

Expand Down
8 changes: 8 additions & 0 deletions docs/jailer.md
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,14 @@ Note: default value for `<api-sock>` is `/run/firecracker.socket`.
logic associated with `--daemonize` runs towards the end, instead of the very
beginning. We are working on adding better logging capabilities.

### Known limitations

- When passing the --daemonize option to Firecracker without the --new-ns-pid
option, the Firecracker process will have a different pid than the Jailer
process. The suggested workaround to get Firecracker process's pid in this
case is using `--new-pid-ns` flag and read Firecracker's pid from the
`firecracker.pid` file present in the jailer's root directory.

## Caveats

- If all the cgroup controllers are bunched up on a single mount point using
Expand Down
14 changes: 14 additions & 0 deletions tests/framework/microvm.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,20 @@ def kill(self):
LOG.exception("Process not found: %d", self.firecracker_pid)
except FileNotFoundError:
LOG.exception("PID file not found")

# if microvm was spawned then check if it gets killed
if self._spawned:
# it is observed that we need to wait some time before
# checking if the process is killed.
time.sleep(1)
# filter ps results for the jailer's unique id
rc, stdout, stderr = utils.run_cmd(
f"ps aux | grep {self.jailer.jailer_id}"
)
# make sure firecracker was killed
assert (
rc == 0 and stderr == "" and stdout.find("firecracker") == -1
), f"Firecracker pid {self.firecracker_pid} was not killed as expected"
else:
# Killing screen will send SIGHUP to underlying Firecracker.
# Needed to avoid false positives in case kill() is called again.
Expand Down