From 2e50006f1c5fbabdace58df779b205c8c8c1bd9b Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Mon, 24 Jul 2017 11:14:18 -0400 Subject: [PATCH 1/2] Avoid using lower-level storage APIs Switch from using the lower-level storage APIs (accessing LayerStore, ImageStore, and ContainerStore types directly) in favor of the higher-level ones that take care of synchronization and locking for us. Signed-off-by: Nalin Dahyabhai --- cmd/kpod/info.go | 38 ++++++++---------------------------- cmd/kpod/rmi.go | 13 ++----------- libkpod/container.go | 40 ++++++++++---------------------------- libkpod/image/imageData.go | 8 ++------ libkpod/image/rmi.go | 27 +++++++++---------------- 5 files changed, 31 insertions(+), 95 deletions(-) diff --git a/cmd/kpod/info.go b/cmd/kpod/info.go index 5a30c0ff..a094606d 100644 --- a/cmd/kpod/info.go +++ b/cmd/kpod/info.go @@ -134,42 +134,20 @@ func storeInfo(c *cli.Context) (string, map[string]interface{}, error) { info := map[string]interface{}{} info["GraphRoot"] = store.GraphRoot() info["GraphDriverName"] = store.GraphDriverName() - if is, err := store.ImageStore(); err != nil { + images, err := store.Images() + if err != nil { info["ImageStore"] = infoErr(err) } else { - images, err := is.Images() - if err != nil { - info["ImageStore"] = infoErr(err) - } else { - info["ImageStore"] = map[string]interface{}{ - "number": len(images), - } + info["ImageStore"] = map[string]interface{}{ + "number": len(images), } } - /* Oh this is in master on containers/storage, rebase later - if is, err := store.ROImageStores(); err != nil { - info["ROImageStore"] = infoErr(err) - } else { - images, err := is.Images() - if err != nil { - info["ROImageStore"] = infoErr(err) - } else { - info["ROImageStore"] = map[string]interface{}{ - "number": len(images), - } - } - } - */ - if cs, err := store.ContainerStore(); err != nil { + containers, err := store.Containers() + if err != nil { info["ContainerStore"] = infoErr(err) } else { - containers, err := cs.Containers() - if err != nil { - info["ContainerStore"] = infoErr(err) - } else { - info["ContainerStore"] = map[string]interface{}{ - "number": len(containers), - } + info["ContainerStore"] = map[string]interface{}{ + "number": len(containers), } } return "store", info, nil diff --git a/cmd/kpod/rmi.go b/cmd/kpod/rmi.go index 7b857a1a..78740ab8 100644 --- a/cmd/kpod/rmi.go +++ b/cmd/kpod/rmi.go @@ -96,12 +96,7 @@ func rmiCmd(c *cli.Context) error { // TODO: replace this with something in libkpod func runningContainers(image *storage.Image, store storage.Store) ([]string, error) { ctrIDs := []string{} - ctrStore, err := store.ContainerStore() - if err != nil { - return nil, err - } - - containers, err := ctrStore.Containers() + containers, err := store.Containers() if err != nil { return nil, err } @@ -115,12 +110,8 @@ func runningContainers(image *storage.Image, store storage.Store) ([]string, err // TODO: replace this with something in libkpod func removeContainers(ctrIDs []string, store storage.Store) error { - ctrStore, err := store.ContainerStore() - if err != nil { - return err - } for _, ctrID := range ctrIDs { - if err = ctrStore.Delete(ctrID); err != nil { + if err := store.DeleteContainer(ctrID); err != nil { return errors.Wrapf(err, "could not remove container %q", ctrID) } } diff --git a/libkpod/container.go b/libkpod/container.go index 9873271c..0de14df1 100644 --- a/libkpod/container.go +++ b/libkpod/container.go @@ -7,11 +7,7 @@ import ( // FindContainer searches for a container with the given name or ID in the given store func FindContainer(store cstorage.Store, container string) (*cstorage.Container, error) { - ctrStore, err := store.ContainerStore() - if err != nil { - return nil, err - } - return ctrStore.Get(container) + return store.Container(container) } // GetContainerTopLayerID gets the ID of the top layer of the given container @@ -25,15 +21,7 @@ func GetContainerTopLayerID(store cstorage.Store, containerID string) (string, e // GetContainerRwSize Gets the size of the mutable top layer of the container func GetContainerRwSize(store cstorage.Store, containerID string) (int64, error) { - ctrStore, err := store.ContainerStore() - if err != nil { - return 0, err - } - container, err := ctrStore.Get(containerID) - if err != nil { - return 0, err - } - lstore, err := store.LayerStore() + container, err := store.Container(containerID) if err != nil { return 0, err } @@ -41,11 +29,11 @@ func GetContainerRwSize(store cstorage.Store, containerID string) (int64, error) // Get the size of the top layer by calculating the size of the diff // between the layer and its parent. The top layer of a container is // the only RW layer, all others are immutable - layer, err := lstore.Get(container.LayerID) + layer, err := store.Layer(container.LayerID) if err != nil { return 0, err } - return lstore.DiffSize(layer.Parent, layer.ID) + return store.DiffSize(layer.Parent, layer.ID) } // GetContainerRootFsSize gets the size of the container's root filesystem @@ -53,38 +41,30 @@ func GetContainerRwSize(store cstorage.Store, containerID string) (int64, error) // mutable layer, and the rest is the RootFS: the set of immutable layers // that make up the image on which the container is based func GetContainerRootFsSize(store cstorage.Store, containerID string) (int64, error) { - ctrStore, err := store.ContainerStore() - if err != nil { - return 0, err - } - container, err := ctrStore.Get(containerID) - if err != nil { - return 0, err - } - lstore, err := store.LayerStore() + container, err := store.Container(containerID) if err != nil { return 0, err } // Ignore the size of the top layer. The top layer is a mutable RW layer // and is not considered a part of the rootfs - rwLayer, err := lstore.Get(container.LayerID) + rwLayer, err := store.Layer(container.LayerID) if err != nil { return 0, err } - layer, err := lstore.Get(rwLayer.Parent) + layer, err := store.Layer(rwLayer.Parent) if err != nil { return 0, err } size := int64(0) for layer.Parent != "" { - layerSize, err := lstore.DiffSize(layer.Parent, layer.ID) + layerSize, err := store.DiffSize(layer.Parent, layer.ID) if err != nil { return size, errors.Wrapf(err, "getting diffsize of layer %q and its parent %q", layer.ID, layer.Parent) } size += layerSize - layer, err = lstore.Get(layer.Parent) + layer, err = store.Layer(layer.Parent) if err != nil { return 0, err } @@ -92,6 +72,6 @@ func GetContainerRootFsSize(store cstorage.Store, containerID string) (int64, er // Get the size of the last layer. Has to be outside of the loop // because the parent of the last layer is "", andlstore.Get("") // will return an error - layerSize, err := lstore.DiffSize(layer.Parent, layer.ID) + layerSize, err := store.DiffSize(layer.Parent, layer.ID) return size + layerSize, err } diff --git a/libkpod/image/imageData.go b/libkpod/image/imageData.go index eff4c484..390b4ebe 100644 --- a/libkpod/image/imageData.go +++ b/libkpod/image/imageData.go @@ -112,15 +112,11 @@ func GetImageData(store storage.Store, name string) (*ImageData, error) { return nil, err } - lstore, err := store.LayerStore() + layer, err := store.Layer(topLayerID) if err != nil { return nil, err } - layer, err := lstore.Get(topLayerID) - if err != nil { - return nil, err - } - size, err := lstore.DiffSize(layer.Parent, layer.ID) + size, err := store.DiffSize(layer.Parent, layer.ID) if err != nil { return nil, err } diff --git a/libkpod/image/rmi.go b/libkpod/image/rmi.go index db17088d..1e968d18 100644 --- a/libkpod/image/rmi.go +++ b/libkpod/image/rmi.go @@ -7,11 +7,7 @@ import ( // UntagImage removes the tag from the given image func UntagImage(store storage.Store, image *storage.Image, imgArg string) (string, error) { - // Remove name from image.Names and set the new name in the ImageStore - imgStore, err := store.ImageStore() - if err != nil { - return "", errors.Wrap(err, "could not untag image") - } + // Remove name from image.Names and set the new names newNames := []string{} removedName := "" for _, name := range image.Names { @@ -21,24 +17,19 @@ func UntagImage(store storage.Store, image *storage.Image, imgArg string) (strin } newNames = append(newNames, name) } - imgStore.SetNames(image.ID, newNames) - err = imgStore.Save() - return removedName, err + if removedName != "" { + if err := store.SetNames(image.ID, newNames); err != nil { + return "", errors.Wrapf(err, "error removing name %q from image %q", removedName, image.ID) + } + } + return removedName, nil } // RemoveImage removes the given image from storage func RemoveImage(image *storage.Image, store storage.Store) (string, error) { - imgStore, err := store.ImageStore() + _, err := store.DeleteImage(image.ID, true) if err != nil { - return "", errors.Wrapf(err, "could not open image store") - } - err = imgStore.Delete(image.ID) - if err != nil { - return "", errors.Wrapf(err, "could not remove image") - } - err = imgStore.Save() - if err != nil { - return "", errors.Wrapf(err, "could not save image store") + return "", errors.Wrapf(err, "could not remove image %q", image.ID) } return image.ID, nil } From 3747048aa492e7a3c832c97f1828e00fd8c897cc Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Tue, 25 Jul 2017 13:30:54 -0400 Subject: [PATCH 2/2] Don't leak containers/image Image references In-memory image objects created using an ImageReference's NewImage() method need to be Close()d. Signed-off-by: Nalin Dahyabhai --- libkpod/image/image.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libkpod/image/image.go b/libkpod/image/image.go index f2896d17..2a840d50 100644 --- a/libkpod/image/image.go +++ b/libkpod/image/image.go @@ -96,6 +96,7 @@ func matchesLabel(image storage.Image, store storage.Store, label string) bool { if err != nil { return false } + defer img.Close() info, err := img.Inspect() if err != nil { return false @@ -229,6 +230,7 @@ func Size(store storage.Store, img storage.Image) (int64, error) { if err != nil { return -1, err } + defer imgRef.Close() imgSize, err := imgRef.Size() if err != nil { return -1, err