Merge pull request #497 from wking/load-assets-on-demand

pkg/asset/filefetcher: Only load files on demand

docs/design/assetgeneration.md

@@ -63,8 +63,8 @@ type File struct {
 // to read specific file(s) from disk.
 type FileFetcher interface {
     // FetchByName returns the file with the given name.
-    FetchByName(string) *File
-    // FetchByPattern returns the files whose name match the given regexp.
+    FetchByName(string) (*File, error)
+    // FetchByPattern returns the files whose name match the given glob.
     FetchByPattern(*regexp.Regexp) ([]*File, error)
 }
 ```

pkg/asset/cluster/cluster.go

@@ -159,8 +159,13 @@ func (c *Cluster) Files() []*asset.File {
 // Load returns error if the tfstate file is already on-disk, because we want to
 // prevent user from accidentally re-launching the cluster.
 func (c *Cluster) Load(f asset.FileFetcher) (found bool, err error) {
-	if f.FetchByName(stateFileName) != nil {
-		return true, fmt.Errorf("%q already exisits", stateFileName)
+	_, err = f.FetchByName(stateFileName)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, nil
+		}
+		return false, err
 	}
-	return false, nil
+
+	return true, fmt.Errorf("%q already exisits.  There may already be a running cluster", stateFileName)
 }

pkg/asset/cluster/tfvars.go

@@ -1,6 +1,8 @@
 package cluster
 
 import (
+	"os"
+
 	"github.com/openshift/installer/pkg/asset"
 	"github.com/openshift/installer/pkg/asset/ignition/bootstrap"
 	"github.com/openshift/installer/pkg/asset/ignition/machine"
@@ -77,9 +79,12 @@ func (t *TerraformVariables) Files() []*asset.File {
 
 // Load reads the terraform.tfvars from disk.
 func (t *TerraformVariables) Load(f asset.FileFetcher) (found bool, err error) {
-	file := f.FetchByName(tfvarsFilename)
-	if file == nil {
-		return false, nil
+	file, err := f.FetchByName(tfvarsFilename)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, nil
+		}
+		return false, err
 	}
 
 	t.File = file

pkg/asset/filefetcher.go

@@ -2,83 +2,56 @@ package asset
 
 import (
 	"io/ioutil"
-	"os"
 	"path/filepath"
-	"regexp"
 	"sort"
 )
 
 // FileFetcher fetches the asset files from disk.
 type FileFetcher interface {
 	// FetchByName returns the file with the given name.
-	FetchByName(string) *File
-	// FetchByPattern returns the files whose name match the given regexp.
-	FetchByPattern(*regexp.Regexp) []*File
+	FetchByName(string) (*File, error)
+	// FetchByPattern returns the files whose name match the given glob.
+	FetchByPattern(pattern string) ([]*File, error)
 }
 
 type fileFetcher struct {
-	onDiskAssets map[string][]byte
-}
-
-func newFileFetcher(clusterDir string) (*fileFetcher, error) {
-	fileMap := make(map[string][]byte)
-
-	// Don't bother if the clusterDir is not created yet because that
-	// means there's no assets generated yet.
-	_, err := os.Stat(clusterDir)
-	if err != nil && os.IsNotExist(err) {
-		return &fileFetcher{}, nil
-	}
-
-	if err := filepath.Walk(clusterDir, func(path string, info os.FileInfo, err error) error {
-		if err != nil {
-			return err
-		}
-
-		if info.IsDir() {
-			return nil
-		}
-
-		filename, err := filepath.Rel(clusterDir, path)
-		if err != nil {
-			return err
-		}
-
-		data, err := ioutil.ReadFile(path)
-		if err != nil {
-			return err
-		}
-
-		fileMap[filename] = data
-		return nil
-	}); err != nil {
-		return nil, err
-	}
-	return &fileFetcher{onDiskAssets: fileMap}, nil
+	directory string
 }
 
 // FetchByName returns the file with the given name.
-func (f *fileFetcher) FetchByName(name string) *File {
-	data, ok := f.onDiskAssets[name]
-	if !ok {
-		return nil
+func (f *fileFetcher) FetchByName(name string) (*File, error) {
+	data, err := ioutil.ReadFile(filepath.Join(f.directory, name))
+	if err != nil {
+		return nil, err
 	}
-	return &File{Filename: name, Data: data}
+	return &File{Filename: name, Data: data}, nil
 }
 
 // FetchByPattern returns the files whose name match the given regexp.
-func (f *fileFetcher) FetchByPattern(re *regexp.Regexp) []*File {
-	var files []*File
+func (f *fileFetcher) FetchByPattern(pattern string) (files []*File, err error) {
+	matches, err := filepath.Glob(filepath.Join(f.directory, pattern))
+	if err != nil {
+		return nil, err
+	}
 
-	for filename, data := range f.onDiskAssets {
-		if re.MatchString(filename) {
-			files = append(files, &File{
-				Filename: filename,
-				Data:     data,
-			})
+	files = make([]*File, 0, len(matches))
+	for _, path := range matches {
+		data, err := ioutil.ReadFile(path)
+		if err != nil {
+			return nil, err
 		}
+
+		filename, err := filepath.Rel(f.directory, path)
+		if err != nil {
+			return nil, err
+		}
+
+		files = append(files, &File{
+			Filename: filename,
+			Data:     data,
+		})
 	}
 
 	sort.Slice(files, func(i, j int) bool { return files[i].Filename < files[j].Filename })
-	return files
+	return files, nil
 }

pkg/asset/filefetcher_test.go

@@ -1,8 +1,9 @@
 package asset
 
 import (
-	"fmt"
-	"regexp"
+	"io/ioutil"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -15,18 +16,6 @@ func TestFetchByName(t *testing.T) {
 		input      string
 		expectFile *File
 	}{
-		{
-			name:       "only dirs",
-			files:      nil,
-			input:      "",
-			expectFile: nil,
-		},
-		{
-			name:       "input empty",
-			files:      map[string][]byte{"foo.bar": []byte("some data")},
-			input:      "",
-			expectFile: nil,
-		},
 		{
 			name:       "input doesn't match",
 			files:      map[string][]byte{"foo.bar": []byte("some data")},
@@ -55,43 +44,89 @@ func TestFetchByName(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			f := &fileFetcher{onDiskAssets: tt.files}
-			file := f.FetchByName(tt.input)
+			tempDir, err := ioutil.TempDir("", "openshift-install-")
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer os.RemoveAll(tempDir)
+
+			for filename, data := range tt.files {
+				err = ioutil.WriteFile(filepath.Join(tempDir, filename), data, 0666)
+				if err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			f := &fileFetcher{directory: tempDir}
+			file, err := f.FetchByName(tt.input)
+			if err != nil {
+				if os.IsNotExist(err) && tt.expectFile == nil {
+					return
+				}
+				t.Fatal(err)
+			}
+
 			assert.Equal(t, tt.expectFile, file)
 		})
 	}
 }
 
 func TestFetchByPattern(t *testing.T) {
+	tempDir, err := ioutil.TempDir("", "openshift-install-")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tempDir)
+
+	files := map[string][]byte{
+		"master-0.ign":   []byte("some data 0"),
+		"master-1.ign":   []byte("some data 1"),
+		"master-2.ign":   []byte("some data 2"),
+		"master-10.ign":  []byte("some data 3"),
+		"master-20.ign":  []byte("some data 4"),
+		"master-00.ign":  []byte("some data 5"),
+		"master-01.ign":  []byte("some data 6"),
+		"amaster-0.ign":  []byte("some data 7"),
+		"master-.ign":    []byte("some data 8"),
+		"master-.igni":   []byte("some data 9"),
+		"master-.ignign": []byte("some data 10"),
+		"manifests/0":    []byte("some data 11"),
+		"manifests/some": []byte("some data 12"),
+		"amanifests/a":   []byte("some data 13"),
+	}
+
+	for path, data := range files {
+		dir := filepath.Dir(path)
+		if dir != "." {
+			err := os.MkdirAll(filepath.Join(tempDir, dir), 0777)
+			if err != nil {
+				t.Fatal(err)
+			}
+		}
+		err = ioutil.WriteFile(filepath.Join(tempDir, path), data, 0666)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
 	tests := []struct {
-		name        string
-		files       map[string][]byte
-		input       *regexp.Regexp
+		input       string
 		expectFiles []*File
 	}{
 		{
-			name: "match master configs",
-			files: map[string][]byte{
-				"master-0.ign":   []byte("some data 0"),
-				"master-1.ign":   []byte("some data 1"),
-				"master-2.ign":   []byte("some data 2"),
-				"master-10.ign":  []byte("some data 3"),
-				"master-20.ign":  []byte("some data 4"),
-				"master-00.ign":  []byte("some data 5"),
-				"master-01.ign":  []byte("some data 6"),
-				"master-0x.ign":  []byte("some data 7"),
-				"master-1x.ign":  []byte("some data 8"),
-				"amaster-0.ign":  []byte("some data 9"),
-				"master-.ign":    []byte("some data 10"),
-				"master-.igni":   []byte("some data 11"),
-				"master-.ignign": []byte("some data 12"),
-			},
-			input: regexp.MustCompile(`^(master-(0|([1-9]\d*))\.ign)$`),
+			input: "master-[0-9]*.ign",
 			expectFiles: []*File{
 				{
 					Filename: "master-0.ign",
 					Data:     []byte("some data 0"),
 				},
+				{
+					Filename: "master-00.ign",
+					Data:     []byte("some data 5"),
+				},
+				{
+					Filename: "master-01.ign",
+					Data:     []byte("some data 6"),
+				},
 				{
 					Filename: "master-1.ign",
 					Data:     []byte("some data 1"),
@@ -111,37 +146,29 @@ func TestFetchByPattern(t *testing.T) {
 			},
 		},
 		{
-			name: "match directory",
-			files: map[string][]byte{
-				"manifests/":     []byte("some data 0"),
-				"manifests/0":    []byte("some data 1"),
-				"manifests/some": []byte("some data 2"),
-				"manifest/":      []byte("some data 3"),
-				"manifests":      []byte("some data 4"),
-				"amanifests/a":   []byte("some data 5"),
-			},
-			input: regexp.MustCompile(fmt.Sprintf(`^%s\%c.*`, "manifests", '/')),
+			input: filepath.Join("manifests", "*"),
 			expectFiles: []*File{
-				{
-					Filename: "manifests/",
-					Data:     []byte("some data 0"),
-				},
 				{
 					Filename: "manifests/0",
-					Data:     []byte("some data 1"),
+					Data:     []byte("some data 11"),
 				},
 				{
 					Filename: "manifests/some",
-					Data:     []byte("some data 2"),
+					Data:     []byte("some data 12"),
 				},
 			},
 		},
 	}
 
 	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			f := &fileFetcher{onDiskAssets: tt.files}
-			assert.Equal(t, tt.expectFiles, f.FetchByPattern(tt.input))
+		t.Run(tt.input, func(t *testing.T) {
+			f := &fileFetcher{directory: tempDir}
+			files, err := f.FetchByPattern(tt.input)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			assert.Equal(t, tt.expectFiles, files)
 		})
 	}
 }

pkg/asset/ignition/bootstrap/bootstrap.go

@@ -303,9 +303,12 @@ func applyTemplateData(template *template.Template, templateData interface{}) st
 
 // Load returns the bootstrap ignition from disk.
 func (a *Bootstrap) Load(f asset.FileFetcher) (found bool, err error) {
-	file := f.FetchByName(bootstrapIgnFilename)
-	if file == nil {
-		return false, nil
+	file, err := f.FetchByName(bootstrapIgnFilename)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, nil
+		}
+		return false, err
 	}
 
 	config := &igntypes.Config{}

pkg/asset/ignition/machine/master.go

@@ -3,7 +3,6 @@ package machine
 import (
 	"encoding/json"
 	"fmt"
-	"regexp"
 
 	igntypes "github.com/coreos/ignition/config/v2_2/types"
 	"github.com/pkg/errors"
@@ -67,7 +66,10 @@ func (a *Master) Files() []*asset.File {
 
 // Load returns the master ignitions from disk.
 func (a *Master) Load(f asset.FileFetcher) (found bool, err error) {
-	fileList := f.FetchByPattern(regexp.MustCompile(`^(master-(0|([1-9]\d*))\.ign)$`))
+	fileList, err := f.FetchByPattern("master-[0-9]*.ign")
+	if err != nil {
+		return false, nil
+	}
 	if len(fileList) == 0 {
 		return false, nil
 	}

pkg/asset/ignition/machine/worker.go

@@ -2,6 +2,7 @@ package machine
 
 import (
 	"encoding/json"
+	"os"
 
 	igntypes "github.com/coreos/ignition/config/v2_2/types"
 	"github.com/pkg/errors"
@@ -66,9 +67,12 @@ func (a *Worker) Files() []*asset.File {
 
 // Load returns the worker ignitions from disk.
 func (a *Worker) Load(f asset.FileFetcher) (found bool, err error) {
-	file := f.FetchByName(workerIgnFilename)
-	if file == nil {
-		return false, nil
+	file, err := f.FetchByName(workerIgnFilename)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, nil
+		}
+		return false, err
 	}
 
 	config := &igntypes.Config{}

pkg/asset/installconfig/installconfig.go

@@ -2,6 +2,7 @@ package installconfig
 
 import (
 	"net"
+	"os"
 
 	"github.com/apparentlymart/go-cidr/cidr"
 	"github.com/ghodss/yaml"
@@ -162,9 +163,12 @@ func parseCIDR(s string) net.IPNet {
 
 // Load returns the installconfig from disk.
 func (a *InstallConfig) Load(f asset.FileFetcher) (found bool, err error) {
-	file := f.FetchByName(installConfigFilename)
-	if file == nil {
-		return false, nil
+	file, err := f.FetchByName(installConfigFilename)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, nil
+		}
+		return false, err
 	}
 
 	config := &types.InstallConfig{}

pkg/asset/kubeconfig/kubeconfig.go

@@ -2,6 +2,7 @@ package kubeconfig
 
 import (
 	"fmt"
+	"os"
 
 	"github.com/ghodss/yaml"
 	"github.com/pkg/errors"
@@ -79,9 +80,12 @@ func (k *kubeconfig) Files() []*asset.File {
 
 // load returns the kubeconfig from disk.
 func (k *kubeconfig) load(f asset.FileFetcher, name string) (found bool, err error) {
-	file := f.FetchByName(name)
-	if file == nil {
-		return false, nil
+	file, err := f.FetchByName(name)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, nil
+		}
+		return false, err
 	}
 
 	config := &clientcmd.Config{}

pkg/asset/manifests/operators.go

@@ -6,7 +6,6 @@ import (
 	"encoding/base64"
 	"fmt"
 	"path/filepath"
-	"regexp"
 	"text/template"
 
 	"github.com/ghodss/yaml"
@@ -222,8 +221,10 @@ func applyTemplateData(template *template.Template, templateData interface{}) []
 
 // Load returns the manifests asset from disk.
 func (m *Manifests) Load(f asset.FileFetcher) (bool, error) {
-	re := fmt.Sprintf(`^%s\%c.*`, manifestDir, filepath.Separator) // e.g. `^manifests\/.*`
-	fileList := f.FetchByPattern(regexp.MustCompile(re))
+	fileList, err := f.FetchByPattern(filepath.Join(manifestDir, "*"))
+	if err != nil {
+		return false, err
+	}
 	if len(fileList) == 0 {
 		return false, nil
 	}

pkg/asset/manifests/tectonic.go

@@ -3,9 +3,7 @@ package manifests
 import (
 	"bytes"
 	"encoding/base64"
-	"fmt"
 	"path/filepath"
-	"regexp"
 
 	"github.com/ghodss/yaml"
 	"github.com/pkg/errors"
@@ -131,8 +129,10 @@ func (t *Tectonic) Files() []*asset.File {
 
 // Load returns the tectonic asset from disk.
 func (t *Tectonic) Load(f asset.FileFetcher) (bool, error) {
-	re := fmt.Sprintf(`^%s\%c.*`, tectonicManifestDir, filepath.Separator) // e.g. `^tectonic\/.*`
-	fileList := f.FetchByPattern(regexp.MustCompile(re))
+	fileList, err := f.FetchByPattern(filepath.Join(tectonicManifestDir, "*"))
+	if err != nil {
+		return false, err
+	}
 	if len(fileList) == 0 {
 		return false, nil
 	}

pkg/asset/store.go

@@ -46,13 +46,8 @@ type StoreImpl struct {
 
 // NewStore returns an asset store that implements the Store interface.
 func NewStore(dir string) (Store, error) {
-	ff, err := newFileFetcher(dir)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to create file fetcher from dir %q", dir)
-	}
-
 	store := &StoreImpl{
-		fileFetcher: ff,
+		fileFetcher: &fileFetcher{directory: dir},
 		assets:      make(map[reflect.Type]assetState),
 	}