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.

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

Fix crash if exception thrown during daemon startup #487

Merged
merged 1 commit into from
Nov 13, 2018

Conversation

gerboland
Copy link
Contributor

The "system" loggers have different lifecycle to the client loggers.
Client loggers register/unregister themselves with the
MultiplexingLogger on creation/destruction. However "system" loggers did
not, and would leave a dangling pointer in the MultiplexingLogger if
they were destroyed before it was.

Fix is to treat system logger ownership differently from client, i.e.
have MultiplexingLogger own the system logger.

Crash without this patch can be easily reproduced by adding a throw in
the QemuVirtualMachineFactory constructor.

The "system" loggers have different lifecycle to the client loggers.
Client loggers register/unregister themselves with the
MultiplexingLogger on creation/destruction. However "system" loggers did
not, and would leave a dangling pointer in the MultiplexingLogger if
they were destroyed before it was.

Fix is to treat system logger ownership differently from client, i.e.
have MultiplexingLogger own the system logger.

Crash without this patch can be easily reproduced by adding a throw in
the QemuVirtualMachineFactory constructor.
@gerboland
Copy link
Contributor Author

Pet peeve: if you run multipassd via command line, no errors are printed to stdout. Can we do something about that?

@gerboland
Copy link
Contributor Author

OSX CI failed, think infrastructure problem

@Saviq
Copy link
Collaborator

Saviq commented Nov 12, 2018

Pet peeve: if you run multipassd via command line, no errors are printed to stdout. Can we do something about that?

$ multipassd --help
Usage: ./build/bin/multipassd [options]
multipass service daemon

Options:
  -h, --help                                  Displays this help.
  -v, --version                               Displays version information.
  --logger <platform|stderr>                  specifies which logger to use
  -V, --verbosity <error|warning|info|debug>  specifies the logging verbosity
                                              level
  --address <server_name:port>                specifies which address to use
                                              for the multipassd service; a
                                              socket can be specified using
                                              unix:<socket_file>

@codecov
Copy link

codecov bot commented Nov 12, 2018

Codecov Report

Merging #487 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #487      +/-   ##
=========================================
- Coverage   66.51%   66.5%   -0.01%     
=========================================
  Files         147     147              
  Lines        5626    5628       +2     
=========================================
+ Hits         3742    3743       +1     
- Misses       1884    1885       +1
Impacted Files Coverage Δ
include/multipass/logging/multiplexing_logger.h 100% <ø> (ø) ⬆️
src/logging/multiplexing_logger.cpp 92.3% <100%> (-7.7%) ⬇️
src/daemon/daemon_config.cpp 50% <100%> (-0.95%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c95ada...76927c4. Read the comment docs.

Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

Just a little inline thought I had, but not going to block on it.

But I'll wait on your feedback before having telling the bot I approve.

src/logging/multiplexing_logger.cpp Show resolved Hide resolved
@gerboland
Copy link
Contributor Author

@Saviq that's not an error. (With this patch) If an exception is thrown, the exception text is only sent to the journal, not stdout/stderr.

@townsend2010
Copy link
Contributor

Hmm, I see the exception text on stderr when starting multipassd like multipassd --logger stderr...

@townsend2010
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Nov 13, 2018
487: Fix crash if exception thrown during daemon startup r=townsend2010 a=gerboland

The "system" loggers have different lifecycle to the client loggers.
Client loggers register/unregister themselves with the
MultiplexingLogger on creation/destruction. However "system" loggers did
not, and would leave a dangling pointer in the MultiplexingLogger if
they were destroyed before it was.

Fix is to treat system logger ownership differently from client, i.e.
have MultiplexingLogger own the system logger.

Crash without this patch can be easily reproduced by adding a throw in
the QemuVirtualMachineFactory constructor.

Co-authored-by: Gerry Boland <[email protected]>
@albaguirre
Copy link
Contributor

Ohhh I see why. I failed to account for something in mp::DaemonConfigBuilder::build() throwing before DaemonConfig is actually constructed.

@albaguirre
Copy link
Contributor

@Saviq that's not an error. (With this patch) If an exception is thrown, the exception text is only sent to the journal, not stdout/stderr.

You want to look at daemon_main. I changed the exception handler to use multipass::log, you could double down and also use fmt::print(stderr, "....").

The init sequence is a little trick - that's why the logger is initialized as soon as possible, so it goes to the correct places - if multipass::log is called before a logger is in place, by default will send it to stderr.

@bors
Copy link
Contributor

bors bot commented Nov 13, 2018

Build succeeded

@bors bors bot merged commit 76927c4 into master Nov 13, 2018
@bors bors bot deleted the fix-startup-exception-logger branch November 13, 2018 17:08
@Saviq Saviq mentioned this pull request Dec 20, 2018
bors bot added a commit that referenced this pull request Dec 20, 2018
551: Release 2018.12.1 r=townsend2010 a=Saviq

### Highlights

- On Linux, suspending/resuming the instance to/from disk is now supported. (#374)
- Better handling of delayed shutdown including posting `wall` messages to logged in users and allowing log ins to the instance unless 1 minute or less remains until shutdown. (#461, #50) 
- On Linux, all CPU flags should be passed into the running instance on newly created instances. (#516)
- Fixed some races around mount handling. (#514, #520)

### Bugs fixed:

- make the recover command idempotent (#528)
- explicitly stop mounts when deleting an instance to avoid a race (#520)
- be smarter about what group owns the multipass socket (#513, #523) 
- pass through all CPU flags when launching QEMU or libvirt instances (#516)
- use `info` log level for metrics issues (#515)
- fix potential race when starting a mount (#514)
- use `wall` shutdown messages for users logged into VM when delayed shutdown is initiated (#501)
- fix crash if exception during daemon start up (#487)
- refactor CLI code (#468)
- add default uid/gid mapping (#331)
- fix file metadata passthrough
- display uid/gid maps in info command (#439)
- add support for the suspend command (#374)
- shell to machine in delayed stop state (#461)
- improve uid/gid validation (#479)
- avoid leaking the libvirt bridge (#327, #413)
- add a restart command (#217)
- upgrade 3rd-party versions (#471)

Co-authored-by: bors[bot] <bors[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants