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.

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

overlord,usersession: initial notifications of pending refreshes #9446

Merged
merged 29 commits into from
Oct 26, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2de348f
usersession: add pending refresh notice API
zyga Sep 30, 2020
0e7e301
overlord/snapstate: notify about postponed snap refresh
zyga Sep 30, 2020
f0b40bc
usersession: add missing i18n.G of notification text
zyga Oct 1, 2020
e9eb7f1
usersession: remove debug printf
zyga Oct 1, 2020
6066d68
usersession: remove ineffective ExpireTimeout
zyga Oct 1, 2020
1be2b1a
overlord/snapstate: send notifications asynchronously
zyga Oct 1, 2020
6c5ae0c
overlord: store snapInfo in BusySnapError
zyga Oct 12, 2020
65d8eeb
overlord: convert populateRefreshHints to method on BusySnapError
zyga Oct 12, 2020
db810ed
overlord: add TODO about missing tests
zyga Oct 12, 2020
99295dd
overlord: make asyncPendingRefreshNotification mockable
zyga Oct 12, 2020
2a36c49
overlord: test dispatch of desktop notifications
zyga Oct 12, 2020
670b01a
overlord: test non-overdue refresh notification
zyga Oct 12, 2020
b6be078
Merge remote-tracking branch 'origin/master' into feature/notify-on-p…
zyga Oct 12, 2020
24dbf7c
Merge remote-tracking branch 'origin/master' into feature/notify-on-p…
zyga Oct 13, 2020
bf12216
Merge remote-tracking branch 'origin/master' into feature/notify-on-p…
zyga Oct 16, 2020
9981312
usersession: use refreshInfo to refer to PendingSnapRefreshInfo
zyga Oct 22, 2020
59d3475
usersession: add smoke test for PendingRefreshNotification client call
zyga Oct 22, 2020
c9e348d
usersession: use SnapDesktopFilesDir over custom constant
zyga Oct 22, 2020
12434b8
usersession: drop TODO about snap-store
zyga Oct 22, 2020
b2fdb1f
usersession: explain TODO better
zyga Oct 22, 2020
48c80b4
usersession: add tests for /v1/notifications/pending-refresh
zyga Oct 22, 2020
17eb9e7
overlord/snapstate: use test counter, not flag
zyga Oct 22, 2020
f680c1e
overlord/snapstate: explain some test measurements
zyga Oct 22, 2020
31dec23
overlord/snapstate: use osutil.FileExists
zyga Oct 22, 2020
c487d86
usersession: tweak variable declaration
zyga Oct 22, 2020
3d43e83
usersession: do not close shared D-Bus connection
zyga Oct 23, 2020
4e68782
usersession: use io.snapcraft.SessionAgent desktop-entry hint
zyga Oct 23, 2020
268557e
usersession: re-factor i18n handling to avoid repetition
zyga Oct 23, 2020
20f559a
usersession: add TODO for sharing DBus connection
zyga Oct 23, 2020
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
40 changes: 40 additions & 0 deletions overlord/snapstate/autorefresh.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@
package snapstate

import (
"context"
"fmt"
"os"
"path/filepath"
"strings"
"time"

"github.com/snapcore/snapd/httputil"
Expand All @@ -35,6 +38,7 @@ import (
"github.com/snapcore/snapd/strutil"
"github.com/snapcore/snapd/timeutil"
"github.com/snapcore/snapd/timings"
userclient "github.com/snapcore/snapd/usersession/client"
)

// the default refresh pattern
Expand Down Expand Up @@ -489,21 +493,46 @@ func getTime(st *state.State, timeKey string) (time.Time, error) {
return t1, nil
}

func populateRefreshHints(refreshInfo *userclient.PendingSnapRefreshInfo, snapInfo *snap.Info, err *BusySnapError) {
for _, appName := range err.AppNames() {
if app, ok := snapInfo.Apps[appName]; ok {
path := app.DesktopFile()
if _, err := os.Stat(path); err == nil {
refreshInfo.BusyAppName = appName
refreshInfo.BusyAppDesktopEntry = strings.SplitN(filepath.Base(path), ".", 2)[0]
Copy link
Member

Choose a reason for hiding this comment

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

is this robust enough because DesktopFile will always end in ".desktop" and we don't allow "." in the names of desktop files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The desktop file is generated by us so yes. I think this is sensible.

}
}
}
}

// inhibitRefresh returns an error if refresh is inhibited by running apps.
//
// Internally the snap state is updated to remember when the inhibition first
// took place. Apps can inhibit refreshes for up to "maxInhibition", beyond
// that period the refresh will go ahead despite application activity.
func inhibitRefresh(st *state.State, snapst *SnapState, info *snap.Info, checker func(*snap.Info) error) error {
if err := checker(info); err != nil {
refreshInfo := &userclient.PendingSnapRefreshInfo{
InstanceName: info.InstanceName(),
}
if err, ok := err.(*BusySnapError); ok {
populateRefreshHints(refreshInfo, info, err)
}

days := int(maxInhibition.Truncate(time.Hour).Hours() / 24)
now := time.Now()
client := userclient.New()
if snapst.RefreshInhibitedTime == nil {
// Store the instant when the snap was first inhibited.
// This is reset to nil on successful refresh.
snapst.RefreshInhibitedTime = &now
Set(st, info.InstanceName(), snapst)
if _, ok := err.(*BusySnapError); ok {
refreshInfo.TimeRemaining = (maxInhibition - now.Sub(*snapst.RefreshInhibitedTime)).Truncate(time.Second)
if err := client.PendingRefreshNotification(context.TODO(), refreshInfo); err != nil {
zyga marked this conversation as resolved.
Show resolved Hide resolved
logger.Noticef("Cannot send notification about pending refresh: %v", err)
}
// XXX: remove the warning or send it only if no notification was delivered?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think we should remove the warning. Fwiw, I think the warning here is really a bit of a misfeature because we cannot make it go away once the app was refreshed. But it's fine to keep the XXX and do that in a followup. Especially this first warning that just warns for the first time seems a bit unneeded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree but I'd like to remove them in a follow up.

st.Warnf(i18n.NG(
"snap %q is currently in use. Its refresh will be postponed for up to %d day to wait for the snap to no longer be in use.",
"snap %q is currently in use. Its refresh will be postponed for up to %d days to wait for the snap to no longer be in use.", days),
Expand All @@ -515,9 +544,20 @@ func inhibitRefresh(st *state.State, snapst *SnapState, info *snap.Info, checker
if now.Sub(*snapst.RefreshInhibitedTime) < maxInhibition {
// If we are still in the allowed window then just return
// the error but don't change the snap state again.
refreshInfo.TimeRemaining = (maxInhibition - now.Sub(*snapst.RefreshInhibitedTime)).Truncate(time.Second)
if err := client.PendingRefreshNotification(context.TODO(), refreshInfo); err != nil {
logger.Noticef("Cannot send notification about pending refresh: %v", err)
}
// TODO: as time left shrinks, send additional notifications with
// increasing frequency, allowing the user to understand the
// urgency.
return err
}
if _, ok := err.(*BusySnapError); ok {
if err := client.PendingRefreshNotification(context.TODO(), refreshInfo); err != nil {
logger.Noticef("Cannot send notification about forced refresh: %v", err)
}
// XXX: remove the warning or send it only if no notification was delivered?
st.Warnf(i18n.NG(
"snap %q has been running for the maximum allowable %d day since its refresh was postponed. It will now be refreshed.",
"snap %q has been running for the maximum allowable %d days since its refresh was postponed. It will now be refreshed.", days),
Expand Down
8 changes: 8 additions & 0 deletions overlord/snapstate/refresh.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,14 @@ type BusySnapError struct {
busyHookNames []string
}

// AppNames returns the name of applications that were busy.
//
// The list may be empty, for example, because a hook is executing, or because
// tracking malfunctioned.
func (err *BusySnapError) AppNames() []string {
return err.busyAppNames
}

// Error formats an error string describing what is running.
func (err *BusySnapError) Error() string {
switch {
Expand Down
119 changes: 119 additions & 0 deletions usersession/agent/rest_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,18 @@ package agent

import (
"encoding/json"
"fmt"
"mime"
"net/http"
"strings"
"sync"
"time"

"github.com/mvo5/goconfigparser"

"github.com/snapcore/snapd/dbusutil"
"github.com/snapcore/snapd/desktop/notification"
"github.com/snapcore/snapd/i18n"
"github.com/snapcore/snapd/systemd"
"github.com/snapcore/snapd/timeout"
)
Expand All @@ -35,6 +41,7 @@ var restApi = []*Command{
rootCmd,
sessionInfoCmd,
serviceControlCmd,
pendingRefreshNotificationCmd,
}

var (
Expand All @@ -52,6 +59,11 @@ var (
Path: "/v1/service-control",
POST: postServiceControl,
}

pendingRefreshNotificationCmd = &Command{
Path: "/v1/notifications/pending-refresh",
POST: postPendingRefreshNotification,
}
)

func sessionInfo(c *Command, r *http.Request) Response {
Expand Down Expand Up @@ -197,3 +209,110 @@ func postServiceControl(c *Command, r *http.Request) Response {
sysd := systemd.New(systemd.UserMode, dummyReporter{})
return impl(&inst, sysd)
}

func postPendingRefreshNotification(c *Command, r *http.Request) Response {
zyga marked this conversation as resolved.
Show resolved Hide resolved
contentType := r.Header.Get("Content-Type")
mediaType, params, err := mime.ParseMediaType(contentType)
if err != nil {
return BadRequest("cannot parse content type: %v", err)
}

if mediaType != "application/json" {
return BadRequest("unknown content type: %s", contentType)
}

charset := strings.ToUpper(params["charset"])
if charset != "" && charset != "UTF-8" {
return BadRequest("unknown charset in content type: %s", contentType)
}

decoder := json.NewDecoder(r.Body)

// pendingSnapRefreshInfo holds information about pending snap refresh provided by snapd.
type pendingSnapRefreshInfo struct {
InstanceName string `json:"instance-name"`
TimeRemaining time.Duration `json:"time-remaining,omitempty"`
BusyAppName string `json:"busy-app-name,omitempty"`
BusyAppDesktopEntry string `json:"busy-app-desktop-entry,omitempty"`
}
var refreshInfo pendingSnapRefreshInfo
if err := decoder.Decode(&refreshInfo); err != nil {
return BadRequest("cannot decode request body into pending snap refresh info: %v", err)
}

conn, err := dbusutil.SessionBus()
zyga marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return SyncResponse(&resp{
Type: ResponseTypeError,
Status: 500,
Result: &errorResult{
Message: fmt.Sprintf("cannot connect to the session bus: %v", err),
},
})
}
defer conn.Close()
zyga marked this conversation as resolved.
Show resolved Hide resolved

// TODO: support desktop-specific notification APIs if they provide a better
// experience. For example, the GNOME notification API.
notifySrv := notification.New(conn)

// TODO: this message needs to be crafted better as it's the only thing guaranteed to be delivered.
summary := fmt.Sprintf(i18n.G("Pending update of %q snap"), refreshInfo.InstanceName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we have the desktop entry of the busy application, perhaps it would be worth including the name from that too? That potentially gives us a localised name. As it is unverified information from the client though, we probably still want to include the snape name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've experimented with some of that from the side of snapd, where we know the app name that is busy. It tends to create repetitive patterns (chrome chrome ...) and I didn't like the result. Desktop file names are sometimes equally weird so I'd postpone that until we can give it a try on a larger sample size.

var urgencyLevel notification.Urgency
var body string
var icon string
var hints []notification.Hint
if daysLeft := int(refreshInfo.TimeRemaining.Truncate(time.Hour).Hours() / 24); daysLeft > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest forming the parenthesised part of the message independently, then construct the body using Close the app to avoid disruptions (%s) as the format string. This would remove the need for translators to repeat the same translation 6+ times (depending on how many plural forms the language has).

We've got similar formatting code in timeutil/human.go for formatting times rather than durations, so perhaps that part of the formatting could be moved there (either in this PR or a follow-up).

urgencyLevel = notification.LowUrgency
body = fmt.Sprintf(i18n.NG(
"Close the app to avoid disruptions (%d day left)",
"Close the app to avoid disruptions (%d days left)", daysLeft), daysLeft)
} else if hoursLeft := int(refreshInfo.TimeRemaining.Truncate(time.Minute).Minutes() / 60); hoursLeft > 0 {
urgencyLevel = notification.NormalUrgency
body = fmt.Sprintf(i18n.NG(
"Close the app to avoid disruptions (%d hour left)",
"Close the app to avoid disruptions (%d hours left)", hoursLeft), hoursLeft)
} else if minutesLeft := int(refreshInfo.TimeRemaining.Truncate(time.Minute).Minutes()); minutesLeft > 0 {
urgencyLevel = notification.CriticalUrgency
body = fmt.Sprintf(i18n.NG(
"Close the app to avoid disruptions (%d minute left)",
"Close the app to avoid disruptions (%d minutes left)", minutesLeft), minutesLeft)
} else {
summary = fmt.Sprintf(i18n.G("Snap %q is refreshing now!"), refreshInfo.InstanceName)
urgencyLevel = notification.CriticalUrgency
}
hints = append(hints, notification.WithUrgency(urgencyLevel))
if refreshInfo.BusyAppDesktopEntry != "" {
hints = append(hints, notification.WithDesktopEntry(refreshInfo.BusyAppDesktopEntry))
zyga marked this conversation as resolved.
Show resolved Hide resolved
// Extract the icon manually
parser := goconfigparser.New()
if err := parser.ReadFile(fmt.Sprintf("/var/lib/snapd/desktop/applications/%s.desktop", refreshInfo.BusyAppDesktopEntry)); err == nil {
icon, _ = parser.Get("Desktop Entry", "Icon")
}
}
msg := &notification.Message{
AppName: refreshInfo.BusyAppName,
Summary: summary,
Icon: icon,
Body: body,
Hints: hints,
}

// TODO: if snap store is installed and actions are supported, add an action
// to open the snap store page for the given snap.
//
// XXX: how are instances supported in the snap store, are they?

// TODO: silently ignore error returned when the notification server does not exist.
// TODO: track returned notification ID and respond to actions, if supported.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we planning to do actual interaction here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given that this was literally designed last week I couldn't say.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to have interactivity for FDO notifications, the idle tracking code will need updating to keep the session agent alive until the notification is dismissed. That's more of a TODO item than something that needs to be done in this PR though.

if _, err := notifySrv.SendNotification(msg); err != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could also track the returned notification ID for as long as userd is alive, and use it to update existing notification, if one exists. This would ensure that the roster of persistent notifications only shows the current, up-to-date entry for each snap, not a separate entry per notification.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the nicer parts of the the new GNOME notification spec: it has client provided notification IDs, so we don't need to track server IDs to update/replace/withdraw old notifications.

return SyncResponse(&resp{
Type: ResponseTypeError,
Status: 500,
Result: &errorResult{
Message: fmt.Sprintf("cannot send notification message: %v", err),
},
})
}
return SyncResponse(nil)
}
20 changes: 20 additions & 0 deletions usersession/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"path/filepath"
"strconv"
"sync"
"time"

"github.com/snapcore/snapd/dirs"
)
Expand Down Expand Up @@ -267,3 +268,22 @@ func (client *Client) ServicesStop(ctx context.Context, services []string) (stop
_, stopFailures, err = client.serviceControlCall(ctx, "stop", services)
return stopFailures, err
}

// PendingSnapRefreshInfo holds information about pending snap refresh provided to userd.
type PendingSnapRefreshInfo struct {
InstanceName string `json:"instance-name"`
TimeRemaining time.Duration `json:"time-remaining,omitempty"`
BusyAppName string `json:"busy-app-name,omitempty"`
BusyAppDesktopEntry string `json:"busy-app-desktop-entry,omitempty"`
}

// PendingRefreshNotification broadcasts information about a refresh.
func (client *Client) PendingRefreshNotification(ctx context.Context, info *PendingSnapRefreshInfo) error {
zyga marked this conversation as resolved.
Show resolved Hide resolved
headers := map[string]string{"Content-Type": "application/json"}
reqBody, err := json.Marshal(info)
if err != nil {
return err
}
_, err = client.doMany(ctx, "POST", "/v1/notifications/pending-refresh", nil, headers, reqBody)
return err
}