The following pull request was submitted through Github. It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/3319
This e-mail was sent by the LXC bot, direct replies will not reach the author unless they happen to be subscribed to this list. === Description (from pull-request) === If the auto-update feature is on, prefer using cached images when launching a container, even if the cache image is "stale" (i.e. there is a more recent version of it in the given remote). I've decided to add a new preferCached boolean parameter to ImageDownload(), as it seemed the simplest option (and there are already other boolean switches, like forContainer). However, the API and implementation of ImageDownload() are getting a bit fat/complicated, so at some point we might want to break it into more modular pieces that offer simpler APIs. Closes #3262 Signed-off-by: Free Ekanayaka <[email protected]>
From 79de144f492646454df37073352813b94184291b Mon Sep 17 00:00:00 2001 From: Free Ekanayaka <[email protected]> Date: Mon, 15 May 2017 12:34:47 +0200 Subject: [PATCH] If present, use cached images when launching containers If the auto-update feature is on, prefer using cached images when launching a container, even if the cache image is "stale" (i.e. there is a more recent version of it in the given remote). I've decided to add a new preferCached boolean parameter to ImageDownload(), as it seemed the simplest option (and there are already other boolean switches, like forContainer). However, the API and implementation of ImageDownload() are getting a bit fat/complicated, so at some point we might want to break it into more modular pieces that offer simpler APIs. Signed-off-by: Free Ekanayaka <[email protected]> --- lxd/containers_post.go | 2 +- lxd/daemon_images.go | 15 ++++++++++- lxd/daemon_images_test.go | 51 ++++++++++++++++++++++++++++++++++++++ lxd/db_images.go | 37 +++++++++++++++++++++++++++ lxd/db_test.go | 23 +++++++++++++++++ lxd/images.go | 6 ++--- test/main.sh | 1 + test/suites/image.sh | 2 +- test/suites/image_prefer_cached.sh | 49 ++++++++++++++++++++++++++++++++++++ 9 files changed, 180 insertions(+), 6 deletions(-) create mode 100644 lxd/daemon_images_test.go create mode 100644 test/suites/image_prefer_cached.sh diff --git a/lxd/containers_post.go b/lxd/containers_post.go index a711281..5eed3f9 100644 --- a/lxd/containers_post.go +++ b/lxd/containers_post.go @@ -97,7 +97,7 @@ func createFromImage(d *Daemon, req *api.ContainersPost) Response { if req.Source.Server != "" { info, err = d.ImageDownload( op, req.Source.Server, req.Source.Protocol, req.Source.Certificate, req.Source.Secret, - hash, true, daemonConfig["images.auto_update_cached"].GetBool(), "") + hash, true, daemonConfig["images.auto_update_cached"].GetBool(), "", true) if err != nil { return err } diff --git a/lxd/daemon_images.go b/lxd/daemon_images.go index 40bf44b..e5b2232 100644 --- a/lxd/daemon_images.go +++ b/lxd/daemon_images.go @@ -91,7 +91,7 @@ func imageLoadStreamCache(d *Daemon) error { } // ImageDownload resolves the image fingerprint and if not in the database, downloads it -func (d *Daemon) ImageDownload(op *operation, server string, protocol string, certificate string, secret string, alias string, forContainer bool, autoUpdate bool, storagePool string) (*api.Image, error) { +func (d *Daemon) ImageDownload(op *operation, server string, protocol string, certificate string, secret string, alias string, forContainer bool, autoUpdate bool, storagePool string, preferCached bool) (*api.Image, error) { var err error var ctxMap log.Ctx @@ -220,6 +220,19 @@ func (d *Daemon) ImageDownload(op *operation, server string, protocol string, ce } } + // If auto-update is on and we're being given the image by + // alias, try to use a locally cached image matching the given + // server/protocol/alias, regardless of whether it's stale or + // not (we can assume that it will be not *too* stale since + // auto-update is on). + interval := daemonConfig["images.auto_update_interval"].GetInt64() + if preferCached && interval > 0 && alias != fp { + cachedFingerprint, err := dbImageSourceGetCachedFingerprint(d.db, server, protocol, alias) + if err == nil && cachedFingerprint != fp { + fp = cachedFingerprint + } + } + // Check if the image already exists (partial hash match) _, imgInfo, err := dbImageGet(d.db, fp, false, true) if err == nil { diff --git a/lxd/daemon_images_test.go b/lxd/daemon_images_test.go new file mode 100644 index 0000000..9240062 --- /dev/null +++ b/lxd/daemon_images_test.go @@ -0,0 +1,51 @@ +package main + +import ( + "testing" + "time" + + "github.com/lxc/lxd/client" + "github.com/lxc/lxd/shared/api" + "github.com/stretchr/testify/suite" +) + +type daemonImagesTestSuite struct { + lxdTestSuite +} + +// If the preferCached parameter of ImageDownload is set to true, and +// an image with matching remote details is already present in the +// database, and the auto-update settings is on, we won't download any +// newer image even if available, and just use the cached one. +func (suite *daemonImagesTestSuite) TestUseCachedImagesIfAvailable() { + // Create an image with alias "test" and fingerprint "abcd". + err := dbImageInsert(suite.d.db, "abcd", "foo.xz", 1, true, false, "amd64", time.Now(), time.Now(), map[string]string{}) + suite.Req.Nil(err) + id, _, err := dbImageGet(suite.d.db, "abcd", false, true) + suite.Req.Nil(err) + err = dbImageSourceInsert(suite.d.db, id, "img.srv", "simplestreams", "", "test") + suite.Req.Nil(err) + + // Pretend we have already a non-expired entry for the remote + // simplestream data, containing a more recent image for the + // given alias. + remote := lxd.ImageServer(&lxd.ProtocolSimpleStreams{}) + alias := api.ImageAliasesEntry{Name: "test"} + alias.Target = "other-more-recent-fingerprint" + fingerprints := []string{"other-more-recent-fingerprint"} + entry := &imageStreamCacheEntry{remote: remote, Aliases: []api.ImageAliasesEntry{alias}, Certificate: "", Fingerprints: fingerprints, expiry: time.Now().Add(time.Hour)} + imageStreamCache["img.srv"] = entry + defer delete(imageStreamCache, "img.srv") + + // Request an image with alias "test" and check that it's the + // one we created above. + op, err := operationCreate(operationClassTask, map[string][]string{}, nil, nil, nil, nil) + suite.Req.Nil(err) + image, err := suite.d.ImageDownload(op, "img.srv", "simplestreams", "", "", "test", false, false, "", true) + suite.Req.Nil(err) + suite.Req.Equal("abcd", image.Fingerprint) +} + +func TestDaemonImagesTestSuite(t *testing.T) { + suite.Run(t, new(daemonImagesTestSuite)) +} diff --git a/lxd/db_images.go b/lxd/db_images.go index 914b39f..ac3783a 100644 --- a/lxd/db_images.go +++ b/lxd/db_images.go @@ -105,6 +105,43 @@ func dbImageSourceGet(db *sql.DB, imageId int) (int, api.ImageSource, error) { } +// Try to find a source entry of a locally cached image that matches +// the given remote details (server, protocol and alias). Return the +// fingerprint linked to the matching entry, if any. +func dbImageSourceGetCachedFingerprint(db *sql.DB, server string, protocol string, alias string) (string, error) { + protocolInt := -1 + for protoInt, protoString := range dbImageSourceProtocol { + if protoString == protocol { + protocolInt = protoInt + } + } + + if protocolInt == -1 { + return "", fmt.Errorf("Invalid protocol: %s", protocol) + } + + q := `SELECT images.fingerprint + FROM images_source + INNER JOIN images + ON images_source.image_id=images.id + WHERE server=? AND protocol=? AND alias=?` + + fingerprint := "" + + arg1 := []interface{}{server, protocolInt, alias} + arg2 := []interface{}{&fingerprint} + err := dbQueryRowScan(db, q, arg1, arg2) + if err != nil { + if err == sql.ErrNoRows { + return "", NoSuchObjectError + } + + return "", err + } + + return fingerprint, nil +} + // Whether an image with the given fingerprint exists. func dbImageExists(db *sql.DB, fingerprint string) (bool, error) { var exists bool diff --git a/lxd/db_test.go b/lxd/db_test.go index b917796..9b5bea1 100644 --- a/lxd/db_test.go +++ b/lxd/db_test.go @@ -400,6 +400,29 @@ func (s *dbTestSuite) Test_dbImageAliasAdd() { s.Equal(alias.Target, "fingerprint") } +func (s *dbTestSuite) Test_dbImageSourceGetCachedFingerprint() { + imageID, _, err := dbImageGet(s.db, "fingerprint", false, false) + s.Nil(err) + + err = dbImageSourceInsert(s.db, imageID, "server.remote", "simplestreams", "", "test") + s.Nil(err) + + fingerprint, err := dbImageSourceGetCachedFingerprint(s.db, "server.remote", "simplestreams", "test") + s.Nil(err) + s.Equal(fingerprint, "fingerprint") +} + +func (s *dbTestSuite) Test_dbImageSourceGetCachedFingerprint_no_match() { + imageID, _, err := dbImageGet(s.db, "fingerprint", false, false) + s.Nil(err) + + err = dbImageSourceInsert(s.db, imageID, "server.remote", "simplestreams", "", "test") + s.Nil(err) + + _, err = dbImageSourceGetCachedFingerprint(s.db, "server.remote", "lxd", "test") + s.Equal(err, NoSuchObjectError) +} + func (s *dbTestSuite) Test_dbContainerConfig() { var err error var result map[string]string diff --git a/lxd/images.go b/lxd/images.go index bc0640c..a6df872 100644 --- a/lxd/images.go +++ b/lxd/images.go @@ -339,7 +339,7 @@ func imgPostRemoteInfo(d *Daemon, req api.ImagesPost, op *operation) (*api.Image return nil, fmt.Errorf("must specify one of alias or fingerprint for init from image") } - info, err := d.ImageDownload(op, req.Source.Server, req.Source.Protocol, req.Source.Certificate, req.Source.Secret, hash, false, req.AutoUpdate, "") + info, err := d.ImageDownload(op, req.Source.Server, req.Source.Protocol, req.Source.Certificate, req.Source.Secret, hash, false, req.AutoUpdate, "", false) if err != nil { return nil, err } @@ -408,7 +408,7 @@ func imgPostURLInfo(d *Daemon, req api.ImagesPost, op *operation) (*api.Image, e } // Import the image - info, err := d.ImageDownload(op, url, "direct", "", "", hash, false, req.AutoUpdate, "") + info, err := d.ImageDownload(op, url, "direct", "", "", hash, false, req.AutoUpdate, "", false) if err != nil { return nil, err } @@ -941,7 +941,7 @@ func autoUpdateImage(d *Daemon, op *operation, fingerprint string, id int, info // Update the image on each pool where it currently exists. hash := fingerprint for _, poolName := range poolNames { - newInfo, err := d.ImageDownload(op, source.Server, source.Protocol, source.Certificate, "", source.Alias, false, true, poolName) + newInfo, err := d.ImageDownload(op, source.Server, source.Protocol, source.Certificate, "", source.Alias, false, true, poolName, false) if err != nil { logger.Error("Failed to update the image", log.Ctx{"err": err, "fp": fingerprint}) diff --git a/test/main.sh b/test/main.sh index 06ae511..47bb352 100755 --- a/test/main.sh +++ b/test/main.sh @@ -600,6 +600,7 @@ run_test test_basic_usage "basic usage" run_test test_security "security features" run_test test_image_expiry "image expiry" run_test test_image_auto_update "image auto-update" +run_test test_image_prefer_cached "image prefer cached" run_test test_concurrent_exec "concurrent exec" run_test test_concurrent "concurrent startup" run_test test_snapshots "container snapshots" diff --git a/test/suites/image.sh b/test/suites/image.sh index 87a3af9..1bc4a10 100644 --- a/test/suites/image.sh +++ b/test/suites/image.sh @@ -30,7 +30,7 @@ test_image_expiry() { lxc_remote delete l2:c1 - # rest the default expiry + # reset the default expiry lxc_remote remote set-default l2 lxc_remote config set images.remote_cache_expiry 10 lxc_remote remote set-default local diff --git a/test/suites/image_prefer_cached.sh b/test/suites/image_prefer_cached.sh new file mode 100644 index 0000000..c999897 --- /dev/null +++ b/test/suites/image_prefer_cached.sh @@ -0,0 +1,49 @@ +# In case a cached image matching the desired alias is present, that +# one is preferred, even if the its remote has a more recent one. +test_image_prefer_cached() { + + if lxc image alias list | grep -q "^| testimage\s*|.*$"; then + lxc image delete testimage + fi + + # shellcheck disable=2039 + local LXD2_DIR LXD2_ADDR + LXD2_DIR=$(mktemp -d -p "${TEST_DIR}" XXX) + chmod +x "${LXD2_DIR}" + spawn_lxd "${LXD2_DIR}" true + LXD2_ADDR=$(cat "${LXD2_DIR}/lxd.addr") + + (LXD_DIR=${LXD2_DIR} deps/import-busybox --alias testimage --public) + fp1=$(LXD_DIR=${LXD2_DIR} lxc image info testimage | awk -F: '/^Fingerprint/ { print $2 }' | awk '{ print $1 }') + + lxc remote add l2 "${LXD2_ADDR}" --accept-certificate --password foo + lxc init l2:testimage c1 + + # Now the first image image is in the local store, since it was + # downloaded to create c1. + alias=$(lxc image info "${fp1}" | awk -F: '/^ Alias/ { print $2 }' | awk '{ print $1 }') + [ "${alias}" = "testimage" ] + + # Delete the first image from the remote store and replace it with a + # new one with a different fingerprint (passing "--template create" + # will do that). + (LXD_DIR=${LXD2_DIR} lxc image delete testimage) + (LXD_DIR=${LXD2_DIR} deps/import-busybox --alias testimage --public --template create) + fp2=$(LXD_DIR=${LXD2_DIR} lxc image info testimage | awk -F: '/^Fingerprint/ { print $2 }' | awk '{ print $1 }') + [ "${fp1}" != "${fp2}" ] + + # At this point starting a new container from "testimage" should not + # result in the new image being downloaded. + lxc init l2:testimage c2 + if lxc image info "${fp2}"; then + echo "The second image ${fp2} was downloaded and the cached one not used" + return 1 + fi + + lxc delete c1 + lxc delete c2 + lxc remote remove l2 + lxc image delete "${fp1}" + + kill_lxd "$LXD2_DIR" +}
_______________________________________________ lxc-devel mailing list [email protected] http://lists.linuxcontainers.org/listinfo/lxc-devel
