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

store: download deltas if explicitly enabled #2017

Merged
merged 6 commits into from
Sep 29, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 snap/info.go
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,10 @@ type DownloadInfo struct {
Size int64 `json:"size,omitempty"`
Sha3_384 string `json:"sha3-384,omitempty"`

// The server can include information about available deltas for a given
// snap at a specific revision during refresh. Currently during refresh the
// server will provide single matching deltas only, from the clients
// revision to the target revision when available, per requested format.
Deltas []DeltaInfo `json:"deltas,omitempty"`
}

Expand Down
83 changes: 73 additions & 10 deletions store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ package store
import (
"bytes"
"encoding/json"
"errors"
"fmt"
"io"
"io/ioutil"
Expand Down Expand Up @@ -141,7 +142,7 @@ type Config struct {
Series string

DetailFields []string
DeltaFormats []string
DeltaFormat string
}

// Store represents the ubuntu snap store
Expand All @@ -160,7 +161,7 @@ type Store struct {
fallbackStoreID string

detailFields []string
deltaFormats []string
deltaFormat string
// reused http client
client *http.Client

Expand Down Expand Up @@ -312,8 +313,8 @@ type searchResults struct {
// The fields we are interested in
var detailFields = getStructFields(snapDetails{})

// The default delta formats if none are configured.
var defaultSupportedDeltaFormats = []string{"xdelta"}
// The default delta format if not configured.
var defaultSupportedDeltaFormat = "xdelta"

// New creates a new Store with the given access configuration and for given the store id.
func New(cfg *Config, authContext auth.AuthContext) *Store {
Expand Down Expand Up @@ -357,9 +358,9 @@ func New(cfg *Config, authContext auth.AuthContext) *Store {
series = cfg.Series
}

deltaFormats := cfg.DeltaFormats
if deltaFormats == nil {
deltaFormats = defaultSupportedDeltaFormats
deltaFormat := cfg.DeltaFormat
if deltaFormat == "" {
deltaFormat = defaultSupportedDeltaFormat
}

// see https://wiki.ubuntu.com/AppStore/Interfaces/ClickPackageIndex
Expand All @@ -377,7 +378,7 @@ func New(cfg *Config, authContext auth.AuthContext) *Store {
detailFields: fields,
client: newHTTPClient(),
authContext: authContext,
deltaFormats: deltaFormats,
deltaFormat: deltaFormat,
}
}

Expand Down Expand Up @@ -1060,9 +1061,9 @@ func (s *Store) ListRefresh(installed []*RefreshCandidate, user *auth.UserState)
Data: jsonData,
}

if os.Getenv("SNAPPY_USE_DELTAS") == "1" {
if os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL") == "1" {
reqOptions.ExtraHeaders = map[string]string{
"X-Ubuntu-Delta-Formats": strings.Join(s.deltaFormats, ","),
"X-Ubuntu-Delta-Formats": s.deltaFormat,
}
}

Expand Down Expand Up @@ -1132,6 +1133,25 @@ func (s *Store) Download(name string, downloadInfo *snap.DownloadInfo, pbar prog
}
}()

if os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL") == "1" && len(downloadInfo.Deltas) == 1 {
downloadDir, err := ioutil.TempDir("", name+"-deltas")
if err == nil {
defer os.RemoveAll(downloadDir)

deltaPath, err := s.downloadDelta(name, downloadDir, downloadInfo, pbar, user)
// We revert to normal downloads if there is any error
if err != nil {
// Just log the error and continue with the normal non-delta
// download.
logger.Noticef("Cannot download deltas for %s: %v", name, err)
} else {
// Currently even on successful delta downloads, continue with the
// normal full download.
logger.Debugf("Successfully downloaded deltas for %s at %s", name, deltaPath)
}
}
}

url := downloadInfo.AnonDownloadURL
if url == "" || user != nil {
url = downloadInfo.DownloadURL
Expand Down Expand Up @@ -1195,6 +1215,49 @@ var download = func(name, downloadURL string, user *auth.UserState, s *Store, w
return err
}

// downloadDelta downloads the delta for the preferred format, returning the path.
func (s *Store) downloadDelta(name string, downloadDir string, downloadInfo *snap.DownloadInfo, pbar progress.Meter, user *auth.UserState) (string, error) {

if len(downloadInfo.Deltas) != 1 {
return "", errors.New("store returned more than one download delta")
}

deltaInfo := downloadInfo.Deltas[0]

if deltaInfo.Format != s.deltaFormat {
return "", fmt.Errorf("store returned a download delta with the wrong format (%q instead of the configured %s format)", deltaInfo.Format, s.deltaFormat)
}

deltaName := fmt.Sprintf("%s_%d_%d_delta.%s", name, deltaInfo.FromRevision, deltaInfo.ToRevision, deltaInfo.Format)

w, err := os.Create(path.Join(downloadDir, deltaName))
if err != nil {
return "", err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

defer w.Close()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done... although I did the similar defer func(){} to ensure we check the error on w.Close() and return that error unless there's an error being returned already (and either way deleting the file in that case). Let me know if that's not what you wanted.

deltaPath := w.Name()
defer func() {
if cerr := w.Close(); cerr != nil && err == nil {
err = cerr
}
if err != nil {
os.Remove(w.Name())
deltaPath = ""
}
}()

url := deltaInfo.AnonDownloadURL
if url == "" || user != nil {
url = deltaInfo.DownloadURL
}

err = download(deltaName, url, user, s, w, pbar)
if err != nil {
return "", err
Copy link
Contributor

Choose a reason for hiding this comment

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

Leaking an open file here, without the defer above.

}

Copy link
Contributor

Choose a reason for hiding this comment

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

so at this point the file is downloaded and stored somewhere, but you have no guarantees that it ever made it to disc. This treatment is fine for tempfiles (such as what Download does) but not at all fine for things you expect to be on permanent storage. You need to download to a temporary filename (in the same directory you want it to then live), and on success sync the file, rename it, and sync the directory. Take a look at AtomicWriteFileChown in osutil/io.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We chatted about this - and you mentioned that it is fine without a w.Sync() even, as xdelta doesn't go through raw io, but goes through the vfs.

return deltaPath, nil
}

type assertionSvcError struct {
Status int `json:"status"`
Type string `json:"type"`
Expand Down
189 changes: 183 additions & 6 deletions store/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ package store
import (
"bytes"
"encoding/json"
"errors"
"fmt"
"io"
"io/ioutil"
"net/http"
"net/http/httptest"
"net/url"
"os"
"path"
"strings"
"testing"

Expand Down Expand Up @@ -391,6 +393,181 @@ func (t *remoteRepoTestSuite) TestActualDownload500(c *C) {
c.Check(n, Equals, len(downloadBackoffs)) // woo!!
}

type downloadBehaviour []struct {
url string
error bool
}

var deltaTests = []struct {
downloads downloadBehaviour
info snap.DownloadInfo
expectedContent string
}{{
// The full snap is downloaded after the delta (currently,
// until the code applying the delta lands).
downloads: downloadBehaviour{
{url: "delta-url"},
{url: "full-snap-url"},
},
info: snap.DownloadInfo{
AnonDownloadURL: "full-snap-url",
Deltas: []snap.DeltaInfo{
{AnonDownloadURL: "delta-url", Format: "xdelta"},
},
},
expectedContent: "full-snap-url-content",
}, {
// If there is an error during the delta download, the
// full snap is downloaded as per normal.
downloads: downloadBehaviour{
{error: true},
{url: "full-snap-url"},
},
info: snap.DownloadInfo{
AnonDownloadURL: "full-snap-url",
Deltas: []snap.DeltaInfo{
{AnonDownloadURL: "delta-url", Format: "xdelta"},
},
},
expectedContent: "full-snap-url-content",
}, {
// If more than one matching delta is returned by the store
// we ignore deltas and do the full download.
downloads: downloadBehaviour{
{url: "full-snap-url"},
},
info: snap.DownloadInfo{
AnonDownloadURL: "full-snap-url",
Deltas: []snap.DeltaInfo{
{AnonDownloadURL: "delta-url", Format: "xdelta"},
{AnonDownloadURL: "delta-url-2", Format: "xdelta"},
},
},
expectedContent: "full-snap-url-content",
}}

func (t *remoteRepoTestSuite) TestDownloadWithDeltas(c *C) {
origUseDeltas := os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL")
defer os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", origUseDeltas)
c.Assert(os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", "1"), IsNil)

for _, testCase := range deltaTests {

downloadIndex := 0
download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error {
if testCase.downloads[downloadIndex].error {
downloadIndex++
return errors.New("Bang")
}
c.Check(url, Equals, testCase.downloads[downloadIndex].url)
w.Write([]byte(testCase.downloads[downloadIndex].url + "-content"))
downloadIndex++
return nil
}

path, err := t.store.Download("foo", &testCase.info, nil, nil)

c.Assert(err, IsNil)
defer os.Remove(path)
content, err := ioutil.ReadFile(path)
c.Assert(err, IsNil)
c.Assert(string(content), Equals, testCase.expectedContent)
}
}

var downloadDeltaTests = []struct {
info snap.DownloadInfo
authenticated bool
format string
expectedURL string
expectedPath string
}{{
// An unauthenticated request downloads the anonymous delta url.
info: snap.DownloadInfo{
Deltas: []snap.DeltaInfo{
{AnonDownloadURL: "anon-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 26},
},
},
authenticated: false,
format: "xdelta",
expectedURL: "anon-delta-url",
expectedPath: "snapname_24_26_delta.xdelta",
}, {
// An authenticated request downloads the authenticated delta url.
info: snap.DownloadInfo{
Deltas: []snap.DeltaInfo{
{DownloadURL: "auth-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 26},
},
},
authenticated: true,
format: "xdelta",
expectedURL: "auth-delta-url",
expectedPath: "snapname_24_26_delta.xdelta",
}, {
// An error is returned if more than one matching delta is returned by the store,
// though this may be handled in the future.
info: snap.DownloadInfo{
Deltas: []snap.DeltaInfo{
{DownloadURL: "xdelta-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 25},
{DownloadURL: "bsdiff-delta-url", Format: "xdelta", FromRevision: 25, ToRevision: 26},
},
},
authenticated: false,
format: "xdelta",
expectedURL: "",
expectedPath: "",
}, {
// If the supported format isn't available, an error is returned.
info: snap.DownloadInfo{
Deltas: []snap.DeltaInfo{
{DownloadURL: "xdelta-delta-url", Format: "xdelta", FromRevision: 24, ToRevision: 26},
{DownloadURL: "ydelta-delta-url", Format: "ydelta", FromRevision: 24, ToRevision: 26},
},
},
authenticated: false,
format: "bsdiff",
expectedURL: "",
expectedPath: "",
}}

func (t *remoteRepoTestSuite) TestDownloadDelta(c *C) {
origUseDeltas := os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL")
defer os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", origUseDeltas)
c.Assert(os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", "1"), IsNil)

for _, testCase := range downloadDeltaTests {
t.store.deltaFormat = testCase.format
download = func(name, url string, user *auth.UserState, s *Store, w io.Writer, pbar progress.Meter) error {
expectedUser := t.user
if !testCase.authenticated {
expectedUser = nil
}
c.Check(user, Equals, expectedUser)
c.Check(url, Equals, testCase.expectedURL)
return nil
}

downloadDir, err := ioutil.TempDir("", "")
c.Assert(err, IsNil)
defer os.RemoveAll(downloadDir)

authedUser := t.user
if !testCase.authenticated {
authedUser = nil
}

deltaPath, err := t.store.downloadDelta("snapname", downloadDir, &testCase.info, nil, authedUser)

if testCase.expectedPath == "" {
c.Assert(deltaPath, Equals, "")
c.Assert(err, NotNil)
} else {
c.Assert(err, IsNil)
c.Assert(deltaPath, Equals, path.Join(downloadDir, testCase.expectedPath))
}
}
}

func (t *remoteRepoTestSuite) TestDoRequestSetsAuth(c *C) {
mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
c.Check(r.UserAgent(), Equals, userAgent)
Expand Down Expand Up @@ -1668,9 +1845,9 @@ var MockUpdatesWithDeltasJSON = `
`

func (t *remoteRepoTestSuite) TestUbuntuStoreRepositoryListRefreshWithDeltas(c *C) {
orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS")
defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas)
c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "1"), IsNil)
origUseDeltas := os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL")
defer os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", origUseDeltas)
c.Assert(os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", "1"), IsNil)

mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
c.Check(r.Header.Get("X-Ubuntu-Delta-Formats"), Equals, `xdelta`)
Expand Down Expand Up @@ -1743,9 +1920,9 @@ func (t *remoteRepoTestSuite) TestUbuntuStoreRepositoryListRefreshWithDeltas(c *

func (t *remoteRepoTestSuite) TestUbuntuStoreRepositoryListRefreshWithoutDeltas(c *C) {
// Verify the X-Delta-Format header is not set.
orig_use_deltas := os.Getenv("SNAPPY_USE_DELTAS")
defer os.Setenv("SNAPPY_USE_DELTAS", orig_use_deltas)
c.Assert(os.Setenv("SNAPPY_USE_DELTAS", "0"), IsNil)
origUseDeltas := os.Getenv("SNAPD_USE_DELTAS_EXPERIMENTAL")
defer os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", origUseDeltas)
c.Assert(os.Setenv("SNAPD_USE_DELTAS_EXPERIMENTAL", "0"), IsNil)

mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
c.Check(r.Header.Get("X-Ubuntu-Delta-Formats"), Equals, ``)
Expand Down