many: support refresh hold/unhold to API and CLI #12073

MiguelPires · 2022-08-24T17:13:21Z

40ee105 adds support for hold and unholding refreshes to the snapd REST API
bde4921 adds support for the snap refresh --hold/--unhold commands to the snap tool
13ebf7d adds a spread test to integration test this feature

pedronis

thank you, did a pass. mostly small things and trying to clarify messages and help

client/snap_op.go

tests/main/snap-refresh-hold/task.yaml

daemon/api_snaps.go

cmd/snap/cmd_snap_op.go

daemon/api_snaps.go

MiguelPires · 2022-09-01T13:26:10Z

@degville could you review the outputted text in this PR please? We'd like to make the help, output and error messages clear to users. They're in cmd/snap/cmd_snap_op.go and daemon/api_snaps.go

degville

Thanks for pinging me! I've made a few comments, with the main suggestion being for the principle help text - but it's nothing major. It's difficult to find the path of least cognitive resistance with hold and held. Please feel free to ignore or update accordingly.

degville · 2022-09-06T11:09:41Z

cmd/snap/cmd_snap_op.go

+Holds (--hold) specified for snaps are only applied to general refreshes ('snap 
+refresh' without specifying snaps and auto-refreshes), but snaps can still be
+refreshed in specific refreshes (i.e, 'snap refresh snap-a snap-b'). Using
+--hold without specifying any snaps will only hold auto-refreshes for all snaps.


I've tried tweaking this slightly to hopefully make our intentions clearer (I left the time specifics and syntax to the command options list):

Suggested change

Holds (--hold) specified for snaps are only applied to general refreshes ('snap

refresh' without specifying snaps and auto-refreshes), but snaps can still be

refreshed in specific refreshes (i.e, 'snap refresh snap-a snap-b'). Using

--hold without specifying any snaps will only hold auto-refreshes for all snaps.

Hold (--hold) is used to postpone general snap refresh updates for either all

snaps, or for the specified snaps. It is only effective with auto-refreshes and

refreshes that do not target snaps specifically. If a snap is first held but

then added to a refresh command, such as `snap refresh target-snap`, it will

still be refreshed.

@degville I think in the reformulation is lost that "snap refresh --hold" blocks only auto-refreshes and not "snap refresh" (no specific snap) itself.

Thanks @pedronis, I'd forgotten that it didn't block a general snap refresh. Does this make it clearer?

Suggested change

Holds (--hold) specified for snaps are only applied to general refreshes ('snap

refresh' without specifying snaps and auto-refreshes), but snaps can still be

refreshed in specific refreshes (i.e, 'snap refresh snap-a snap-b'). Using

--hold without specifying any snaps will only hold auto-refreshes for all snaps.

Hold (--hold) is used to postpone automatic snap refresh updates for all snaps

when no snaps are specified, or for the specified snaps. It is only effective

with auto-refreshes and will not block either general refresh requests from

`snap refresh` or specific snap requests from `snap refresh target-snap`.

It does block general refreshes for specific snaps. So I still don't think I read that in the new version. Maybe the easiest is really to have two paragraphs, one about "snap refresh --hold" and a separate one about "snap refresh --hold <snap>"?

I tried to split into a general/intro paragraph and then one paragraph about when no snaps are specified and one for snaps being specified.

cmd/snap/cmd_snap_op.go

pedronis · 2022-09-06T12:05:39Z

thanks @degville, I'll review your comments first and then @MiguelPires we'll see how to proceed

pedronis

the summaries for the remove cases can probably be made to match more @degville suggestions for the cmd/snap ones

pedronis · 2022-09-06T14:52:38Z

daemon/api_snaps.go

+		}
+
+		tss = []*state.TaskSet{ts}
+		msg = i18n.G("Remove hold on auto-refreshes of all snaps.")


this probably can be changed to be a bit more like the new cmd/snap message

pedronis · 2022-09-06T14:52:44Z

daemon/api_snaps.go

+			return nil, err
+		}
+
+		msg = fmt.Sprintf(i18n.G("Remove hold on general refreshes of %s."), strutil.Quoted(inst.Snaps))


pedronis · 2022-09-07T09:52:24Z

marking blocked as while the command behavior matches our knoewn use cases and precedent, we likely want first to make the API itself less implicit/ambiguous

I talked about this with @MiguelPires

pedronis · 2022-10-12T16:45:23Z

I rebased this on master and switched it to use the new API. Next step is adding a level parameter to the hold APIs themselves

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>

it can have values "auto-refresh" or "general" matching snapstate.HoldAutoRefresh or snapstate.HoldGeneral holding general refreshes for all snaps is not supported atm

@degville

…ssages thanks @degville

degville · 2022-10-18T15:14:33Z

cmd/snap/cmd_snap_op.go

+When specific snaps are mentioned --hold is effective on their auto-refreshes
+and will also silently block general refresh requests from 'snap refresh' of
+those snaps while explicit targeted 'snap refresh target-snap' will not be
+blocked.


@pedronis Thanks for thinking about this - I much prefer your split approach, which I think is far easier to understand. It all looks good but I've tried to clarify the final paragraph slightly by copying the terminology and structure of the previous paragraph.

Suggested change

When specific snaps are mentioned --hold is effective on their auto-refreshes

and will also silently block general refresh requests from 'snap refresh' of

those snaps while explicit targeted 'snap refresh target-snap' will not be

blocked.

When snaps are specified --hold is effective on both their auto-refreshes

and general refresh requests from `snap refresh`. However, specific snap

requests from 'snap refresh target-snap' remain unblocked and will proceed.

MiguelPires

The recent changes LGTM

mvo5

Thanks, this is fine and my nitpicks can be follwups

mvo5 · 2022-10-24T09:45:07Z

cmd/snap/cmd_snap_op.go

+		changeID, err = x.client.HoldRefreshesMany(names, &opts)
+	}
+
+	if err != nil {


(super)nitpick. I would remove the newline so that the err check is as close as possible to the branches that generate the errors.

mvo5 · 2022-10-24T09:45:51Z

cmd/snap/cmd_snap_op.go

+	names := installedSnapNames(x.Positional.Snaps)
+	var changeID string
+	opts.HoldLevel = "general"
+	if len(names) == 0 {


(nitpick^3) I wonder if this would looks nicer as a switch ?

mvo5 · 2022-10-24T09:52:02Z

daemon/api_snaps.go

+		if inst.Time == "" {
+			return errors.New("hold action requires a non-empty time value")
+		} else if inst.Time != "forever" {
+			_, err := time.Parse(time.RFC3339, inst.Time)


(nitpick) this could be a sinfle line: if _, err := time...; err != nil {

mvo5 · 2022-10-24T11:36:57Z

Looking at the spread tests I see one issue that looks related to this PR:

2022-10-17T20:56:21.7279655Z 2022-10-17 20:56:21 Executing google:ubuntu-18.04-64:tests/main/parallel-install-basic (oct172016-343438) (312/606)...
2022-10-17T20:56:22.9477219Z ##[error]2022-10-17 20:56:22 Error executing google:ubuntu-18.04-64:tests/main/snap-refresh-hold (oct172016-343420) : 
2022-10-17T20:56:22.9485780Z -----
2022-10-17T20:56:22.9486494Z + echo 'No snaps auto-refresh in an all-snaps hold'
2022-10-17T20:56:22.9487098Z No snaps auto-refresh in an all-snaps hold
2022-10-17T20:56:22.9487641Z + snap refresh --hold=forever
2022-10-17T20:56:22.9488168Z Auto-refresh of all snaps held until forever
2022-10-17T20:56:22.9488573Z + systemctl stop snapd.service snapd.socket
2022-10-17T20:56:22.9489300Z + /home/gopath/src/github.com/snapcore/snapd/tests/lib/tools/snapd-state change-snap-channel test-snapd-tools edge
2022-10-17T20:56:22.9490111Z + /home/gopath/src/github.com/snapcore/snapd/tests/lib/tools/snapd-state force-autorefresh
2022-10-17T20:56:22.9490639Z + systemctl start snapd.socket snapd.service
2022-10-17T20:56:22.9491263Z + retry -n 5 --quiet sh -c 'snap changes | tail -2 | grep "Done.*Auto-refresh"'
2022-10-17T20:56:22.9496048Z + echo 'Snap doesn'\''t auto-refresh with a specific hold'
2022-10-17T20:56:22.9496665Z Snap doesn't auto-refresh with a specific hold
2022-10-17T20:56:22.9497002Z + reset
2022-10-17T20:56:22.9497761Z + snap refresh --channel=latest/stable test-snapd-tools
2022-10-17T20:56:22.9498516Z test-snapd-tools 1.0 from Test snaps (test-snaps-canonical) refreshed
2022-10-17T20:56:22.9499103Z + snap refresh --channel=latest/stable test-snapd-tools_instance
2022-10-17T20:56:22.9499670Z snap "test-snapd-tools_instance" has no updates available
2022-10-17T20:56:22.9500205Z + snap refresh --unhold test-snapd-tools
2022-10-17T20:56:22.9501225Z Removed general refresh hold of "test-snapd-tools"
2022-10-17T20:56:22.9501679Z + snap refresh --unhold
2022-10-17T20:56:22.9502996Z Removed auto-refresh hold on all snaps
2022-10-17T20:56:22.9503590Z + snap refresh --hold test-snapd-tools
2022-10-17T20:56:22.9590270Z General refreshes of "test-snapd-tools" held until forever
2022-10-17T20:56:22.9590841Z + systemctl stop snapd.service snapd.socket
2022-10-17T20:56:22.9591627Z + /home/gopath/src/github.com/snapcore/snapd/tests/lib/tools/snapd-state change-snap-channel test-snapd-tools edge
2022-10-17T20:56:22.9592450Z + /home/gopath/src/github.com/snapcore/snapd/tests/lib/tools/snapd-state change-snap-channel test-snapd-tools_instance edge
2022-10-17T20:56:22.9593273Z + /home/gopath/src/github.com/snapcore/snapd/tests/lib/tools/snapd-state force-autorefresh
2022-10-17T20:56:22.9593806Z + systemctl start snapd.socket snapd.service
2022-10-17T20:56:22.9594569Z + retry -n 5 --quiet sh -c 'snap changes | tail -2 | grep "Done.*Auto-refresh snap "test-snapd-tools_instance""'
2022-10-17T20:56:22.9595138Z + echo 'Snaps auto-refresh without holds'
2022-10-17T20:56:22.9595616Z Snaps auto-refresh without holds
2022-10-17T20:56:22.9595956Z + reset
2022-10-17T20:56:22.9596364Z + snap refresh --channel=latest/stable test-snapd-tools
2022-10-17T20:56:22.9614942Z test-snapd-tools 1.0 from Test snaps (test-snaps-canonical) refreshed
2022-10-17T20:56:22.9615885Z + snap refresh --channel=latest/stable test-snapd-tools_instance
2022-10-17T20:56:22.9616485Z error: snap "test-snapd-tools_instance" has "auto-refresh" change in progress
2022-10-17T20:56:22.9616919Z -----

pedronis · 2022-10-24T15:58:46Z

2022-10-17T20:56:22.9596364Z + snap refresh --channel=latest/stable test-snapd-tools
2022-10-17T20:56:22.9614942Z test-snapd-tools 1.0 from Test snaps (test-snaps-canonical) refreshed
2022-10-17T20:56:22.9615885Z + snap refresh --channel=latest/stable test-snapd-tools_instance
2022-10-17T20:56:22.9616485Z error: snap "test-snapd-tools_instance" has "auto-refresh" change in progress

it seems the auto-refresh for which we check above is still in progress, is the grep check wrong? or do we have a very unexpected race?

A missing ! was preventing the test from failing if the auto-refresh for test-snapd-tools_instance wasn't completed by the end of retry. This commit also adds another check to ensure that the other snap isn't auto-refreshed. Since this bug was discovered because the auto-refresh was still ongoing after the retry, it's possible that even with the fix more retries are necessary so I doubled the number. Signed-off-by: Miguel Pires <miguel.pires@canonical.com>

mvo5

Thank you!

MiguelPires force-pushed the refresh-hold-daemon branch from 060e468 to 13ebf7d Compare August 24, 2022 17:52

MiguelPires requested a review from pedronis August 25, 2022 08:23

pedronis reviewed Aug 31, 2022

View reviewed changes

MiguelPires requested a review from degville September 1, 2022 13:17

MiguelPires mentioned this pull request Sep 2, 2022

many: hold and resume refreshes #11907

Closed

degville approved these changes Sep 6, 2022

View reviewed changes

pedronis self-requested a review September 6, 2022 12:05

pedronis reviewed Sep 6, 2022

View reviewed changes

pedronis self-requested a review September 6, 2022 17:30

pedronis added the ⛔ Blocked label Sep 7, 2022

pedronis self-assigned this Oct 11, 2022

pedronis force-pushed the refresh-hold-daemon branch from b1aacfc to 757f30e Compare October 12, 2022 16:44

MiguelPires and others added 8 commits October 17, 2022 21:55

daemon: support refresh hold/unhold in API

fdd74fa

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>

cmd/snap: support refresh hold/unhold in CLI

7bff877

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>

tests: spread test holding and unhold refreshes

968bc3e

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>

many: improve output messages

229c406

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>

tests: extend refresh hold spread test checks

c5dbb08

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>

daemon: introduce a mandatory hold-level parameter for hold ops

d10a3a5

it can have values "auto-refresh" or "general" matching snapstate.HoldAutoRefresh or snapstate.HoldGeneral holding general refreshes for all snaps is not supported atm

client: support providing hold-level

78ce1ae

many: pass hold-level for refresh --hold, clarify/cleanup help and me…

2f8f540

…ssages thanks @degville

pedronis force-pushed the refresh-hold-daemon branch from 757f30e to 2f8f540 Compare October 17, 2022 19:57

pedronis removed the ⛔ Blocked label Oct 17, 2022

degville reviewed Oct 18, 2022

View reviewed changes

MiguelPires commented Oct 19, 2022

View reviewed changes

pedronis approved these changes Oct 19, 2022

View reviewed changes

pedronis added this to the 2.58 milestone Oct 20, 2022

mvo5 approved these changes Oct 24, 2022

View reviewed changes

mvo5 approved these changes Oct 25, 2022

View reviewed changes

mvo5 merged commit f73eb23 into snapcore:master Oct 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: support refresh hold/unhold to API and CLI #12073

many: support refresh hold/unhold to API and CLI #12073

MiguelPires commented Aug 24, 2022 •

edited

pedronis left a comment

MiguelPires commented Sep 1, 2022

degville left a comment

degville Sep 6, 2022

pedronis Sep 6, 2022

degville Sep 6, 2022

pedronis Sep 7, 2022 •

edited

pedronis Oct 17, 2022

pedronis commented Sep 6, 2022

pedronis left a comment

pedronis Sep 6, 2022

pedronis Sep 6, 2022

pedronis commented Sep 7, 2022 •

edited

pedronis commented Oct 12, 2022

degville Oct 18, 2022

MiguelPires left a comment •

edited

mvo5 left a comment

mvo5 Oct 24, 2022

mvo5 Oct 24, 2022

mvo5 Oct 24, 2022

mvo5 commented Oct 24, 2022

pedronis commented Oct 24, 2022

mvo5 left a comment

-Holds (--hold) specified for snaps are only applied to general refreshes ('snap
-refresh' without specifying snaps and auto-refreshes), but snaps can still be
-refreshed in specific refreshes (i.e, 'snap refresh snap-a snap-b'). Using
---hold without specifying any snaps will only hold auto-refreshes for all snaps.
+Hold (--hold) is used to postpone general snap refresh updates for either all
+snaps, or for the specified snaps. It is only effective with auto-refreshes and
+refreshes that do not target snaps specifically. If a snap is first held but
+then added to a refresh command, such as `snap refresh target-snap`, it will
+still be refreshed.

many: support refresh hold/unhold to API and CLI #12073

many: support refresh hold/unhold to API and CLI #12073

Conversation

MiguelPires commented Aug 24, 2022 • edited

pedronis left a comment

Choose a reason for hiding this comment

MiguelPires commented Sep 1, 2022

degville 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

pedronis Sep 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis commented Sep 6, 2022

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

pedronis commented Sep 7, 2022 • edited

pedronis commented Oct 12, 2022

Choose a reason for hiding this comment

MiguelPires left a comment • edited

Choose a reason for hiding this comment

mvo5 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

mvo5 commented Oct 24, 2022

pedronis commented Oct 24, 2022

mvo5 left a comment

Choose a reason for hiding this comment

MiguelPires commented Aug 24, 2022 •

edited

pedronis Sep 7, 2022 •

edited

pedronis commented Sep 7, 2022 •

edited

MiguelPires left a comment •

edited