o/snapstate: account for remodeling when installing prereqs as well as updating them #14137

andrewphelpsj · 2024-06-27T15:03:28Z

In #14088 I broke tests/nested/manual/hybrid-remodel when attempting to fix a different bug. By moving the remodeling check, I accidentally omitted checking for remodeling when updating a prereq. This change makes sure that we check for remodeling when installing and updating prereqs.

…s updating them

pedronis

it feels that we need some matching unit test(s) to capture these behaviors, that would have avoided the regression being surface only by spread?

valentindavid · 2024-06-28T08:07:04Z

There are still lots of failures in spread tests related to remodel.

…hat might be getting updated during a remodel

MiguelPires

LGTM for the fix, but the function is a bit strangely named now. It'd be nice to clean it up a bit but not a blocker for this PR

MiguelPires · 2024-07-01T12:50:05Z

overlord/snapstate/handlers.go

@@ -346,28 +355,17 @@ func (m *SnapManager) installOneBaseOrRequired(t *state.Task, snapName string, c
 		}

 		// other kind of dependency, check if it's in progress
-		if ok, err := inProgress(snapName); err != nil {
+		if _, err := shouldAttemptInstall(snapName); err != nil {


This looks a bit weird now. It's inside a isInstalled case but we run a shouldAttemptInstall check? It's also inconsistent with the comment above it.
Tbh the original is also not great, since it has more logic to it than than just checking if the snap is running

I agree that the name isn't good. Unfortunately I haven't been able to come up with something better.

pedronis

wondering whether the code can't be simplified further now

pedronis · 2024-07-01T16:40:57Z

overlord/snapstate/handlers.go

@@ -346,28 +355,17 @@ func (m *SnapManager) installOneBaseOrRequired(t *state.Task, snapName string, c
 		}

 		// other kind of dependency, check if it's in progress
-		if ok, err := inProgress(snapName); err != nil {
+		if _, err := shouldAttemptInstall(snapName); err != nil {


do we actually need this code inside the "if" now? wouldn't the code outside the "if" do the same in practice?

No, it can be dropped here to just always return err, regardless if nil or not.

pedronis · 2024-07-01T16:41:36Z

overlord/snapstate/handlers.go

+		// downloads during a remodel happen prior to snap installation. thus,
+		// we cannot wait for snaps to be installed here. see remodelTasks for
+		// more information on how the tasks are ordered.
+		if deviceCtx.ForRemodeling() {
 			return false, nil
 		}

 		// snap is being installed, retry later


is this comment making sense in all cases now?

Updated to account for the refresh case

pedronis

did another pass, some questions/comments

pedronis · 2024-07-02T11:35:39Z

overlord/snapstate/handlers.go

+		// tasks are ordered by the remodeling code. specifically, all snap
+		// downloads during a remodel happen prior to snap installation. thus,
+		// we cannot wait for snaps to be installed here. see remodelTasks for
+		// more information on how the tasks are ordered.


wont' this be a problem also for updatePrereqIfOutdated code? doesn't it mean we should simply check this before all other checks and return early? instead of doing the check here?

I spoke to Miguel about this earlier, because I was also confused about the ordering of the original code:

Those are mutually exclusive, unless I'm misunderstanding the question. The first if runs when the prerequisite is a default provider and the rest runs if it's another type of dependency (base, snapd). So that checks doesn't run after updating. Also the updatePrereqIfOutdated has code to check if its already being updated so it would catch a conflict

Just to clarify, that comment was about the inProgress check vs the updatePrereqIfOutdated call which was the code snippet in the link. It wasn't about the remodel check

Hmm I see I was conflating them.

Then I misunderstood the question, my bad

pedronis · 2024-07-02T11:40:18Z

overlord/snapstate/handlers.go

-		// snap is being installed, retry later
-		return true, nil
+		// snap is either being installed or updated
+		return false, onInFlight


I'm not sure that return directly onInFlight is helpful here

I return onInFlight here to distinguish between when we are remodeling or when we aren't.

I reworked how this looks in the most recent commit. Since we don't actually want to go into updatePrereqIfOutdated during a remodel since it tries to reach out to the store (we should have already downloaded all the snaps needed for the remodel, cc @MiguelPires to make sure I'm not lying), I think we can just move the check for remodeling earlier in the function.

pedronis · 2024-07-02T11:40:56Z

overlord/snapstate/handlers.go

@@ -324,19 +324,28 @@ func (m *SnapManager) installOneBaseOrRequired(t *state.Task, snapName string, c
 		return nil, err
 	}

-	inProgress := func(snapName string) (bool, error) {
+	shouldAttemptInstall := func(snapName string) (bool, error) {


maybe then this name can go back to the old one? it's really a bit confusing for the usage inside if isInstalled

… want to update prereqs during a remodel anyways

MiguelPires

lgtm

MiguelPires · 2024-07-02T13:52:31Z

overlord/snapstate/handlers_prereq_test.go

@@ -403,6 +403,76 @@ func (s *prereqSuite) TestDoPrereqRetryWhenBaseInFlight(c *C) {
 	c.Check(chg.Status(), Equals, state.DoneStatus)
 }

+func (s *prereqSuite) TestDoPrereqNoRetryWhenBaseInFlightDuringRemodel(c *C) {


This could have other variants (skips when there's a install in progress, a possible update of a prereq, a missing base, etc) but maybe it's not worth it to add those right now since we want this merged soon

MiguelPires · 2024-07-02T13:54:12Z

overlord/snapstate/handlers.go

+		// tasks are ordered by the remodeling code. specifically, all snap
+		// downloads during a remodel happen prior to snap installation. thus,
+		// we cannot wait for snaps to be installed here. see remodelTasks for
+		// more information on how the tasks are ordered.


Then I misunderstood the question, my bad

pedronis

thank you

o/snapstate: account for remodeling when installing prereqs as well a…

6ae8f92

…s updating them

andrewphelpsj added the Run nested The PR also runs tests inluded in nested suite label Jun 27, 2024

andrewphelpsj closed this Jun 27, 2024

andrewphelpsj reopened this Jun 27, 2024

andrewphelpsj requested review from pedronis and MiguelPires June 27, 2024 15:03

pedronis reviewed Jun 27, 2024

View reviewed changes

o/snapstate: test that we do not wait for already installed prereqs t…

046dbf1

…hat might be getting updated during a remodel

MiguelPires approved these changes Jul 1, 2024

View reviewed changes

andrewphelpsj requested a review from pedronis July 1, 2024 15:29

pedronis added this to the 2.64 milestone Jul 1, 2024

pedronis added the ⚠ Critical High-priority stuff (e.g. to fix master) label Jul 1, 2024

pedronis reviewed Jul 1, 2024

View reviewed changes

andrewphelpsj requested a review from pedronis July 1, 2024 16:51

o/snapstate: flatten code and update comment to account for refresh

e0a62d6

pedronis reviewed Jul 2, 2024

View reviewed changes

o/snapstate: move check for remodeling to be earlier, since we do not…

5509871

… want to update prereqs during a remodel anyways

andrewphelpsj requested review from pedronis and MiguelPires July 2, 2024 13:28

MiguelPires approved these changes Jul 2, 2024

View reviewed changes

pedronis approved these changes Jul 2, 2024

View reviewed changes

alfonsosanchezbeato merged commit 015a2e4 into canonical:master Jul 2, 2024
47 of 51 checks passed

andrewphelpsj deleted the fix-hybrid-remodel branch July 2, 2024 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

o/snapstate: account for remodeling when installing prereqs as well as updating them #14137

o/snapstate: account for remodeling when installing prereqs as well as updating them #14137

andrewphelpsj commented Jun 27, 2024

pedronis left a comment

valentindavid commented Jun 28, 2024

MiguelPires left a comment

MiguelPires Jul 1, 2024

andrewphelpsj Jul 1, 2024

pedronis left a comment

pedronis Jul 1, 2024

andrewphelpsj Jul 1, 2024

pedronis Jul 1, 2024

andrewphelpsj Jul 1, 2024

pedronis left a comment

pedronis Jul 2, 2024

andrewphelpsj Jul 2, 2024

MiguelPires Jul 2, 2024

andrewphelpsj Jul 2, 2024

MiguelPires Jul 2, 2024

pedronis Jul 2, 2024

andrewphelpsj Jul 2, 2024

andrewphelpsj Jul 2, 2024 •

edited

Loading

pedronis Jul 2, 2024

MiguelPires left a comment

MiguelPires Jul 2, 2024

MiguelPires Jul 2, 2024

pedronis left a comment

o/snapstate: account for remodeling when installing prereqs as well as updating them #14137

o/snapstate: account for remodeling when installing prereqs as well as updating them #14137

Conversation

andrewphelpsj commented Jun 27, 2024

pedronis left a comment

Choose a reason for hiding this comment

valentindavid commented Jun 28, 2024

MiguelPires left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewphelpsj Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelPires left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

andrewphelpsj Jul 2, 2024 •

edited

Loading