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

many: add refresh.rate-limit core option #5606

Merged
merged 26 commits into from Sep 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
6b03013
many: add `snap set core network.rate-limit` option
mvo5 Aug 6, 2018
4a4a411
snapstate,store: support `core.refresh.rate-limit` to limit to auto-r…
mvo5 Aug 7, 2018
8ee1cdd
add juju/ratelimit to fedora.spec
mvo5 Aug 7, 2018
fa6ca88
snapstate: do not (ab)use the context to pass auto-refresh information
mvo5 Aug 17, 2018
83aec89
Merge remote-tracking branch 'upstream/master' into rate-limit
mvo5 Aug 22, 2018
1e85e83
strutil: add new ParseValueWithUnit
mvo5 Aug 24, 2018
637a712
strutil: add new ParseValueWithUnit
mvo5 Aug 24, 2018
9f4ca69
configcore,snapstate: allow only rate limit with units
mvo5 Aug 24, 2018
67c797c
Merge remote-tracking branch 'upstream/master' into rate-limit
mvo5 Aug 24, 2018
c30d164
update tests
mvo5 Aug 24, 2018
f07b003
address review feedback
mvo5 Aug 24, 2018
360df12
image: add store.DownloadOptions and remove RateLimitContext
mvo5 Aug 24, 2018
6cc7e79
Merge branch 'parseUnit' into rate-limit
mvo5 Aug 24, 2018
1c35f9a
Merge remote-tracking branch 'origin/parseUnit' into rate-limit
mvo5 Aug 24, 2018
8f08694
Merge remote-tracking branch 'upstream/master' into rate-limit
mvo5 Aug 27, 2018
d24c46f
Merge branch 'parseUnit' into rate-limit
mvo5 Aug 27, 2018
df0cbb8
Merge branch 'parseUnit' into rate-limit
mvo5 Aug 28, 2018
80cff0c
configstate,snapstate: update for new strutil.ParseByteSize name
mvo5 Aug 28, 2018
d6ae4e6
Merge remote-tracking branch 'upstream/master' into rate-limit
mvo5 Aug 28, 2018
36e78c6
Merge remote-tracking branch 'upstream/master' into rate-limit
mvo5 Aug 29, 2018
fa0011d
packaging: fix merge artifact
mvo5 Aug 29, 2018
c219838
Merge remote-tracking branch 'upstream/master' into rate-limit
mvo5 Aug 29, 2018
debb5c5
address review feedback
mvo5 Sep 4, 2018
93d7562
add missing unit tests
mvo5 Sep 4, 2018
f513f3b
store: add unit tests for download()
mvo5 Sep 4, 2018
3a956ad
remove download_test.go again, may come back in a separate PR
mvo5 Sep 4, 2018
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
2 changes: 1 addition & 1 deletion daemon/api.go
Expand Up @@ -968,7 +968,7 @@ func snapUpdateMany(inst *snapInstruction, st *state.State) (*snapInstructionRes
}

// TODO: use a per-request context
updated, tasksets, err := snapstateUpdateMany(context.TODO(), st, inst.Snaps, inst.userID)
updated, tasksets, err := snapstateUpdateMany(context.TODO(), st, inst.Snaps, inst.userID, nil)
if err != nil {
return nil, err
}
Expand Down
10 changes: 5 additions & 5 deletions daemon/api_test.go
Expand Up @@ -3427,7 +3427,7 @@ func (s *apiSuite) TestRefreshIgnoreValidation(c *check.C) {

func (s *apiSuite) TestPostSnapsOp(c *check.C) {
assertstateRefreshSnapDeclarations = func(*state.State, int) error { return nil }
snapstateUpdateMany = func(_ context.Context, s *state.State, names []string, userID int) ([]string, []*state.TaskSet, error) {
snapstateUpdateMany = func(_ context.Context, s *state.State, names []string, userID int, flags *snapstate.Flags) ([]string, []*state.TaskSet, error) {
c.Check(names, check.HasLen, 0)
t := s.NewTask("fake-refresh-all", "Refreshing everything")
return []string{"fake1", "fake2"}, []*state.TaskSet{state.NewTaskSet(t)}, nil
Expand Down Expand Up @@ -3472,7 +3472,7 @@ func (s *apiSuite) TestRefreshAll(c *check.C) {
} {
refreshSnapDecls = false

snapstateUpdateMany = func(_ context.Context, s *state.State, names []string, userID int) ([]string, []*state.TaskSet, error) {
snapstateUpdateMany = func(_ context.Context, s *state.State, names []string, userID int, flags *snapstate.Flags) ([]string, []*state.TaskSet, error) {
c.Check(names, check.HasLen, 0)
t := s.NewTask("fake-refresh-all", "Refreshing everything")
return tst.snaps, []*state.TaskSet{state.NewTaskSet(t)}, nil
Expand All @@ -3496,7 +3496,7 @@ func (s *apiSuite) TestRefreshAllNoChanges(c *check.C) {
return assertstate.RefreshSnapDeclarations(s, userID)
}

snapstateUpdateMany = func(_ context.Context, s *state.State, names []string, userID int) ([]string, []*state.TaskSet, error) {
snapstateUpdateMany = func(_ context.Context, s *state.State, names []string, userID int, flags *snapstate.Flags) ([]string, []*state.TaskSet, error) {
c.Check(names, check.HasLen, 0)
return nil, nil, nil
}
Expand All @@ -3519,7 +3519,7 @@ func (s *apiSuite) TestRefreshMany(c *check.C) {
return nil
}

snapstateUpdateMany = func(_ context.Context, s *state.State, names []string, userID int) ([]string, []*state.TaskSet, error) {
snapstateUpdateMany = func(_ context.Context, s *state.State, names []string, userID int, flags *snapstate.Flags) ([]string, []*state.TaskSet, error) {
c.Check(names, check.HasLen, 2)
t := s.NewTask("fake-refresh-2", "Refreshing two")
return names, []*state.TaskSet{state.NewTaskSet(t)}, nil
Expand All @@ -3544,7 +3544,7 @@ func (s *apiSuite) TestRefreshMany1(c *check.C) {
return nil
}

snapstateUpdateMany = func(_ context.Context, s *state.State, names []string, userID int) ([]string, []*state.TaskSet, error) {
snapstateUpdateMany = func(_ context.Context, s *state.State, names []string, userID int, flags *snapstate.Flags) ([]string, []*state.TaskSet, error) {
c.Check(names, check.HasLen, 1)
t := s.NewTask("fake-refresh-1", "Refreshing one")
return names, []*state.TaskSet{state.NewTaskSet(t)}, nil
Expand Down
4 changes: 2 additions & 2 deletions image/helpers.go
Expand Up @@ -50,7 +50,7 @@ import (
// A Store can find metadata on snaps, download snaps and fetch assertions.
type Store interface {
SnapAction(context.Context, []*store.CurrentSnap, []*store.SnapAction, *auth.UserState, *store.RefreshOptions) ([]*snap.Info, error)
Download(ctx context.Context, name, targetFn string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState) error
Download(ctx context.Context, name, targetFn string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState, dlOpts *store.DownloadOptions) error

Assertion(assertType *asserts.AssertionType, primaryKey []string, user *auth.UserState) (asserts.Assertion, error)
}
Expand Down Expand Up @@ -265,7 +265,7 @@ func (tsto *ToolingStore) DownloadSnap(name string, revision snap.Revision, opts
os.Exit(1)
}()

if err = sto.Download(context.TODO(), name, targetFn, &snap.DownloadInfo, pb, tsto.user); err != nil {
if err = sto.Download(context.TODO(), name, targetFn, &snap.DownloadInfo, pb, tsto.user, nil); err != nil {
return "", nil, err
}

Expand Down
4 changes: 2 additions & 2 deletions image/image_test.go
Expand Up @@ -54,7 +54,7 @@ func (s *emptyStore) SnapAction(context.Context, []*store.CurrentSnap, []*store.
return nil, fmt.Errorf("cannot find snap")
}

func (s *emptyStore) Download(ctx context.Context, name, targetFn string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState) error {
func (s *emptyStore) Download(ctx context.Context, name, targetFn string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState, dlOpts *store.DownloadOptions) error {
return fmt.Errorf("cannot download")
}

Expand Down Expand Up @@ -202,7 +202,7 @@ func (s *imageSuite) SnapAction(_ context.Context, _ []*store.CurrentSnap, actio
return nil, fmt.Errorf("no %q in the fake store", actions[0].InstanceName)
}

func (s *imageSuite) Download(ctx context.Context, name, targetFn string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState) error {
func (s *imageSuite) Download(ctx context.Context, name, targetFn string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState, dlOpts *store.DownloadOptions) error {
return osutil.CopyFile(s.downloadedSnaps[name], targetFn, 0)
}

Expand Down
3 changes: 3 additions & 0 deletions overlord/configstate/configcore/corecfg.go
Expand Up @@ -101,6 +101,9 @@ func Run(tr Conf) error {
if err := validateRefreshSchedule(tr); err != nil {
return err
}
if err := validateRefreshRateLimit(tr); err != nil {
return err
}
if err := validateExperimentalSettings(tr); err != nil {
return err
}
Expand Down
17 changes: 17 additions & 0 deletions overlord/configstate/configcore/refresh.go
Expand Up @@ -25,6 +25,7 @@ import (
"time"

"github.com/snapcore/snapd/overlord/devicestate"
"github.com/snapcore/snapd/strutil"
"github.com/snapcore/snapd/timeutil"
)

Expand All @@ -34,6 +35,7 @@ func init() {
supportedConfigurations["core.refresh.timer"] = true
supportedConfigurations["core.refresh.metered"] = true
supportedConfigurations["core.refresh.retain"] = true
supportedConfigurations["core.refresh.rate-limit"] = true
}

func validateRefreshSchedule(tr Conf) error {
Expand Down Expand Up @@ -114,3 +116,18 @@ func validateRefreshSchedule(tr Conf) error {
_, err = timeutil.ParseLegacySchedule(refreshScheduleStr)
return err
}

func validateRefreshRateLimit(tr Conf) error {
refreshRateLimit, err := coreCfg(tr, "refresh.rate-limit")
if err != nil {
return err
}
// reset is fine
if len(refreshRateLimit) == 0 {
return nil
}
if _, err := strutil.ParseByteSize(refreshRateLimit); err != nil {
return err
}
return nil
}
2 changes: 1 addition & 1 deletion overlord/hookstate/ctlcmd/services_test.go
Expand Up @@ -346,7 +346,7 @@ func (s *servicectlSuite) TestQueuedCommandsUpdateMany(c *C) {
s.st.Lock()

chg := s.st.NewChange("update many change", "update change")
installed, tts, err := snapstate.UpdateMany(context.TODO(), s.st, []string{"test-snap", "other-snap"}, 0)
installed, tts, err := snapstate.UpdateMany(context.TODO(), s.st, []string{"test-snap", "other-snap"}, 0, nil)
c.Assert(err, IsNil)
sort.Strings(installed)
c.Check(installed, DeepEquals, []string{"other-snap", "test-snap"})
Expand Down
10 changes: 5 additions & 5 deletions overlord/managers_test.go
Expand Up @@ -1023,7 +1023,7 @@ version: @VERSION@
snapPath, _ = ms.makeStoreTestSnap(c, strings.Replace(snapYamlContent, "@VERSION@", ver, -1), revno)
ms.serveSnap(snapPath, revno)

updated, tss, err := snapstate.UpdateMany(context.TODO(), st, []string{"foo"}, 0)
updated, tss, err := snapstate.UpdateMany(context.TODO(), st, []string{"foo"}, 0, nil)
c.Check(updated, IsNil)
c.Check(tss, IsNil)
// no validation we, get an error
Expand All @@ -1043,7 +1043,7 @@ version: @VERSION@
c.Assert(err, IsNil)

// ... and try again
updated, tss, err = snapstate.UpdateMany(context.TODO(), st, []string{"foo"}, 0)
updated, tss, err = snapstate.UpdateMany(context.TODO(), st, []string{"foo"}, 0, nil)
c.Assert(err, IsNil)
c.Assert(updated, DeepEquals, []string{"foo"})
c.Assert(tss, HasLen, 1)
Expand Down Expand Up @@ -1590,7 +1590,7 @@ apps:
ms.serveSnap(fooPath, "15")

// refresh all
updated, tss, err := snapstate.UpdateMany(context.TODO(), st, nil, 0)
updated, tss, err := snapstate.UpdateMany(context.TODO(), st, nil, 0, nil)
c.Assert(err, IsNil)
c.Assert(updated, DeepEquals, []string{"foo"})
c.Assert(tss, HasLen, 1)
Expand Down Expand Up @@ -1836,7 +1836,7 @@ apps:
err = assertstate.RefreshSnapDeclarations(st, 0)
c.Assert(err, IsNil)

updated, tss, err := snapstate.UpdateMany(context.TODO(), st, nil, 0)
updated, tss, err := snapstate.UpdateMany(context.TODO(), st, nil, 0, nil)
c.Assert(err, IsNil)
sort.Strings(updated)
c.Assert(updated, DeepEquals, []string{"bar", "foo"})
Expand Down Expand Up @@ -2361,7 +2361,7 @@ version: @VERSION@`
err := assertstate.RefreshSnapDeclarations(st, 0)
c.Assert(err, IsNil)

updates, tts, err := snapstate.UpdateMany(context.TODO(), st, []string{"core", "some-snap", "other-snap"}, 0)
updates, tts, err := snapstate.UpdateMany(context.TODO(), st, []string{"core", "some-snap", "other-snap"}, 0, nil)
c.Assert(err, IsNil)
c.Check(updates, HasLen, 3)
c.Assert(tts, HasLen, 3)
Expand Down
2 changes: 1 addition & 1 deletion overlord/snapstate/backend.go
Expand Up @@ -42,7 +42,7 @@ type StoreService interface {
Sections(ctx context.Context, user *auth.UserState) ([]string, error)
WriteCatalogs(ctx context.Context, names io.Writer, adder store.SnapAdder) error

Download(context.Context, string, string, *snap.DownloadInfo, progress.Meter, *auth.UserState) error
Download(context.Context, string, string, *snap.DownloadInfo, progress.Meter, *auth.UserState, *store.DownloadOptions) error

Assertion(assertType *asserts.AssertionType, primaryKey []string, user *auth.UserState) (asserts.Assertion, error)

Expand Down
8 changes: 7 additions & 1 deletion overlord/snapstate/backend_test.go
Expand Up @@ -97,6 +97,7 @@ type fakeDownload struct {
name string
macaroon string
target string
opts *store.DownloadOptions
}

type byName []store.CurrentSnap
Expand Down Expand Up @@ -485,7 +486,7 @@ func (f *fakeStore) SuggestedCurrency() string {
return "XTS"
}

func (f *fakeStore) Download(ctx context.Context, name, targetFn string, snapInfo *snap.DownloadInfo, pb progress.Meter, user *auth.UserState) error {
func (f *fakeStore) Download(ctx context.Context, name, targetFn string, snapInfo *snap.DownloadInfo, pb progress.Meter, user *auth.UserState, dlOpts *store.DownloadOptions) error {
f.pokeStateLock()

if _, key := snap.SplitInstanceName(name); key != "" {
Expand All @@ -495,10 +496,15 @@ func (f *fakeStore) Download(ctx context.Context, name, targetFn string, snapInf
if user != nil {
macaroon = user.StoreMacaroon
}
// only add the options if they contain anything interessting
if *dlOpts == (store.DownloadOptions{}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting :)

dlOpts = nil
}
f.downloads = append(f.downloads, fakeDownload{
macaroon: macaroon,
name: name,
target: targetFn,
opts: dlOpts,
})
f.fakeBackend.ops = append(f.fakeBackend.ops, fakeOp{op: "storesvc-download", name: name})

Expand Down
3 changes: 3 additions & 0 deletions overlord/snapstate/flags.go
Expand Up @@ -57,6 +57,9 @@ type Flags struct {
// Amend allows refreshing out of a snap unknown to the store
// and into one that is known.
Amend bool `json:"amend,omitempty"`

// IsAutoRefresh is true if the snap is currently auto-refreshed
IsAutoRefresh bool `json:"is-auto-refresh,omitempty"`
}

// DevModeAllowed returns whether a snap can be installed with devmode confinement (either set or overridden)
Expand Down
29 changes: 27 additions & 2 deletions overlord/snapstate/handlers.go
Expand Up @@ -43,6 +43,8 @@ import (
"github.com/snapcore/snapd/overlord/state"
"github.com/snapcore/snapd/release"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/store"
"github.com/snapcore/snapd/strutil"
)

// hook setup by devicestate
Expand Down Expand Up @@ -401,6 +403,23 @@ func installInfoUnlocked(st *state.State, snapsup *SnapSetup) (*snap.Info, error
return installInfo(st, snapsup.InstanceName(), snapsup.Channel, snapsup.Revision(), snapsup.UserID)
}

// autoRefreshRateLimited returns the rate limit of auto-refreshes or 0 if
// there is no limit.
func autoRefreshRateLimited(st *state.State) (rate int64) {
tr := config.NewTransaction(st)

var rateLimit string
err := tr.Get("core", "refresh.rate-limit", &rateLimit)
if err != nil {
return 0
}
val, err := strutil.ParseByteSize(rateLimit)
if err != nil {
return 0
}
return val
}

func (m *SnapManager) doDownloadSnap(t *state.Task, tomb *tomb.Tomb) error {
st := t.State()
st.Lock()
Expand All @@ -412,6 +431,7 @@ func (m *SnapManager) doDownloadSnap(t *state.Task, tomb *tomb.Tomb) error {

st.Lock()
theStore := Store(st)
rate := autoRefreshRateLimited(st)
user, err := userFromUserID(st, snapsup.UserID)
st.Unlock()
if err != nil {
Expand All @@ -420,6 +440,11 @@ func (m *SnapManager) doDownloadSnap(t *state.Task, tomb *tomb.Tomb) error {

meter := NewTaskProgressAdapterUnlocked(t)
targetFn := snapsup.MountFile()

dlOpts := &store.DownloadOptions{}
if snapsup.IsAutoRefresh && rate > 0 {
dlOpts.RateLimit = rate
}
if snapsup.DownloadInfo == nil {
var storeInfo *snap.Info
// COMPATIBILITY - this task was created from an older version
Expand All @@ -429,10 +454,10 @@ func (m *SnapManager) doDownloadSnap(t *state.Task, tomb *tomb.Tomb) error {
if err != nil {
return err
}
err = theStore.Download(tomb.Context(nil), snapsup.SnapName(), targetFn, &storeInfo.DownloadInfo, meter, user)
err = theStore.Download(tomb.Context(nil), snapsup.SnapName(), targetFn, &storeInfo.DownloadInfo, meter, user, dlOpts)
snapsup.SideInfo = &storeInfo.SideInfo
} else {
err = theStore.Download(tomb.Context(nil), snapsup.SnapName(), targetFn, snapsup.DownloadInfo, meter, user)
err = theStore.Download(tomb.Context(nil), snapsup.SnapName(), targetFn, snapsup.DownloadInfo, meter, user, dlOpts)
}
if err != nil {
return err
Expand Down
48 changes: 48 additions & 0 deletions overlord/snapstate/handlers_download_test.go
Expand Up @@ -20,8 +20,12 @@
package snapstate_test

import (
"path/filepath"

. "gopkg.in/check.v1"

"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/overlord/configstate/config"
"github.com/snapcore/snapd/overlord/snapstate"
"github.com/snapcore/snapd/overlord/state"
"github.com/snapcore/snapd/snap"
Expand Down Expand Up @@ -186,3 +190,47 @@ func (s *downloadSnapSuite) TestDoUndoDownloadSnap(c *C) {
c.Assert(err, Equals, state.ErrNoState)

}

func (s *downloadSnapSuite) TestDoDownloadRateLimitedIntegration(c *C) {
s.state.Lock()

// set auto-refresh rate-limit
tr := config.NewTransaction(s.state)
tr.Set("core", "refresh.rate-limit", "1234B")
tr.Commit()

// setup fake auto-refresh download
si := &snap.SideInfo{
RealName: "foo",
SnapID: "foo-id",
Revision: snap.R(11),
}
t := s.state.NewTask("download-snap", "test")
t.Set("snap-setup", &snapstate.SnapSetup{
SideInfo: si,
DownloadInfo: &snap.DownloadInfo{
DownloadURL: "http://some-url.com/snap",
},
Flags: snapstate.Flags{
IsAutoRefresh: true,
},
})
s.state.NewChange("dummy", "...").AddTask(t)

s.state.Unlock()

s.se.Ensure()
s.se.Wait()

// ensure that rate limit was honored
c.Assert(s.fakeStore.downloads, DeepEquals, []fakeDownload{
{
name: "foo",
target: filepath.Join(dirs.SnapBlobDir, "foo_11.snap"),
opts: &store.DownloadOptions{
RateLimit: 1234,
},
},
})

}