Save Firecracker PID in Jailer's root directory #4442
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4442 +/- ##
==========================================
- Coverage 81.39% 81.39% -0.01%
==========================================
Files 243 243
Lines 29430 29431 +1
==========================================
Hits 23955 23955
- Misses 5475 5476 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
wearyzen
added
the
Status: Awaiting review
pb8o
reviewed
Feb 12, 2024
zulinx86
reviewed
Feb 12, 2024
pb8o
reviewed
Feb 12, 2024
added 4 commits
February 13, 2024 11:32
da92bf6 commit changed behaviour of the PID returned by the Jailer such that when passing the --daemonize option to Firecracker without the --new-ns-pid option, the Firecracker process will have a different PID than the Jailer. This makes it difficult to find the PID of Firecracker process from the system so, save the Firecracker PID in Jailer root directory as a reliable workaround to fetch Firecracker PID. With this change, Firecracker process's PID will always be available in Jailer's root directory regardless of whether new_pid_ns was set. Signed-off-by: Sudan Landge <sudanl@amazon.com>
Jailer now saves Firecracker PID in a file in Jailer's root directory so refactor the code to read the FC pid always from this file regardless of whether the new_pid_ns flag was set. Signed-off-by: Sudan Landge <sudanl@amazon.com>
For the case where Jailer is no daemonized but new_pid_ns flag is set, we won't be able to fetch Firecracker pid from the screen process and the right way to get Firecracker PID is by calling Microvm's self.firecracker_pid so, remove usage _screen_firecracker_pid. The change to kill screen process if screen_pid is valid is done to make the next commit more readable. Signed-off-by: Sudan Landge <sudanl@amazon.com>
Since Microvm's self.firecracker_pid is valid in all cases, use it to kill Firecracker in all cases. Use screen_pid to identify and kill if screen process was used. Make the logic, which detect if Firecracker PID was killed, common for all (daemonize or not) cases. Signed-off-by: Sudan Landge <sudanl@amazon.com>
pb8o
reviewed
Feb 13, 2024
added 2 commits
February 13, 2024 14:36
The code assumed that if daemonize is set to False for the Jailer then, new_pid_ns should also be set to False. This assumption makes it difficult to add tests which require "daemonize=False and new_pid_ns=True". So remove the assumption and call enable_console() to explicitly which explicitly explains what functionality is needed. Note: apart from setting daemonize and new_pid_ns to False, `help.enable_console()` also appends default boot args to the vm object but the testing calling enable_console() replace the boot args while calling basic_config(). Signed-off-by: Sudan Landge <sudanl@amazon.com>
Microvm kill() has the logic to detect if Firecracker was actually killed using the PID stored in the Jailers root directory but, not all combinations of Jailers daemonize/new_pid_ns flags are tested so add new a test to try all 4 use cases. Signed-off-by: Sudan Landge <sudanl@amazon.com>
pb8o
previously approved these changes
Feb 13, 2024
zulinx86
reviewed
Feb 13, 2024
There was a change in behaviour introduced in Jailer because of which killing the Jailer does not kill the Firecracker process. Highlight the change in behaviour and suggested workaround in the CHANGELOG and Jailer doc. Signed-off-by: Sudan Landge <sudanl@amazon.com>
zulinx86
approved these changes
Feb 13, 2024
pb8o
approved these changes
Feb 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
new_pid-nsflag was set.Reason
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
PR.
CHANGELOG.md.TODOs link to an issue.contribution quality standards.
rust-vmm.