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

many: go into state.Retry state when unmounting a snap fails. #1087

Merged
merged 11 commits into from May 2, 2016

Conversation

@mvo5
Copy link
Collaborator

commented Apr 27, 2016

This is first iteration of adding more robustness to the snap removal. If the snap mount point is busy it will put the task into retry mode and snappy will keep retrying.

This is to get early feedback, I will work on adding some UI if its retrying ext.

@pedronis

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2016

where do we fail actually? already at Disable? or only at Stop? if the latter we could/should probably exchange the order (with the state engine behavior is the same, ultimately we will have done both)


// find change id of the remove
output := cli.ExecCommand(c, "snap", "changes")
id := regexp.MustCompile(`(?m)([0-9])+.*Doing.*Remove.*"`).FindStringSubmatch(output)[1]

This comment has been minimized.

Copy link
@niemeyer

niemeyer Apr 27, 2016

Contributor

The + needs to be inside the parenthesis, otherwise it will pick "2" rather than "12".

c.Check(err, check.IsNil)
<-ch

// ensure no changes are left in Doing state

This comment has been minimized.

Copy link
@niemeyer

niemeyer Apr 27, 2016

Contributor

Shouldn't that be above the channel receive? Otherwise the test will probably pass as a side effect of the channel receive, rather than snap remove blocking until done.

@@ -319,7 +319,10 @@ func (m *SnapManager) doDiscardSnap(t *state.Task, _ *tomb.Tomb) error {
pb := &TaskProgressAdapter{task: t}
err = m.backend.RemoveSnapFiles(ss.placeInfo(), pb)
if err != nil {
return err
st.Lock()
t.Errorf("cannot remove snap file: %s, retrying", err)

This comment has been minimized.

Copy link
@niemeyer

niemeyer Apr 27, 2016

Contributor

That might potentially end up a bit confusing depending on what is inside err, and it is also slightly misguiding in that it points out a retrying is in progress while it may take quite a while. I suggest:

"cannot remove snap file, will retry: %v"

@niemeyer

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2016

A couple of suggestions, and LGTM

@pedronis

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2016

@mvo5 in case you are wondering why my question, I wonder about the behavior if we reboot while in the wait for retry phase

@mvo5

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 28, 2016

@pedronis Good point, I switched the order of disable/stop around now, i.e. stop first, then disable. This should address the point for the reboot.

output, err := cli.ExecCommandErr("sudo", "snap", "remove", data.BasicBinariesSnapName)
c.Check(err, check.IsNil)
c.Check(output, testutil.Contains, `will retry: `)
<-ch

This comment has been minimized.

Copy link
@pedronis

pedronis Apr 28, 2016

Contributor

I would use some form of:
select {
case <- ch:
case <-time.After(2 * time.Minute):
fail with message about taking too long
}

lots of moving parts, better not to block on this forever

@pedronis

This comment has been minimized.

Copy link
Contributor

commented May 2, 2016

+1

@pedronis

This comment has been minimized.

Copy link
Contributor

commented May 2, 2016

retest this please

@mvo5 mvo5 merged commit 8269ff9 into snapcore:master May 2, 2016
4 checks passed
4 checks passed
Integration tests Success
Details
autopkgtest Success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.09%) to 79.233%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.