From edcd705e467b93a1f4bdedc98b3e1861df8eceaa Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 19 Oct 2018 09:53:52 -0700 Subject: [PATCH] pkg/asset/filefetcher: Only load files on demand Avoid: $ bin/openshift-install cluster FATAL Error executing openshift-install: open tests/smoke/vendor/github.com/prometheus/procfs/fixtures/26231/fd/0: no such file or directory as the old implementation attempts to walk the whole directory and hits: $ ls -l tests/smoke/vendor/github.com/prometheus/procfs/fixtures/26231/fd/ total 0 lrwxrwxrwx. 1 trking trking 24 Oct 5 01:26 0 -> ../../symlinktargets/abc lrwxrwxrwx. 1 trking trking 24 Oct 5 01:26 1 -> ../../symlinktargets/def lrwxrwxrwx. 1 trking trking 24 Oct 5 01:26 10 -> ../../symlinktargets/xyz lrwxrwxrwx. 1 trking trking 24 Oct 5 01:26 2 -> ../../symlinktargets/ghi lrwxrwxrwx. 1 trking trking 24 Oct 5 01:26 3 -> ../../symlinktargets/uvw With this commit, we only load files from the disk when someone asks for them. I've adjusted the unit tests a bit because: * ioutil.ReadFile returns errors like: read /: is a directory for directories. There does not appear to be an analog to os.IsNotExist() for this condition, so instead of checking for it in the tests, I've just dropped the empty-string input cases. If we break something and call FetchByName on an empty string, we want to error out, and that error message is appropriately descriptive already. * Globs are not as precise as regular expressions, so our glob would match master-1x.ign and similar which the previous regexp excluded. But loading a few extra files doesn't seem like that big a deal, and folks adding files with names like that seems unlikely. --- docs/design/assetgeneration.md | 4 +- pkg/asset/cluster/cluster.go | 11 +- pkg/asset/cluster/tfvars.go | 11 +- pkg/asset/filefetcher.go | 87 +++++--------- pkg/asset/filefetcher_test.go | 137 +++++++++++++--------- pkg/asset/ignition/bootstrap/bootstrap.go | 9 +- pkg/asset/ignition/machine/master.go | 6 +- pkg/asset/ignition/machine/worker.go | 10 +- pkg/asset/installconfig/installconfig.go | 10 +- pkg/asset/kubeconfig/kubeconfig.go | 10 +- pkg/asset/manifests/operators.go | 7 +- pkg/asset/manifests/tectonic.go | 8 +- pkg/asset/store.go | 7 +- 13 files changed, 170 insertions(+), 147 deletions(-) diff --git a/docs/design/assetgeneration.md b/docs/design/assetgeneration.md index 8718699101..098ad08569 100644 --- a/docs/design/assetgeneration.md +++ b/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) } ``` diff --git a/pkg/asset/cluster/cluster.go b/pkg/asset/cluster/cluster.go index 6b43b63150..661885d973 100644 --- a/pkg/asset/cluster/cluster.go +++ b/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) } diff --git a/pkg/asset/cluster/tfvars.go b/pkg/asset/cluster/tfvars.go index 08696af76c..de064f507b 100644 --- a/pkg/asset/cluster/tfvars.go +++ b/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 diff --git a/pkg/asset/filefetcher.go b/pkg/asset/filefetcher.go index 11fad17661..a8a76c2a58 100644 --- a/pkg/asset/filefetcher.go +++ b/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 } diff --git a/pkg/asset/filefetcher_test.go b/pkg/asset/filefetcher_test.go index 9548527ca5..43cb52b6c2 100644 --- a/pkg/asset/filefetcher_test.go +++ b/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) }) } } diff --git a/pkg/asset/ignition/bootstrap/bootstrap.go b/pkg/asset/ignition/bootstrap/bootstrap.go index 2d7644bf20..62ca4ef14e 100644 --- a/pkg/asset/ignition/bootstrap/bootstrap.go +++ b/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{} diff --git a/pkg/asset/ignition/machine/master.go b/pkg/asset/ignition/machine/master.go index 7cde423c04..13afe0d4ad 100644 --- a/pkg/asset/ignition/machine/master.go +++ b/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 } diff --git a/pkg/asset/ignition/machine/worker.go b/pkg/asset/ignition/machine/worker.go index 201b2e86dd..32a5ee07a9 100644 --- a/pkg/asset/ignition/machine/worker.go +++ b/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{} diff --git a/pkg/asset/installconfig/installconfig.go b/pkg/asset/installconfig/installconfig.go index 46d70cb020..0c022b2d53 100644 --- a/pkg/asset/installconfig/installconfig.go +++ b/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{} diff --git a/pkg/asset/kubeconfig/kubeconfig.go b/pkg/asset/kubeconfig/kubeconfig.go index d17ff37b51..5ac43530dd 100644 --- a/pkg/asset/kubeconfig/kubeconfig.go +++ b/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{} diff --git a/pkg/asset/manifests/operators.go b/pkg/asset/manifests/operators.go index 4be12421c6..eb9b8cc920 100644 --- a/pkg/asset/manifests/operators.go +++ b/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 } diff --git a/pkg/asset/manifests/tectonic.go b/pkg/asset/manifests/tectonic.go index 1a4791911b..66390b8b5c 100644 --- a/pkg/asset/manifests/tectonic.go +++ b/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" @@ -127,8 +125,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 } diff --git a/pkg/asset/store.go b/pkg/asset/store.go index d029465b07..983e2d78db 100644 --- a/pkg/asset/store.go +++ b/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), }