overlord: fix prerequisite base snap silently not being installed due to conflict #13589

zyga · 2024-02-14T08:49:13Z

This pull request contains a number of patches that should be dropped, followed by a currently-failing test that reproduces the scenario from https://warthogs.atlassian.net/browse/SNAPDENG-13768

As debugged by Ernest and Samuele earlier, the debug output of the test shows (in messages prefixed with @@@) that a conflict is detected and then subsequently ignored.

This branch serves as a playground for fixing the problem.

ernestl

I think we are ready to discuss solution options

ernestl · 2024-02-14T13:29:30Z

overlord/snapstate/handlers.go

@@ -345,6 +351,7 @@ func (m *SnapManager) installOneBaseOrRequired(t *state.Task, snapName string, c
 	if conflErr, ok := err.(*ChangeConflictError); ok {
 		// conflicted with an install in the same change, just skip
 		if conflErr.ChangeID == t.Change().ID() {


This was added 3 years ago already as part of this PR

This change works for the specific case described here, but is arguable dangerous in terms of the ability to causing silent failure.

One improvement would be to at least issue warning when this happens so that the reason for downstream failure is obvious to the user.

Could the conflict check perhaps take into account if the tasks associated with a conflicting snap was completed (e.g. after retries). From the description where A continues to install dependencies B, but still the conflict remains.. this superficially seems a little odd.

ernestl · 2024-02-14T13:36:45Z

overlord/snapstate/conflict.go

@@ -135,6 +141,7 @@ func changeIsSnapdDowngrade(st *state.State, chg *state.Change) (bool, error) {
 	// On older snapd's 'Version' might be empty, and in this case we assume
 	// that snapd is downgrading as we cannot determine otherwise.
 	if snapsup.Version == "" {
+		fmt.Printf("@@@ This is a snapd downgrade!!!\n")


This was introduced here.

The snapd snapsetup with version == "" comes from a previous snapd version changing to this code which is the later version. The changes to add the version to snapsetup and to add this check was merged on 23 and 30 June 2023.

overlord/snapstate/snapstate_update_test.go

The test sets up a scenario where a snap, upon refresh, needs to pull in core22 base snap, but the change incorrectly conflicts with a parallel refresh of snapd that is artifically crippled to not record its version attribute. Two new test-only snap IDs are added to fake store backend: - some-snap-with-new-base-id this snap uses core22 as base - snapd-without-version-id this snap creates a snapd without the version field. In fact the suffix -without-version-id causes this behavior for other snaps should we choose to add more. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

…sOnCore This is needed by the new non-test logic. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

As seen on https://paste.ubuntu.com/p/QmDxmh2Mvg/ Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

pedronis

thank you, one small remark

pedronis · 2024-02-15T13:09:00Z

overlord/snapstate/snapstate.go

+		if err != nil {
+			return fmt.Errorf("cannot get current snapd snap info: %v", err)
+		}
+		if snapsup.Version == "" {


we might want some kind of comment here:

// fill version to make snapd downgrade check not trigger ()

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Meulengracht

lgtm

… to conflict (snapcore#13589) * o/snapstate: add test for spurious snapd downgrade The test sets up a scenario where a snap, upon refresh, needs to pull in core22 base snap, but the change incorrectly conflicts with a parallel refresh of snapd that is artifically crippled to not record its version attribute. Two new test-only snap IDs are added to fake store backend: - some-snap-with-new-base-id this snap uses core22 as base - snapd-without-version-id this snap creates a snapd without the version field. In fact the suffix -without-version-id causes this behavior for other snaps should we choose to add more. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> * o/snapstate: add snap-setup to TestFinishRestartGeneratesSnapdWrappersOnCore This is needed by the new non-test logic. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> * o/snapstate: apply fix for snapd downgrade from Samuele As seen on https://paste.ubuntu.com/p/QmDxmh2Mvg/ Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> * o/snapstate: explain snapd version special logic Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> --------- Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

… to conflict (#13589) * o/snapstate: add test for spurious snapd downgrade The test sets up a scenario where a snap, upon refresh, needs to pull in core22 base snap, but the change incorrectly conflicts with a parallel refresh of snapd that is artifically crippled to not record its version attribute. Two new test-only snap IDs are added to fake store backend: - some-snap-with-new-base-id this snap uses core22 as base - snapd-without-version-id this snap creates a snapd without the version field. In fact the suffix -without-version-id causes this behavior for other snaps should we choose to add more. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> * o/snapstate: add snap-setup to TestFinishRestartGeneratesSnapdWrappersOnCore This is needed by the new non-test logic. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> * o/snapstate: apply fix for snapd downgrade from Samuele As seen on https://paste.ubuntu.com/p/QmDxmh2Mvg/ Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> * o/snapstate: explain snapd version special logic Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> --------- Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

… to conflict (snapcore#13589) * o/snapstate: add test for spurious snapd downgrade The test sets up a scenario where a snap, upon refresh, needs to pull in core22 base snap, but the change incorrectly conflicts with a parallel refresh of snapd that is artifically crippled to not record its version attribute. Two new test-only snap IDs are added to fake store backend: - some-snap-with-new-base-id this snap uses core22 as base - snapd-without-version-id this snap creates a snapd without the version field. In fact the suffix -without-version-id causes this behavior for other snaps should we choose to add more. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> * o/snapstate: add snap-setup to TestFinishRestartGeneratesSnapdWrappersOnCore This is needed by the new non-test logic. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> * o/snapstate: apply fix for snapd downgrade from Samuele As seen on https://paste.ubuntu.com/p/QmDxmh2Mvg/ Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> * o/snapstate: explain snapd version special logic Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> --------- Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga added ⛔ Blocked Skip spread Indicate that spread job should not run labels Feb 14, 2024

zyga requested review from ernestl and bboozzoo February 14, 2024 08:49

pedronis self-requested a review February 14, 2024 09:08

ernestl reviewed Feb 14, 2024

View reviewed changes

zyga added 3 commits February 14, 2024 23:15

o/snapstate: add snap-setup to TestFinishRestartGeneratesSnapdWrapper…

c91ad91

…sOnCore This is needed by the new non-test logic. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

o/snapstate: apply fix for snapd downgrade from Samuele

38d5b9d

As seen on https://paste.ubuntu.com/p/QmDxmh2Mvg/ Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga force-pushed the fix/downgrade-misdetection branch 3 times, most recently from 9fc2022 to 38d5b9d Compare February 15, 2024 12:55

zyga marked this pull request as ready for review February 15, 2024 13:02

pedronis approved these changes Feb 15, 2024

View reviewed changes

pedronis removed the ⛔ Blocked label Feb 15, 2024

pedronis added this to the 2.61 milestone Feb 15, 2024

o/snapstate: explain snapd version special logic

5907e7e

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga changed the title ~~overlord: failing test showing prerequisite base snap silently not being installed due to conflict~~ overlord: fix prerequisite base snap silently not being installed due to conflict Feb 15, 2024

Meulengracht approved these changes Feb 15, 2024

View reviewed changes

ernestl added the Squash-merge Please squash this PR when merging. label Feb 15, 2024

ernestl merged commit 47c4208 into snapcore:master Feb 15, 2024
43 checks passed

ernestl added the cherry-picked label Feb 15, 2024

ernestl mentioned this pull request Feb 15, 2024

release: 2.61.2 #13600

Merged

zyga deleted the fix/downgrade-misdetection branch February 17, 2024 08:20

ernestl mentioned this pull request Feb 18, 2024

release: 2.61.2 #13607

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlord: fix prerequisite base snap silently not being installed due to conflict #13589

overlord: fix prerequisite base snap silently not being installed due to conflict #13589

zyga commented Feb 14, 2024

ernestl left a comment

ernestl Feb 14, 2024

ernestl Feb 14, 2024 •

edited

pedronis left a comment

pedronis Feb 15, 2024

Meulengracht left a comment

overlord: fix prerequisite base snap silently not being installed due to conflict #13589

overlord: fix prerequisite base snap silently not being installed due to conflict #13589

Conversation

zyga commented Feb 14, 2024

ernestl left a comment

Choose a reason for hiding this comment

ernestl Feb 14, 2024

Choose a reason for hiding this comment

ernestl Feb 14, 2024 • edited

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis Feb 15, 2024

Choose a reason for hiding this comment

Meulengracht left a comment

Choose a reason for hiding this comment

ernestl Feb 14, 2024 •

edited