The following pull request was submitted through Github. It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/8163
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) ===
From b62438c41c6b51c9776b8af468d84fbb51ccd9fa Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Tue, 17 Nov 2020 11:26:09 +0000 Subject: [PATCH 1/5] lxd/images: Add word "Image" to log line to give context Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- lxd/images.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lxd/images.go b/lxd/images.go index 4fb55b9087..123ac310d6 100644 --- a/lxd/images.go +++ b/lxd/images.go @@ -1241,7 +1241,7 @@ func autoUpdateImage(d *Daemon, op *operations.Operation, id int, info *api.Imag hash = newInfo.Fingerprint if hash == fingerprint { - logger.Debug("Already up to date", log.Ctx{"fp": fingerprint}) + logger.Debug("Image already up to date", log.Ctx{"fp": fingerprint}) continue } From 3833ce4d916d0d603713dc1f5ecb6b1a9611e35a Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Tue, 17 Nov 2020 11:27:07 +0000 Subject: [PATCH 2/5] lxd/daemon/images: Add contextual logging and use "fp" rather than "image" for consistency with other code areas Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- lxd/daemon_images.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/lxd/daemon_images.go b/lxd/daemon_images.go index 3a5b244d7e..a491ae4f4e 100644 --- a/lxd/daemon_images.go +++ b/lxd/daemon_images.go @@ -173,41 +173,45 @@ func (d *Daemon) ImageDownload(op *operations.Operation, server string, protocol } if imgInfo != nil { - logger.Debugf("Image %q already exists in the DB", fp) info = imgInfo + ctxMap = log.Ctx{"fp": info.Fingerprint} + logger.Debug("Image already exists in the DB", ctxMap) // If not requested in a particular pool, we're done. if storagePool == "" { return info, nil } - // Get the ID of the target storage pool + ctxMap["pool"] = storagePool + + // Get the ID of the target storage pool. poolID, err := d.cluster.GetStoragePoolID(storagePool) if err != nil { return nil, err } - // Check if the image is already in the pool + // Check if the image is already in the pool. poolIDs, err := d.cluster.GetPoolsWithImage(info.Fingerprint) if err != nil { return nil, err } if shared.Int64InSlice(poolID, poolIDs) { - logger.Debugf("Image already exists on storage pool %q", storagePool) + logger.Debug("Image already exists on storage pool", ctxMap) return info, nil } - // Import the image in the pool - logger.Debugf("Image does not exist on storage pool %q", storagePool) + // Import the image in the pool. + logger.Debug("Image does not exist on storage pool", ctxMap) err = imageCreateInPool(d, info, storagePool) if err != nil { - logger.Debugf("Failed to create image on storage pool %q: %v", storagePool, err) - return nil, err + ctxMap["err"] = err + logger.Debug("Failed to create image on storage pool", ctxMap) + return nil, errors.Wrapf(err, "Failed to create image %q on storage pool %q", info.Fingerprint, storagePool) } - logger.Debugf("Created image on storage pool %q", storagePool) + logger.Debugf("Created image on storage pool", ctxMap) return info, nil } @@ -217,9 +221,7 @@ func (d *Daemon) ImageDownload(op *operations.Operation, server string, protocol // We are already downloading the image imagesDownloadingLock.Unlock() - logger.Debug( - "Already downloading the image, waiting for it to succeed", - log.Ctx{"image": fp}) + logger.Debug("Already downloading the image, waiting for it to succeed", log.Ctx{"fp": fp}) // Wait until the download finishes (channel closes) <-waitChannel @@ -228,7 +230,7 @@ func (d *Daemon) ImageDownload(op *operations.Operation, server string, protocol _, imgInfo, err := d.cluster.GetImage(project, fp, false) if err != nil { // Other download failed, lets try again - logger.Error("Other image download didn't succeed", log.Ctx{"image": fp}) + logger.Error("Other image download didn't succeed", log.Ctx{"fp": fp}) } else { // Other download succeeded, we're done return imgInfo, nil @@ -256,7 +258,7 @@ func (d *Daemon) ImageDownload(op *operations.Operation, server string, protocol if op == nil { ctxMap = log.Ctx{"alias": alias, "server": server} } else { - ctxMap = log.Ctx{"trigger": op.URL(), "image": fp, "operation": op.ID(), "alias": alias, "server": server} + ctxMap = log.Ctx{"trigger": op.URL(), "fp": fp, "operation": op.ID(), "alias": alias, "server": server} } logger.Info("Downloading image", ctxMap) From d5dcf5465221a35baa96e7e4d20d2969b26a1490 Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Tue, 17 Nov 2020 11:28:14 +0000 Subject: [PATCH 3/5] lxd/profiles/utils: Remove container references, improve comments Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- lxd/profiles_utils.go | 96 +++++++++++++++++++++---------------------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/lxd/profiles_utils.go b/lxd/profiles_utils.go index 2a6aa30700..a601e140f1 100644 --- a/lxd/profiles_utils.go +++ b/lxd/profiles_utils.go @@ -7,7 +7,7 @@ import ( deviceConfig "github.com/lxc/lxd/lxd/device/config" "github.com/lxc/lxd/lxd/instance" "github.com/lxc/lxd/lxd/instance/instancetype" - projecthelpers "github.com/lxc/lxd/lxd/project" + "github.com/lxc/lxd/lxd/project" "github.com/lxc/lxd/shared" "github.com/lxc/lxd/shared/api" "github.com/pkg/errors" @@ -16,66 +16,69 @@ import ( func doProfileUpdate(d *Daemon, projectName string, name string, id int64, profile *api.Profile, req api.ProfilePut) error { // Check project limits. err := d.cluster.Transaction(func(tx *db.ClusterTx) error { - return projecthelpers.AllowProfileUpdate(tx, projectName, name, req) + return project.AllowProfileUpdate(tx, projectName, name, req) }) if err != nil { return err } - // Sanity checks + // Sanity checks. err = instance.ValidConfig(d.os, req.Config, true, false) if err != nil { return err } - // At this point we don't know the instance type, so just use instancetype.Any type for validation. + // Profiles can be applied to any instance type, so just use instancetype.Any type for validation so that + // instance type specific validation checks are not performed. err = instance.ValidDevices(d.State(), d.cluster, projectName, instancetype.Any, deviceConfig.NewDevices(req.Devices), false) if err != nil { return err } - containers, err := getProfileContainersInfo(d.cluster, projectName, name) + insts, err := getProfileInstancesInfo(d.cluster, projectName, name) if err != nil { - return errors.Wrapf(err, "failed to query instances associated with profile '%s'", name) + return errors.Wrapf(err, "Failed to query instances associated with profile %q", name) } - // Check if the root device is supposed to be changed or removed. + // Check if the root disk device's pool is supposed to be changed or removed and prevent that if there are + // instances using that root disk device. oldProfileRootDiskDeviceKey, oldProfileRootDiskDevice, _ := shared.GetRootDiskDevice(profile.Devices) _, newProfileRootDiskDevice, _ := shared.GetRootDiskDevice(req.Devices) - if len(containers) > 0 && oldProfileRootDiskDevice["pool"] != "" && newProfileRootDiskDevice["pool"] == "" || (oldProfileRootDiskDevice["pool"] != newProfileRootDiskDevice["pool"]) { - // Check for containers using the device - for _, container := range containers { - // Check if the device is locally overridden - k, v, _ := shared.GetRootDiskDevice(container.Devices.CloneNative()) + if len(insts) > 0 && oldProfileRootDiskDevice["pool"] != "" && newProfileRootDiskDevice["pool"] == "" || (oldProfileRootDiskDevice["pool"] != newProfileRootDiskDevice["pool"]) { + // Check for instances using the device. + for _, inst := range insts { + // Check if the device is locally overridden. + k, v, _ := shared.GetRootDiskDevice(inst.Devices.CloneNative()) if k != "" && v["pool"] != "" { continue } - // Check what profile the device comes from - profiles := container.Profiles + // Check what profile the device comes from by working backwards along the profiles list. + profiles := inst.Profiles for i := len(profiles) - 1; i >= 0; i-- { - _, profile, err := d.cluster.GetProfile(projecthelpers.Default, profiles[i]) + // tomp TODO project.Default here look suspicious. + _, profile, err := d.cluster.GetProfile(project.Default, profiles[i]) if err != nil { return err } - // Check if we find a match for the device + // Check if we find a match for the device. _, ok := profile.Devices[oldProfileRootDiskDeviceKey] if ok { - // Found the profile + // Found the profile. if profiles[i] == name { - // If it's the current profile, then we can't modify that root device + // If it's the current profile, then we can't modify that root device. return fmt.Errorf("At least one instance relies on this profile's root disk device") - } else { - // If it's not, then move on to the next container - break } + + // If it's not, then move on to the next instance. + break } } } } - // Update the database + // Update the database. err = d.cluster.Transaction(func(tx *db.ClusterTx) error { return tx.UpdateProfile(projectName, name, db.Profile{ Project: projectName, @@ -89,8 +92,7 @@ func doProfileUpdate(d *Daemon, projectName string, name string, id int64, profi return err } - // Update all the containers on this node using the profile. Must be - // done after db.TxCommit due to DB lock. + // Update all the instances on this node using the profile. Must be done after db.TxCommit due to DB lock. nodeName := "" err = d.cluster.Transaction(func(tx *db.ClusterTx) error { var err error @@ -101,15 +103,15 @@ func doProfileUpdate(d *Daemon, projectName string, name string, id int64, profi return errors.Wrap(err, "failed to query local node name") } failures := map[string]error{} - for _, args := range containers { - err := doProfileUpdateContainer(d, name, profile.ProfilePut, nodeName, args) + for _, args := range insts { + err := doProfileUpdateInstance(d, name, profile.ProfilePut, nodeName, args) if err != nil { failures[args.Name] = err } } if len(failures) != 0 { - msg := "The following containers failed to update (profile change still saved):\n" + msg := "The following instances failed to update (profile change still saved):\n" for cname, err := range failures { msg += fmt.Sprintf(" - %s: %s\n", cname, err) } @@ -132,21 +134,21 @@ func doProfileUpdateCluster(d *Daemon, project, name string, old api.ProfilePut) return errors.Wrap(err, "Failed to query local node name") } - containers, err := getProfileContainersInfo(d.cluster, project, name) + insts, err := getProfileInstancesInfo(d.cluster, project, name) if err != nil { - return errors.Wrapf(err, "Failed to query instances associated with profile '%s'", name) + return errors.Wrapf(err, "Failed to query instances associated with profile %q", name) } failures := map[string]error{} - for _, args := range containers { - err := doProfileUpdateContainer(d, name, old, nodeName, args) + for _, args := range insts { + err := doProfileUpdateInstance(d, name, old, nodeName, args) if err != nil { failures[args.Name] = err } } if len(failures) != 0 { - msg := "The following containers failed to update (profile change still saved):\n" + msg := "The following instances failed to update (profile change still saved):\n" for cname, err := range failures { msg += fmt.Sprintf(" - %s: %s\n", cname, err) } @@ -156,10 +158,10 @@ func doProfileUpdateCluster(d *Daemon, project, name string, old api.ProfilePut) return nil } -// Profile update of a single container. -func doProfileUpdateContainer(d *Daemon, name string, old api.ProfilePut, nodeName string, args db.InstanceArgs) error { +// Profile update of a single instance. +func doProfileUpdateInstance(d *Daemon, name string, old api.ProfilePut, nodeName string, args db.InstanceArgs) error { if args.Node != "" && args.Node != nodeName { - // No-op, this container does not belong to this node. + // No-op, this instance does not belong to this node. return nil } @@ -197,26 +199,24 @@ func doProfileUpdateContainer(d *Daemon, name string, old api.ProfilePut, nodeNa }, true) } -// Query the db for information about containers associated with the given -// profile. -func getProfileContainersInfo(cluster *db.Cluster, project, profile string) ([]db.InstanceArgs, error) { - // Query the db for information about containers associated with the - // given profile. - names, err := cluster.GetInstancesWithProfile(project, profile) +// Query the db for information about instances associated with the given profile. +func getProfileInstancesInfo(cluster *db.Cluster, projectName string, profileName string) ([]db.InstanceArgs, error) { + // Query the db for information about instances associated with the given profile. + projectInstNames, err := cluster.GetInstancesWithProfile(projectName, profileName) if err != nil { - return nil, errors.Wrapf(err, "Failed to query instances with profile '%s'", profile) + return nil, errors.Wrapf(err, "Failed to query instances with profile %q", profileName) } - containers := []db.InstanceArgs{} + instances := []db.InstanceArgs{} err = cluster.Transaction(func(tx *db.ClusterTx) error { - for ctProject, ctNames := range names { - for _, ctName := range ctNames { - container, err := tx.GetInstance(ctProject, ctName) + for instProject, instNames := range projectInstNames { + for _, instName := range instNames { + inst, err := tx.GetInstance(instProject, instName) if err != nil { return err } - containers = append(containers, db.InstanceToArgs(container)) + instances = append(instances, db.InstanceToArgs(inst)) } } @@ -226,5 +226,5 @@ func getProfileContainersInfo(cluster *db.Cluster, project, profile string) ([]d return nil, errors.Wrapf(err, "Failed to fetch instances") } - return containers, nil + return instances, nil } From 54ff08a9ec1941a2df0cad5df93b039bd7a2f4a0 Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Tue, 17 Nov 2020 11:28:52 +0000 Subject: [PATCH 4/5] lxd/db/profiles: Updates GetInstancesWithProfile to return all instance types, not just containers Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- lxd/db/profiles.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lxd/db/profiles.go b/lxd/db/profiles.go index f04eeabe26..809451beb1 100644 --- a/lxd/db/profiles.go +++ b/lxd/db/profiles.go @@ -216,8 +216,7 @@ func (c *Cluster) GetInstancesWithProfile(project, profile string) (map[string][ WHERE instances_profiles.profile_id == (SELECT profiles.id FROM profiles JOIN projects ON projects.id == profiles.project_id - WHERE profiles.name=? AND projects.name=?) - AND instances.type == 0` + WHERE profiles.name=? AND projects.name=?)` results := map[string][]string{} inargs := []interface{}{profile, project} From e45002d7b1cb87970d1eb7388cb181a69db36b06 Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Tue, 17 Nov 2020 11:29:19 +0000 Subject: [PATCH 5/5] shared/instance: Improves comments Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- shared/instance.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/shared/instance.go b/shared/instance.go index 023573a0cf..c79f618753 100644 --- a/shared/instance.go +++ b/shared/instance.go @@ -25,7 +25,7 @@ const ( ) // IsRootDiskDevice returns true if the given device representation is configured as root disk for -// a container. It typically get passed a specific entry of api.Instance.Devices. +// an instance. It typically get passed a specific entry of api.Instance.Devices. func IsRootDiskDevice(device map[string]string) bool { // Root disk devices also need a non-empty "pool" property, but we can't check that here // because this function is used with clients talking to older servers where there was no @@ -38,7 +38,8 @@ func IsRootDiskDevice(device map[string]string) bool { return false } -// GetRootDiskDevice returns the container device that is configured as root disk +// GetRootDiskDevice returns the instance device that is configured as root disk. +// Returns the device name and device config map. func GetRootDiskDevice(devices map[string]map[string]string) (string, map[string]string, error) { var devName string var dev map[string]string
_______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel