Api server thread clean shutdown #3348

acatangiu · 2023-01-03T14:37:26Z

Changes

Use dedicated kill switch mechanism to break out of micro-http loop and gracefully shutdown the API server thread.

Remove hacky "/shutdown-internal" HTTP request which was externally exposing a way to bring down parts of firecracker components.

Dependencies

Ideally needs firecracker-microvm/micro-http#33 merged first, then point the dependency there.

License Acceptance

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

PR Checklist

All commits in this PR are signed (git commit -s).
~~If a specific issue led to this PR, this PR closes the issue.~~
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
~~New unsafe code is documented.~~
~~API changes follow the Runbook for Firecracker API changes.~~
~~User-facing changes are mentioned in CHANGELOG.md.~~
All added/changed functionality is tested.
~~New TODOs link to an issue.~~

~~This functionality can be added in rust-vmm.~~

JonathanWoollett-Light · 2023-02-28T09:29:30Z

Thank you for the contribution, looks like a good change.

xmarcalx · 2023-08-21T23:54:55Z

Hi @acatangiu ,

Thank you very much for your contribution and sorry for the late turnaround.
The PR needs to be rebased and @ShadowCurse will perform the review including firecracker-microvm/micro-http#33.

ShadowCurse · 2023-08-25T13:57:12Z

Hi @acatangiu do you want to rebase this PR on top of main or do you want me to do that?

acatangiu · 2023-08-25T13:58:36Z

Hi @acatangiu do you want to rebase this PR on top of main or do you want me to do that?

Yes, feel free to do it, thank you!

codecov · 2023-08-25T16:06:15Z

Codecov Report

Patch coverage: 20.00% and project coverage change: +0.02% 🎉

Comparison is base (af054d7) 82.26% compared to head (9e9df32) 82.28%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3348      +/-   ##
==========================================
+ Coverage   82.26%   82.28%   +0.02%     
==========================================
  Files         225      225              
  Lines       28466    28457       -9     
==========================================
- Hits        23417    23416       -1     
+ Misses       5049     5041       -8

Flag	Coverage Δ
4.14-c7g.metal	`77.68% <20.00%> (+0.02%)`	⬆️
4.14-m5d.metal	`79.59% <20.00%> (+0.02%)`	⬆️
4.14-m6a.metal	`78.69% <20.00%> (+0.02%)`	⬆️
4.14-m6g.metal	`77.68% <20.00%> (+0.02%)`	⬆️
4.14-m6i.metal	`79.57% <20.00%> (+0.02%)`	⬆️
5.10-c7g.metal	`80.60% <20.00%> (+0.02%)`	⬆️
5.10-m5d.metal	`82.25% <20.00%> (+0.02%)`	⬆️
5.10-m6a.metal	`81.46% <20.00%> (+0.02%)`	⬆️
5.10-m6g.metal	`80.60% <20.00%> (+0.02%)`	⬆️
5.10-m6i.metal	`82.24% <20.00%> (+0.02%)`	⬆️
6.1-c7g.metal	`80.60% <20.00%> (+0.02%)`	⬆️
6.1-m5d.metal	`82.25% <20.00%> (+0.02%)`	⬆️
6.1-m6a.metal	`81.46% <20.00%> (+0.02%)`	⬆️
6.1-m6g.metal	`80.60% <20.00%> (+0.02%)`	⬆️
6.1-m6i.metal	`82.24% <20.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/api_server/src/parsed_request.rs	`93.04% <ø> (-0.08%)`	⬇️
src/firecracker/src/api_server_adapter.rs	`0.00% <0.00%> (ø)`
src/api_server/src/lib.rs	`85.71% <100.00%> (+4.91%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/api_server/src/lib.rs

Added shutdown event-fd acting as a kill switch. Used to signal the api server thread that it needs to shut down. Signed-off-by: Egor Lazarchuk <[email protected]> Co-authored-by: acatangiu <[email protected]>

JonathanWoollett-Light

@ShadowCurse It is not clear to me why we need try_connect, what is the problem with using UnixStream::connect where we used it originally?

ShadowCurse · 2023-09-18T11:08:24Z

@JonathanWoollett-Light I did not change the PR. I just rebased it on top of main and fixed merge conflicts.
Considerring try_connect is only used in one test, I will remove it.

bchalios · 2023-09-18T11:24:04Z

Codecov Report
Patch coverage: 20.00% and project coverage change: -4.58% ⚠️

Is there anything we can do to fix this?

Use dedicated kill switch mechanism to break out of micro-http loop and gracefully shutdown the API server thread. Remove hacky "/shutdown-internal" HTTP request which was externally exposing a way to bring down parts of firecracker components. Signed-off-by: Egor Lazarchuk <[email protected]> Co-authored-by:: acatangiu <[email protected]>

acatangiu · 2023-09-18T14:34:03Z

@ShadowCurse It is not clear to me why we need try_connect, what is the problem with using UnixStream::connect where we used it originally?

Iirc I added this “try_connect” just for tests to make sure tests don’t sporadically fail based on different thread scheduling on different machines. At some point I know for sure tests would sporadically fail when test thread tried to connect to api thread before api thread had the chance to create the socket.

Not sure if this is still a problem anymore, but I would still keep “try_connect” as defensive programming.

JonathanWoollett-Light

lgtm

acatangiu changed the title ~~Api kill switch~~ Api server thread clean shutdown Jan 3, 2023

JonathanWoollett-Light added the Status: Blocked Indicates that an issue or pull request cannot currently be worked on label Feb 28, 2023

JonathanWoollett-Light self-assigned this Feb 28, 2023

JonathanWoollett-Light added Type: Enhancement Indicates new feature requests Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` labels Mar 24, 2023

ShadowCurse force-pushed the api-kill-switch branch 3 times, most recently from 96d8b6c to a79d06e Compare August 25, 2023 15:57

ShadowCurse force-pushed the api-kill-switch branch 4 times, most recently from 1ad413e to 30fbaf4 Compare August 25, 2023 17:18

ShadowCurse added Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed Status: Blocked Indicates that an issue or pull request cannot currently be worked on labels Aug 25, 2023

ShadowCurse force-pushed the api-kill-switch branch 4 times, most recently from a98a0d0 to cc2dea9 Compare September 4, 2023 08:52

wearyzen force-pushed the api-kill-switch branch from 4f3cd70 to 93738e3 Compare September 15, 2023 14:22

wearyzen previously approved these changes Sep 15, 2023

View reviewed changes

src/api_server/src/lib.rs Outdated Show resolved Hide resolved

ShadowCurse force-pushed the api-kill-switch branch from 93738e3 to 915fab8 Compare September 18, 2023 08:59

api_server: add kill switch capability

272ae95

Added shutdown event-fd acting as a kill switch. Used to signal the api server thread that it needs to shut down. Signed-off-by: Egor Lazarchuk <[email protected]> Co-authored-by: acatangiu <[email protected]>

ShadowCurse force-pushed the api-kill-switch branch from 915fab8 to 9d53e04 Compare September 18, 2023 09:01

JonathanWoollett-Light suggested changes Sep 18, 2023

View reviewed changes

ShadowCurse dismissed wearyzen’s stale review via 35e563a September 18, 2023 11:12

ShadowCurse force-pushed the api-kill-switch branch from 9d53e04 to 35e563a Compare September 18, 2023 11:12

ShadowCurse force-pushed the api-kill-switch branch from 35e563a to 2160cc2 Compare September 18, 2023 11:27

Merge branch 'main' into api-kill-switch

9e9df32

JonathanWoollett-Light approved these changes Sep 20, 2023

View reviewed changes

wearyzen approved these changes Sep 20, 2023

View reviewed changes

ShadowCurse merged commit 3156667 into firecracker-microvm:main Sep 20, 2023
3 of 5 checks passed

acatangiu deleted the api-kill-switch branch September 21, 2023 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api server thread clean shutdown #3348

Api server thread clean shutdown #3348

acatangiu commented Jan 3, 2023

JonathanWoollett-Light commented Feb 28, 2023

xmarcalx commented Aug 21, 2023 •

edited

Loading

ShadowCurse commented Aug 25, 2023

acatangiu commented Aug 25, 2023

codecov bot commented Aug 25, 2023 •

edited

Loading

JonathanWoollett-Light left a comment •

edited

Loading

ShadowCurse commented Sep 18, 2023

bchalios commented Sep 18, 2023

acatangiu commented Sep 18, 2023

JonathanWoollett-Light left a comment

Api server thread clean shutdown #3348

Api server thread clean shutdown #3348

Conversation

acatangiu commented Jan 3, 2023

Changes

Dependencies

License Acceptance

PR Checklist

JonathanWoollett-Light commented Feb 28, 2023

xmarcalx commented Aug 21, 2023 • edited Loading

ShadowCurse commented Aug 25, 2023

acatangiu commented Aug 25, 2023

codecov bot commented Aug 25, 2023 • edited Loading

Codecov Report

JonathanWoollett-Light left a comment • edited Loading

Choose a reason for hiding this comment

ShadowCurse commented Sep 18, 2023

bchalios commented Sep 18, 2023

acatangiu commented Sep 18, 2023

JonathanWoollett-Light left a comment

Choose a reason for hiding this comment

xmarcalx commented Aug 21, 2023 •

edited

Loading

codecov bot commented Aug 25, 2023 •

edited

Loading

JonathanWoollett-Light left a comment •

edited

Loading