o/snapstate: do not keep the state lock while removing snap files (#1…

…4399) * o/snapstate: do not keep the state lock while removing snap files Do not take the state lock when removing snap files, since it is not known how long the removal will take. We have observed a system in the field, where there were issues with I/O on the storage device and unlink() took significant time, this preventing snapd from functioning normally as the state lock was held. Related: SNAPDENG-26617 Signed-off-by: Maciej Borzecki <[email protected]> * overlord/snapstate: comment on peculiarities of snap dir removal and state lock Signed-off-by: Maciej Borzecki <[email protected]> --------- Signed-off-by: Maciej Borzecki <[email protected]>
canonical · Aug 23, 2024 · c660e25 · c660e25
1 parent 0eccb01
commit c660e25
Showing 1 changed file with 13 additions and 2 deletions.
diff --git a/overlord/snapstate/handlers.go b/overlord/snapstate/handlers.go
@@ -1163,7 +1163,10 @@ func (m *SnapManager) doMountSnap(t *state.Task, _ *tomb.Tomb) error {
 			return
 		}
 
-		// remove snap dir is idempotent so it's ok to always call it in the cleanup path
+		// remove snap dir is idempotent so it's ok to always call it in
+		// the cleanup path; make sure to hold a state lock to prevent
+		// conflicts when snaps sharing the same snap name are being
+		// installed/removed,
 		if err := m.backend.RemoveSnapDir(snapsup.placeInfo(), otherInstances); err != nil {
 			t.Errorf("cannot cleanup partial setup snap %q: %v", snapsup.InstanceName(), err)
 		}
@@ -1289,6 +1292,8 @@ func (m *SnapManager) undoMountSnap(t *state.Task, _ *tomb.Tomb) error {
 		return err
 	}
 
+	// make sure to hold a state lock to prevent conflicts when snaps
+	// sharing the same snap name are being installed/removed,
 	return m.backend.RemoveSnapDir(snapsup.placeInfo(), otherInstances)
 }
 
@@ -3494,16 +3499,20 @@ func (m *SnapManager) doDiscardSnap(t *state.Task, _ *tomb.Tomb) error {
 		}
 	}
 
-	pb := NewTaskProgressAdapterLocked(t)
+	pb := NewTaskProgressAdapterUnlocked(t)
 	typ, err := snapst.Type()
 	if err != nil {
 		return err
 	}
+
+	st.Unlock()
 	err = m.backend.RemoveSnapFiles(snapsup.placeInfo(), typ, nil, deviceCtx, pb)
+	st.Lock()
 	if err != nil {
 		t.Errorf("cannot remove snap file %q, will retry in 3 mins: %s", snapsup.InstanceName(), err)
 		return &state.Retry{After: 3 * time.Minute}
 	}
+
 	if len(snapst.Sequence.Revisions) == 0 {
 		if err = m.backend.RemoveContainerMountUnits(snapsup.containerInfo(), nil); err != nil {
 			return err
@@ -3539,6 +3548,8 @@ func (m *SnapManager) doDiscardSnap(t *state.Task, _ *tomb.Tomb) error {
 			return err
 		}
 
+		// make sure to hold a state lock to prevent conflicts when
+		// snaps sharing the same snap name are being installed/removed,
 		if err := m.backend.RemoveSnapDir(snapsup.placeInfo(), otherInstances); err != nil {
 			return fmt.Errorf("cannot remove snap directory: %v", err)
 		}