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

shell to machine in delayed-shutdown state #465

Merged
merged 1 commit into from
Oct 31, 2018
Merged

shell to machine in delayed-shutdown state #465

merged 1 commit into from
Oct 31, 2018

Conversation

pekof
Copy link
Contributor

@pekof pekof commented Oct 26, 2018

Fix bug #461
ssh_info returns connection data if machine is still running

 Changes to be committed:
	modified:   include/multipass/virtual_machine.h
	modified:   src/daemon/daemon.cpp
@codecov
Copy link

codecov bot commented Oct 26, 2018

Codecov Report

Merging #465 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #465      +/-   ##
==========================================
- Coverage   66.65%   66.64%   -0.02%     
==========================================
  Files         141      141              
  Lines        5413     5414       +1     
==========================================
  Hits         3608     3608              
- Misses       1805     1806       +1
Impacted Files Coverage Δ
src/daemon/daemon.cpp 23.72% <0%> (ø) ⬆️
include/multipass/virtual_machine.h 0% <0%> (ø) ⬆️

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 9428d15...b88cff6. Read the comment docs.

Copy link
Collaborator

@Saviq Saviq left a comment

Choose a reason for hiding this comment

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

Hey @pekof, thanks for this! Allowing shelling in is one thing, but we also need to cancel the shutdown timer, otherwise the machine will go down from under the user's feet.

@pekof
Copy link
Contributor Author

pekof commented Oct 29, 2018

@Saviq if you think that's necessary I will do it.

My only issue is what's use case for delayed stop if shell or exec would cancel it.
I can imagine that one user might want machine stopped in 30 minutes and connect to it in meantime.

@townsend2010
Copy link
Contributor

@pekof, thanks for the contribution too!

The original reason we recommended cancelling the delayed shutdown was a case were snapcraft would do a build in a Multipass instance and then when it's done, it puts the instance in a delayed shutdown. At this point, a user shell's/exec's into the instance and does stuff and then all of a sudden, the instance is shutdown from underneath them.

However, after thinking about it, a user poking around a snapcraft instance is probably not the best thing to do and if they do, they are on their own. snapcraft is the true consumer of that instance and not the user.

So my recommendation is that the consumer of the instance should be the one to issue a multipass stop --cancel on the instance or the user who is subverting the consumer should be cognizant of what state the instance is in. Given that, shell and exec should not cancel a delayed shutdown.

@pekof
Copy link
Contributor Author

pekof commented Oct 30, 2018

@Saviq and @townsend2010 if I add commit that cancels delayed shutdown and you decide that that behavior is not desired you can still merge prior commit?
Or should I wait for decision?

Copy link
Collaborator

@Saviq Saviq left a comment

Choose a reason for hiding this comment

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

After chatting a bit I think we'll try and replicate the shutdown behaviour using wall to warn logged in users and preventing new shells and execs within a minute from shutdown or so.

We'll do this in a separate PR though.

bors r+

bors bot added a commit that referenced this pull request Oct 30, 2018
465: shell to machine in delayed-shutdown state r=Saviq a=pekof

Fix bug #461 
ssh_info returns connection data if machine is still running

Co-authored-by: pekof <[email protected]>
@townsend2010
Copy link
Contributor

Hmm, let's try this again...
bors r=saviq

bors bot added a commit that referenced this pull request Oct 30, 2018
465: shell to machine in delayed-shutdown state r=saviq a=pekof

Fix bug #461 
ssh_info returns connection data if machine is still running

Co-authored-by: pekof <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 30, 2018

Build failed

@townsend2010 townsend2010 merged commit 2c89d67 into canonical:master Oct 31, 2018
@pekof pekof deleted the ssh-info-delayed-shutdown branch October 31, 2018 13:32
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.

3 participants