Merge pull request #837 from shomron/issue-624-garbage-collection

Garbage collection for vendored packages

pkg/actions/actions_test.go

@@ -245,6 +245,7 @@ func withApp(t *testing.T, fn func(*mocks.App)) {
 	appMock := &mocks.App{}
 	appMock.On("Fs").Return(fs)
 	appMock.On("Root").Return("/")
+	appMock.On("VendorPath").Return("/vendor")
 
 	fn(appMock)
 }

pkg/actions/pkg_describe.go

@@ -21,6 +21,7 @@ import (
 	"text/template"
 
 	"github.com/ksonnet/ksonnet/pkg/app"
+	"github.com/ksonnet/ksonnet/pkg/pkg"
 	"github.com/ksonnet/ksonnet/pkg/registry"
 )
 
@@ -69,7 +70,12 @@ func NewPkgDescribe(m map[string]interface{}) (*PkgDescribe, error) {
 
 // Run describes a package.
 func (pd *PkgDescribe) Run() error {
-	p, err := pd.packageManager.Find(pd.pkgName)
+	d, err := pkg.Parse(pd.pkgName)
+	if err != nil {
+		return err
+	}
+
+	p, err := pd.packageManager.Find(d)
 	if err != nil {
 		return err
 	}

pkg/actions/pkg_describe_test.go

@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	"github.com/ksonnet/ksonnet/pkg/app"
+	"github.com/ksonnet/ksonnet/pkg/pkg"
 	"github.com/ksonnet/ksonnet/pkg/prototype"
 	"github.com/pkg/errors"
 
@@ -34,6 +35,7 @@ import (
 )
 
 func TestPkgDescribe(t *testing.T) {
+	d := pkg.Descriptor{Name: "apache"}
 
 	cases := []struct {
 		name        string
@@ -52,7 +54,7 @@ func TestPkgDescribe(t *testing.T) {
 				p.On("IsInstalled").Return(false, nil)
 
 				pkgManager := &regmocks.PackageManager{}
-				pkgManager.On("Find", "apache").Return(p, nil)
+				pkgManager.On("Find", d).Return(p, nil)
 
 				return pkgManager
 			},
@@ -76,7 +78,7 @@ func TestPkgDescribe(t *testing.T) {
 				p.On("Prototypes").Return(prototypes, nil)
 
 				pkgManager := &regmocks.PackageManager{}
-				pkgManager.On("Find", "apache").Return(p, nil)
+				pkgManager.On("Find", d).Return(p, nil)
 
 				return pkgManager
 			},
@@ -86,7 +88,7 @@ func TestPkgDescribe(t *testing.T) {
 			isErr: true,
 			pkgManager: func() registry.PackageManager {
 				pkgManager := &regmocks.PackageManager{}
-				pkgManager.On("Find", "apache").Return(nil, errors.New("failed"))
+				pkgManager.On("Find", d).Return(nil, errors.New("failed"))
 
 				return pkgManager
 			},
@@ -100,7 +102,7 @@ func TestPkgDescribe(t *testing.T) {
 				p.On("IsInstalled").Return(false, errors.New("failed"))
 
 				pkgManager := &regmocks.PackageManager{}
-				pkgManager.On("Find", "apache").Return(p, nil)
+				pkgManager.On("Find", d).Return(p, nil)
 
 				return pkgManager
 			},
@@ -115,7 +117,7 @@ func TestPkgDescribe(t *testing.T) {
 				p.On("IsInstalled").Return(true, nil)
 
 				pkgManager := &regmocks.PackageManager{}
-				pkgManager.On("Find", "apache").Return(p, nil)
+				pkgManager.On("Find", d).Return(p, nil)
 
 				return pkgManager
 			},
@@ -129,7 +131,7 @@ func TestPkgDescribe(t *testing.T) {
 				p.On("IsInstalled").Return(false, nil)
 
 				pkgManager := &regmocks.PackageManager{}
-				pkgManager.On("Find", "apache").Return(p, nil)
+				pkgManager.On("Find", d).Return(p, nil)
 
 				return pkgManager
 			},

pkg/actions/pkg_install.go

@@ -46,6 +46,7 @@ type PkgInstall struct {
 	envName      string
 	force        bool
 	checker      registry.InstalledChecker
+	gc           registry.GarbageCollector
 	libCacherFn  libCacher
 	libUpdateFn  libUpdater
 	envCheckerFn envChecker
@@ -62,13 +63,16 @@ func NewPkgInstall(m map[string]interface{}) (*PkgInstall, error) {
 	httpClient := ol.LoadHTTPClient()
 	httpClientOpt := registry.HTTPClientOpt(httpClient)
 
+	pm := registry.NewPackageManager(a, httpClientOpt)
+
 	nl := &PkgInstall{
 		app:        a,
 		libName:    ol.LoadString(OptionPkgName),
 		customName: ol.LoadString(OptionName),
 		force:      ol.LoadBool(OptionForce),
 		envName:    ol.LoadOptionalString(OptionEnvName),
-		checker:    registry.NewPackageManager(a, httpClientOpt),
+		checker:    pm,
+		gc:         registry.NewGarbageCollector(a.Fs(), pm, a.VendorPath()),
 
 		libCacherFn: func(a app.App, checker registry.InstalledChecker, d pkg.Descriptor, customName string, force bool) (*app.LibraryConfig, error) {
 			return registry.CacheDependency(a, checker, d, customName, force, httpClient)
@@ -123,7 +127,14 @@ func (pi *PkgInstall) Run() error {
 		return nil
 	}
 
-	// TODO Garbage collector hook here
+	// Optionally remove any orphaned vendor directories
+	if err := pi.gc.RemoveOrphans(pkg.Descriptor{
+		Registry: oldCfg.Registry,
+		Name:     oldCfg.Name,
+		Version:  oldCfg.Version,
+	}); err != nil {
+		return errors.Wrapf(err, "garbage collection for package %v", oldCfg)
+	}
 
 	return nil
 }

pkg/actions/pkg_remove.go

@@ -19,6 +19,8 @@ import (
 	"github.com/ksonnet/ksonnet/pkg/app"
 	"github.com/ksonnet/ksonnet/pkg/pkg"
 	"github.com/ksonnet/ksonnet/pkg/registry"
+	"github.com/pkg/errors"
+	log "github.com/sirupsen/logrus"
 )
 
 // PkgRemove removes packages
@@ -27,6 +29,7 @@ type PkgRemove struct {
 	pkgName     string
 	envName     string
 	checker     registry.InstalledChecker
+	gc          registry.GarbageCollector
 	libUpdateFn libUpdater
 }
 
@@ -39,11 +42,14 @@ func NewPkgRemove(m map[string]interface{}) (*PkgRemove, error) {
 		return nil, ol.err
 	}
 
+	pm := registry.NewPackageManager(a)
+
 	pr := &PkgRemove{
 		app:         a,
 		pkgName:     ol.LoadString(OptionPkgName),
 		envName:     ol.LoadOptionalString(OptionEnvName),
 		libUpdateFn: a.UpdateLib,
+		gc:          registry.NewGarbageCollector(a.Fs(), pm, a.VendorPath()),
 	}
 
 	if ol.err != nil {
@@ -79,6 +85,16 @@ func (pr *PkgRemove) Run() error {
 		return nil
 	}
 
-	// TODO: Garbage collection hook goes here
+	log.Infof("Removing package %v", oldCfg)
+
+	// Optionally remove any orphaned vendor directories
+	if err := pr.gc.RemoveOrphans(pkg.Descriptor{
+		Registry: oldCfg.Registry,
+		Name:     oldCfg.Name,
+		Version:  oldCfg.Version,
+	}); err != nil {
+		return errors.Wrapf(err, "garbage collection for package %v", oldCfg)
+	}
+
 	return nil
 }

pkg/app/schema.go

@@ -563,3 +563,19 @@ func (s *Spec) UpdateEnvironmentConfig(name string, env *EnvironmentConfig) erro
 	s.Environments[env.Name] = env
 	return nil
 }
+
+func (l LibraryConfig) String() string {
+	switch {
+	case l.Registry != "" && l.Version != "":
+		return fmt.Sprintf("%s/%s@%s", l.Registry, l.Name, l.Version)
+	case l.Registry != "" && l.Version == "":
+		return fmt.Sprintf("%s/%s", l.Registry, l.Name)
+	case l.Registry == "" && l.Version != "":
+		return fmt.Sprintf("%s@%s", l.Name, l.Version)
+	case l.Registry == "" && l.Version == "":
+		return l.Name
+	default:
+		// Not sure which case we missed, just default to verbose
+		return fmt.Sprintf("%s/%s@%s", l.Registry, l.Name, l.Version)
+	}
+}

pkg/pkg/helm.go

@@ -198,3 +198,16 @@ func (h *Helm) Path() string {
 	}
 	return path
 }
+
+// HelmVendorPath returns a path for vendoring the described package.
+func HelmVendorPath(a app.App, d Descriptor) string {
+	if a == nil {
+		return ""
+	}
+
+	path, err := chartConfigDir(a, d.Name, d.Registry, d.Version)
+	if err != nil {
+		return ""
+	}
+	return path
+}

pkg/pkg/local.go

@@ -204,3 +204,12 @@ func (l *Local) Path() string {
 
 	return buildPath(l.a, l.registryName, l.name, l.version)
 }
+
+// LocalVendorPath returns a path for vendoring the described package.
+func LocalVendorPath(a app.App, d Descriptor) string {
+	if a == nil {
+		return ""
+	}
+
+	return buildPath(a, d.Registry, d.Name, d.Version)
+}

pkg/pkg/name.go

@@ -34,21 +34,42 @@ type Descriptor struct {
 	Version  string
 }
 
+func (d Descriptor) String() string {
+	switch {
+	case d.Registry != "" && d.Version != "":
+		return fmt.Sprintf("%s/%s@%s", d.Registry, d.Name, d.Version)
+	case d.Registry != "" && d.Version == "":
+		return fmt.Sprintf("%s/%s", d.Registry, d.Name)
+	case d.Registry == "" && d.Version != "":
+		return fmt.Sprintf("%s@%s", d.Name, d.Version)
+	case d.Registry == "" && d.Version == "":
+		return d.Name
+	default:
+		// Not sure which case we missed, just default to verbose
+		return fmt.Sprintf("%s/%s@%s", d.Registry, d.Name, d.Version)
+	}
+}
+
 // Parse parses a package identifier into its components
 // <registry>/<name>@<version>
 func Parse(id string) (Descriptor, error) {
+	var registry, name, version string
+
 	matches := reDescriptor.FindStringSubmatch(id)
 	if len(matches) == 0 {
 		return Descriptor{}, errInvalidSpec
 	}
 
 	if matches[2] == "" {
-		return Descriptor{Name: strings.TrimPrefix(matches[1], "/")}, nil
+		// No registry
+		name = strings.TrimPrefix(matches[1], "/")
+	} else {
+		// Registry and name
+		registry = matches[1]
+		name = strings.TrimPrefix(matches[2], "/")
 	}
 
-	registry := matches[1]
-	name := strings.TrimPrefix(matches[2], "/")
-	version := strings.TrimPrefix(matches[3], "@")
+	version = strings.TrimPrefix(matches[3], "@")
 
 	return Descriptor{
 		Registry: registry,

pkg/pkg/name_test.go

@@ -39,6 +39,10 @@ func Test_Parse(t *testing.T) {
 			name:     "parts-infra/contour@0.1.0",
 			expected: Descriptor{Registry: "parts-infra", Name: "contour", Version: "0.1.0"},
 		},
+		{
+			name:     "contour@0.1.0",
+			expected: Descriptor{Registry: "", Name: "contour", Version: "0.1.0"},
+		},
 		{
 			name:  "@foo/bar@baz@doh",
 			isErr: true,

pkg/pkg/pkg.go

@@ -98,12 +98,22 @@ func (ic *DefaultInstallChecker) IsInstalled(name string) (bool, error) {
 		return false, errors.New("app is nil")
 	}
 
+	d, err := Parse(name)
+	if err != nil {
+		return false, errors.Wrapf(err, "parsing package descriptor: %s", name)
+	}
+
 	libs, err := ic.App.Libraries()
 	if err != nil {
 		return false, errors.Wrapf(err, "checking if package %q is installed", name)
 	}
 
-	_, isGlobal := libs[name]
+	var isGlobal bool
+	if l, ok := libs[d.Name]; ok {
+		if d.Version == "" || l.Version == d.Version {
+			isGlobal = true
+		}
+	}
 
 	envs, err := ic.App.Environments()
 	if err != nil {
@@ -112,9 +122,11 @@ func (ic *DefaultInstallChecker) IsInstalled(name string) (bool, error) {
 
 	var isLocal bool
 	for _, e := range envs {
-		_, isLocal = e.Libraries[name]
-		if isLocal {
-			break
+		if l, ok := e.Libraries[d.Name]; ok {
+			if d.Version == "" || l.Version == d.Version {
+				isLocal = true
+				break
+			}
 		}
 	}
 

pkg/pkg/pkg_test.go

@@ -29,15 +29,17 @@ import (
 func Test_DefaultInstallChecker_isInstalled(t *testing.T) {
 	cases := []struct {
 		name           string
+		libName        string
 		setupLibraries func(*amocks.App)
 		isInstalled    bool
 		isErr          bool
 	}{
 		{
-			name: "is installed globally",
+			name:    "is installed globally",
+			libName: "redis",
 			setupLibraries: func(a *amocks.App) {
 				libraries := app.LibraryConfigs{
-					"redis": &app.LibraryConfig{},
+					"redis": &app.LibraryConfig{Version: "5.0.0"},
 				}
 
 				a.On("Libraries").Return(libraries, nil)
@@ -46,14 +48,42 @@ func Test_DefaultInstallChecker_isInstalled(t *testing.T) {
 			isInstalled: true,
 		},
 		{
-			name: "not installed",
+			name:    "is installed globally - match version",
+			libName: "redis@5.0.0",
+			setupLibraries: func(a *amocks.App) {
+				libraries := app.LibraryConfigs{
+					"redis": &app.LibraryConfig{Version: "5.0.0"},
+				}
+
+				a.On("Libraries").Return(libraries, nil)
+				a.On("Environments").Return(app.EnvironmentConfigs{}, nil)
+			},
+			isInstalled: true,
+		},
+		{
+			name:    "is installed globally - version mismatch",
+			libName: "redis@5.0.1",
+			setupLibraries: func(a *amocks.App) {
+				libraries := app.LibraryConfigs{
+					"redis": &app.LibraryConfig{Version: "5.0.0"},
+				}
+
+				a.On("Libraries").Return(libraries, nil)
+				a.On("Environments").Return(app.EnvironmentConfigs{}, nil)
+			},
+			isInstalled: false,
+		},
+		{
+			name:    "not installed",
+			libName: "redis",
 			setupLibraries: func(a *amocks.App) {
 				a.On("Libraries").Return(nil, nil)
 				a.On("Environments").Return(app.EnvironmentConfigs{}, nil)
 			},
 		},
 		{
-			name: "libraries error",
+			name:    "libraries error",
+			libName: "redis",
 			setupLibraries: func(a *amocks.App) {
 				a.On("Libraries").Return(nil, errors.New("failed"))
 				a.On("Environments").Return(app.EnvironmentConfigs{}, nil)
@@ -61,10 +91,29 @@ func Test_DefaultInstallChecker_isInstalled(t *testing.T) {
 			isErr: true,
 		},
 		{
-			name: "is installed in an environment",
+			name:    "is installed in an environment",
+			libName: "redis",
 			setupLibraries: func(a *amocks.App) {
 				libraries := app.LibraryConfigs{
-					"redis": &app.LibraryConfig{},
+					"redis": &app.LibraryConfig{Version: "5.0.0"},
+				}
+
+				a.On("Libraries").Return(app.LibraryConfigs{}, nil)
+				a.On("Environments").Return(app.EnvironmentConfigs{
+					"default": &app.EnvironmentConfig{
+						Name:      "default",
+						Libraries: libraries,
+					},
+				}, nil)
+			},
+			isInstalled: true,
+		},
+		{
+			name:    "is installed in an environment - match version",
+			libName: "redis@5.0.0",
+			setupLibraries: func(a *amocks.App) {
+				libraries := app.LibraryConfigs{
+					"redis": &app.LibraryConfig{Version: "5.0.0"},
 				}
 
 				a.On("Libraries").Return(app.LibraryConfigs{}, nil)
@@ -78,7 +127,26 @@ func Test_DefaultInstallChecker_isInstalled(t *testing.T) {
 			isInstalled: true,
 		},
 		{
-			name: "is installed both globally and in environment",
+			name:    "is installed in an environment - version mismatch",
+			libName: "redis@5.0.1",
+			setupLibraries: func(a *amocks.App) {
+				libraries := app.LibraryConfigs{
+					"redis": &app.LibraryConfig{Version: "5.0.0"},
+				}
+
+				a.On("Libraries").Return(app.LibraryConfigs{}, nil)
+				a.On("Environments").Return(app.EnvironmentConfigs{
+					"default": &app.EnvironmentConfig{
+						Name:      "default",
+						Libraries: libraries,
+					},
+				}, nil)
+			},
+			isInstalled: false,
+		},
+		{
+			name:    "is installed both globally and in environment",
+			libName: "redis",
 			setupLibraries: func(a *amocks.App) {
 				libraries := app.LibraryConfigs{
 					"redis": &app.LibraryConfig{},
@@ -103,7 +171,7 @@ func Test_DefaultInstallChecker_isInstalled(t *testing.T) {
 
 				ic := DefaultInstallChecker{App: a}
 
-				i, err := ic.IsInstalled("redis")
+				i, err := ic.IsInstalled(tc.libName)
 				if tc.isErr {
 					require.Error(t, err)
 					return

pkg/registry/cache.go

@@ -152,16 +152,11 @@ func versionAndVendorRelPath(lib *app.LibraryConfig, vendorRoot string, relPath
 	// Version the path
 	var versionedPath string
 	if lib.Version != "" {
-		//filepath.ToSlash()
 		parts := strings.SplitN(filepath.ToSlash(relPath), "/", -1)
 		if parts[0] == lib.Name {
 			parts[0] = fmt.Sprintf("%s@%s", lib.Name, lib.Version)
 		}
 		versionedPath = filepath.FromSlash(strings.Join(parts, "/"))
-
-		// oldPrefix := filepath.Join(lib.Registry, lib.Name)
-		// newPrefix := fmt.Sprintf("%s@%s", lib.Name, lib.Version)
-		// versionedPath = strings.Replace(relPath, oldPrefix, newPrefix, 1)
 	} else {
 		// For unversioned packages, use path as-is
 		versionedPath = relPath

pkg/registry/gc.go

+// Copyright 2018 The ksonnet authors
+//
+//
+//    Licensed under the Apache License, Version 2.0 (the "License");
+//    you may not use this file except in compliance with the License.
+//    You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+//    Unless required by applicable law or agreed to in writing, software
+//    distributed under the License is distributed on an "AS IS" BASIS,
+//    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//    See the License for the specific language governing permissions and
+//    limitations under the License.
+
+package registry
+
+import (
+	"io"
+	"path/filepath"
+	"strings"
+
+	"github.com/ksonnet/ksonnet/pkg/pkg"
+	"github.com/pkg/errors"
+	log "github.com/sirupsen/logrus"
+	"github.com/spf13/afero"
+)
+
+// FsRemoveAller Subset of afero.Fs - just remove a directory
+type FsRemoveAller interface {
+	// RemoveAll removes a directory path and any children it contains. It
+	// does not fail if the path does not exist (return nil).
+	RemoveAll(path string) error
+}
+
+type vendorPathResolver interface {
+	InstalledChecker
+	VendorPath(pkg.Descriptor) (string, error)
+}
+
+// GarbageCollector removes vendored packages that are no longer needed
+type GarbageCollector struct {
+	pkgManager vendorPathResolver
+	fs         FsRemoveAller
+	root       string
+
+	removeEmptyParentsFn func(path string, root string) error
+}
+
+// NewGarbageCollector constructs a GarbageCollector
+func NewGarbageCollector(fs afero.Fs, pm vendorPathResolver, root string) GarbageCollector {
+	return GarbageCollector{
+		pkgManager: pm,
+		fs:         fs,
+		root:       root,
+
+		removeEmptyParentsFn: func(path string, root string) error {
+			return removeEmptyParents(fs, path, root)
+		},
+	}
+}
+
+// RemoveOrphans removes vendored packages that have been orphaned
+func (gc GarbageCollector) RemoveOrphans(d pkg.Descriptor) error {
+	log := log.WithField("action", "GarbageCollector.RemoveOrphans")
+	installed, err := gc.pkgManager.IsInstalled(d)
+	if err != nil {
+		return errors.Wrapf(err, "checking installed status: %v", d)
+	}
+	// Only remove orphans
+	if installed {
+		return nil
+	}
+
+	path, err := gc.pkgManager.VendorPath(d)
+	if err != nil {
+		return errors.Wrapf(err, "resolving path for descriptor: %v", d)
+	}
+
+	if path == "" {
+		return nil
+	}
+
+	if gc.fs == nil {
+		return errors.New("nil fs")
+	}
+	log.Debugf("removing path %s", path)
+	if err := gc.fs.RemoveAll(path); err != nil {
+		return errors.Wrapf(err, "removing path %s for package %v", path, d)
+	}
+
+	if gc.removeEmptyParentsFn == nil {
+		return nil
+	}
+	if err := gc.removeEmptyParentsFn(path, gc.root); err != nil {
+		return errors.Wrapf(err, "removing empty parents path %s", path)
+	}
+
+	return nil
+}
+
+// Returns true if the specified directory is empty
+func isDirEmpty(fs afero.Fs, path string) (bool, error) {
+	if fs == nil {
+		return false, errors.New("nil fs")
+	}
+	f, err := fs.Open(path)
+	if err != nil {
+		return false, err
+	}
+	defer f.Close()
+	_, err = f.Readdirnames(1)
+	if err == io.EOF {
+		return true, nil
+	}
+	return false, err
+}
+
+// Remove empty directories, up to the provided root, exclusive
+func removeEmptyParents(fs afero.Fs, path string, root string) error {
+	if fs == nil {
+		return errors.New("nil fs")
+	}
+
+	root = filepath.Clean(root)