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.

Sign up for GitHub

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

Jump to bottom

api: Add snap info to each task #13902

Closed
wants to merge 1 commit into from
Closed

api: Add snap info to each task #13902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions client/change.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ type Task struct {

SpawnTime time.Time `json:"spawn-time,omitempty"`
ReadyTime time.Time `json:"ready-time,omitempty"`

SnapName string `json:"snap-name,omitempty"`
InstanceName string `json:"instance-name,omitempty"`
Revision string `json:"revision,omitempty"`
Comment on lines +69 to +71
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't want to add this kind of data this way. We probably want to add support instead for something like api-data that we have for Changes. Also (even) if we do that the closest thing we can do that doesn't feel too forced would be to expose a list of instance names "affected-snaps" that matches the same concept in snapstate conflict logic, we would have to expose some of that to daemon

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately, how are the consumers expected to use this information? Say if a change touches more than one, would they list all the tasks, and then continue observing which ones for which snap are done?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bboozzoo consumer is expected to poll the associated refresh change, if it happens to include multiple snaps then filtering on the task level is needed.

Check the "Proposed snap-run during refresh UX flow" section in SD167.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see a video with what I have right now: https://warthogs.atlassian.net/browse/UDENG-2282 , in the most recent comment. As you can see, the idea is to have a progress bar for each snap being updated, and that requires to know how many tasks are assigned to that snap refresh, and how many are already done. Currently I'm presuming that there is one change per snap, so each change has only one snap in its snap_names property, and thus all the tasks belong to that snap. But if there are several snaps in the same change, then each progress bar will show the same progress, unless this patch (or something similar) is implemented, that allows me to separate the tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, here is an screen capture
imagen

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unclear that "snap-tasks" is a good name for it, so we don't have a good name, also it's not an obvious approach except is compact

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also the obvious way to attach the information in snap-tasks form would be when we set snap-names, but actually a Change can grow more tasks after that point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point... if the task list can grow, then it makes sense to include this info in the task.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but actually a Change can grow more tasks after that point

to reiterate on this, snap can pull prerequisites (like a new base) or a re-refresh may occur, at which point the task list will grow again, so any progress bar the user sees will need to be adjusted such that it doesn't jump backwards

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, as I said in the top, if it is guaranteed that, when doing a refresh after the user closes the application, only one snap is associated with each change, this won't be needed. I tried killing two applications "at the same time" using kill XXXXX && kill YYYYY, and still receive two changes, one per snap...

There is still another scenario where snap run of a certain snap is inhibited from running due to a normal refresh/auto-refresh that includes that snap and the progress UI should only be relevant to the snap that is being started and not the other snaps in the auto-refresh. It not just when running the snap after closing.

}

type TaskProgress struct {
Expand Down
15 changes: 9 additions & 6 deletions client/change_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func (cs *clientSuite) TestClientChangeError(c *check.C) {
"summary": "...",
"status": "Error",
"ready": true,
"tasks": [{"kind": "bar", "summary": "...", "status": "Error", "progress": {"done": 1, "total": 1}, "log": ["ERROR: something broke"]}],
"tasks": [{"kind": "bar", "summary": "...", "status": "Error", "progress": {"done": 1, "total": 1}, "log": ["ERROR: something broke"], "snap-name": "a_snap", "instance-name": "instance_value", "revision": "891"}],
"err": "error message"
}}`

Expand All @@ -119,11 +119,14 @@ func (cs *clientSuite) TestClientChangeError(c *check.C) {
Summary: "...",
Status: "Error",
Tasks: []*client.Task{{
Kind: "bar",
Summary: "...",
Status: "Error",
Progress: client.TaskProgress{Done: 1, Total: 1},
Log: []string{"ERROR: something broke"},
Kind: "bar",
Summary: "...",
Status: "Error",
Progress: client.TaskProgress{Done: 1, Total: 1},
Log: []string{"ERROR: something broke"},
SnapName: "a_snap",
InstanceName: "instance_value",
Revision: "891",
}},
Err: "error message",
Ready: true,
Expand Down
11 changes: 11 additions & 0 deletions daemon/api_general.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"github.com/snapcore/snapd/overlord/auth"
"github.com/snapcore/snapd/overlord/configstate/config"
"github.com/snapcore/snapd/overlord/devicestate"
"github.com/snapcore/snapd/overlord/snapstate"
"github.com/snapcore/snapd/overlord/state"
"github.com/snapcore/snapd/release"
"github.com/snapcore/snapd/sandbox"
Expand Down Expand Up @@ -336,6 +337,10 @@ type taskInfo struct {

SpawnTime time.Time `json:"spawn-time,omitempty"`
ReadyTime *time.Time `json:"ready-time,omitempty"`

SnapName string `json:"snap-name,omitempty"`
InstanceName string `json:"instance-name,omitempty"`
Revision string `json:"revision,omitempty"`
}

type taskInfoProgress struct {
Expand Down Expand Up @@ -381,6 +386,12 @@ func change2changeInfo(chg *state.Change) *changeInfo {
},
SpawnTime: t.SpawnTime(),
}
snapsup, err := snapstate.TaskSnapSetup(t)
if err == nil {
taskInfo.SnapName = snapsup.SnapName()
taskInfo.InstanceName = snapsup.InstanceName()
taskInfo.Revision = snapsup.Revision().String()
}
readyTime := t.ReadyTime()
if !readyTime.IsZero() {
taskInfo.ReadyTime = &readyTime
Expand Down