[lxc-devel] [lxd/master] Fix regression in image auto-update logic
freeekanayaka on Github
lxc-bot at linuxcontainers.org
Mon May 15 08:07:13 UTC 2017
A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 1985 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20170515/b52dadfa/attachment.bin>
-------------- next part --------------
From e51906bbe8bf8e1439d9b3ead18e85498e14affc Mon Sep 17 00:00:00 2001
From: Free Ekanayaka <free.ekanayaka at canonical.com>
Date: Mon, 15 May 2017 08:37:03 +0200
Subject: [PATCH] Fix regression in image auto-update logic
From what I understand, it seems that this commit:
https://github.com/lxc/lxd/commit/7d4869527f1cef579015a06258c93a33e76cc865
introduced a regression in the auto-update logic. The code that was
setting the AutoUpdate flag in the *api.Image structure in line 541:
https://github.com/lxc/lxd/commit/7d4869527f1cef579015a06258c93a33e76cc865#diff-6182827642cd00e061c6ffe52891acc5L541
was moved down in the function:
https://github.com/lxc/lxd/commit/7d4869527f1cef579015a06258c93a33e76cc865#diff-6182827642cd00e061c6ffe52891acc5R482
however the new location is after the call to dbImageInsert:
```
err = dbImageInsert(d.db, info.Fingerprint, info.Filename, info.Size, info.Public, info.AutoUpdate, info.Architecture, info.CreatedAt, info.ExpiresAt, info.Properties)
```
that actually makes use of such setting. This results in the
auto_update column in the database always being 0, no matter what, and
the server logic to auto-update images will never kick in.
This commit should also fix another (non fatal) regression introduced
by:
https://github.com/lxc/lxd/commit/806d55c88562ab82b32d2d8563c20a8ec9bdac98
where appending the empty string "" to the poolNames to be processed
ends up triggering a call to ```doDeleteImageFromPool()``` from
``autoUpdateImage``:
https://github.com/lxc/lxd/blob/471604191b9793ffe479036dab8e527cb743cee1/lxd/images.go#L975
which as far as I understand will always end up with an error in case
poolName is the empty string, so a spurious error will be logged.
Note that auto-update regression affects the latest 2.13 release, but
not 2.12.
Signed-off-by: Free Ekanayaka <free.ekanayaka at canonical.com>
---
lxd/daemon_images.go | 9 +++++--
lxd/images.go | 10 +++++---
test/main.sh | 1 +
test/suites/image_auto_update.sh | 51 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 66 insertions(+), 5 deletions(-)
create mode 100644 test/suites/image_auto_update.sh
diff --git a/lxd/daemon_images.go b/lxd/daemon_images.go
index 859dbb6..40bf44b 100644
--- a/lxd/daemon_images.go
+++ b/lxd/daemon_images.go
@@ -460,6 +460,13 @@ func (d *Daemon) ImageDownload(op *operation, server string, protocol string, ce
// Override visiblity
info.Public = false
+ // We want to enable auto-update only if we were passed an
+ // alias name, so we can figure when the associated
+ // fingerprint changes in the remote.
+ if alias != fp {
+ info.AutoUpdate = autoUpdate
+ }
+
// Create the database entry
err = dbImageInsert(d.db, info.Fingerprint, info.Filename, info.Size, info.Public, info.AutoUpdate, info.Architecture, info.CreatedAt, info.ExpiresAt, info.Properties)
if err != nil {
@@ -479,8 +486,6 @@ func (d *Daemon) ImageDownload(op *operation, server string, protocol string, ce
if err != nil {
return nil, err
}
-
- info.AutoUpdate = autoUpdate
}
// Import into the requested storage pool
diff --git a/lxd/images.go b/lxd/images.go
index 4021479..bc0640c 100644
--- a/lxd/images.go
+++ b/lxd/images.go
@@ -972,9 +972,13 @@ func autoUpdateImage(d *Daemon, op *operation, fingerprint string, id int, info
continue
}
- err = doDeleteImageFromPool(d, fingerprint, poolName)
- if err != nil {
- logger.Error("Error deleting image", log.Ctx{"err": err, "fp": fingerprint})
+ // If we do have optimized pools, make sure we remove
+ // the volumes associated with the image.
+ if poolName != "" {
+ err = doDeleteImageFromPool(d, fingerprint, poolName)
+ if err != nil {
+ logger.Error("Error deleting image from pool", log.Ctx{"err": err, "fp": fingerprint})
+ }
}
}
diff --git a/test/main.sh b/test/main.sh
index d73597d..06ae511 100755
--- a/test/main.sh
+++ b/test/main.sh
@@ -599,6 +599,7 @@ run_test test_remote_usage "remote usage"
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_concurrent_exec "concurrent exec"
run_test test_concurrent "concurrent startup"
run_test test_snapshots "container snapshots"
diff --git a/test/suites/image_auto_update.sh b/test/suites/image_auto_update.sh
new file mode 100644
index 0000000..a77152a
--- /dev/null
+++ b/test/suites/image_auto_update.sh
@@ -0,0 +1,51 @@
+test_image_auto_update() {
+ 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}" ]
+
+ # Restart the server to force an image refresh immediately
+ # shellcheck disable=2153
+ shutdown_lxd "${LXD_DIR}"
+ respawn_lxd "${LXD_DIR}"
+
+ # The first image got deleted from the local storage
+ if lxc image info "${fp1}"; then
+ echo "First image ${fp1} not deleted from local store"
+ return 1
+ fi
+
+ # The second image replaced the first one in the local storage.
+ alias=$(lxc image info "${fp2}" | awk -F: '/^ Alias/ { print $2 }' | awk '{ print $1 }')
+ [ "${alias}" = "testimage" ]
+
+ lxc delete c1
+ lxc remote remove l2
+ lxc image delete "${fp2}"
+ kill_lxd "$LXD2_DIR"
+}
More information about the lxc-devel
mailing list