From 30b2ca543f4816693238ee0aec83fe9e62ca5f51 Mon Sep 17 00:00:00 2001 From: Abhinav Dahiya Date: Fri, 2 Nov 2018 15:20:15 -0700 Subject: [PATCH 1/2] asset: trim Store interface and add Destroy to Store Save and Purge functions don't really seem to belong to Store interface. Fetch fetches the asset, checkpoints the state file to disk and consumes all the assets that were loaded from disk to create the asset. Also fixes the error in save() where it would only save the assets in memory and drops all the other assets present in the state file but were not fetched into the store's asset map eg: ```console ./openshift-install ign configs # state has everything ./openshift-install install-config # state now only has assets uptill install-config, all the ign config assets are lost from statefile ./openshift-install ign configs # state has everything ./openshift-install install-config # state still includes all the state from 'ign-config' run. ``` Destroy deletes the asset from all the possible sources, state file and disk. --- pkg/asset/asset.go | 40 +++++++++++++++++ pkg/asset/store.go | 107 +++++++++++++++++++++++++++++---------------- 2 files changed, 110 insertions(+), 37 deletions(-) diff --git a/pkg/asset/asset.go b/pkg/asset/asset.go index c3fb50f4dc1..b08652b0d2a 100644 --- a/pkg/asset/asset.go +++ b/pkg/asset/asset.go @@ -1,11 +1,13 @@ package asset import ( + "io" "io/ioutil" "os" "path/filepath" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) // Asset used to install OpenShift. @@ -55,3 +57,41 @@ func PersistToFile(asset WritableAsset, directory string) error { } return nil } + +// deleteAssetFromDisk removes all the files for asset from disk. +// this is function is not safe for calling concurrently on the same directory. +func deleteAssetFromDisk(asset WritableAsset, directory string) error { + logrus.Debugf("Purging asset %q from disk", asset.Name()) + for _, f := range asset.Files() { + path := filepath.Join(directory, f.Filename) + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return errors.Wrap(err, "failed to remove file") + } + + dir := filepath.Dir(path) + ok, err := isDirEmpty(dir) + if err != nil && !os.IsNotExist(err) { + return errors.Wrap(err, "failed to read directory") + } + if ok { + if err := os.Remove(dir); err != nil { + return errors.Wrap(err, "failed to remove directory") + } + } + } + return nil +} + +func isDirEmpty(name string) (bool, error) { + f, err := os.Open(name) + if err != nil { + return false, err + } + defer f.Close() + + _, err = f.Readdirnames(1) // Or f.Readdir(1) + if err == io.EOF { + return true, nil + } + return false, err // Either not empty or error, suits both cases +} diff --git a/pkg/asset/store.go b/pkg/asset/store.go index 59369caba15..63e59304b0d 100644 --- a/pkg/asset/store.go +++ b/pkg/asset/store.go @@ -21,12 +21,9 @@ type Store interface { // dependencies if necessary. Fetch(Asset) error - // Save dumps the entire state map into a file - Save(dir string) error - - // Purge deletes the on-disk assets that are consumed already. - // E.g., install-config.yml will be deleted after fetching 'manifests'. - Purge(excluded []WritableAsset) error + // Destroy removes the asset from all its internal state and also from + // disk if possible. + Destroy(Asset) error } // assetState includes an asset and a boolean that indicates @@ -42,7 +39,8 @@ type StoreImpl struct { assets map[reflect.Type]assetState stateFileAssets map[string]json.RawMessage fileFetcher *fileFetcher - onDiskAssets []WritableAsset // This records the on-disk assets that are loaded already, which will be cleaned up in the end. + + markedForPurge []WritableAsset // This records the on-disk assets that are loaded already, which will be cleaned up in the end. } // NewStore returns an asset store that implements the Store interface. @@ -53,25 +51,57 @@ func NewStore(dir string) (Store, error) { assets: make(map[reflect.Type]assetState), } - if err := store.load(dir); err != nil { + if err := store.load(); err != nil { return nil, err } - return store, nil } // Fetch retrieves the state of the given asset, generating it and its // dependencies if necessary. func (s *StoreImpl) Fetch(asset Asset) error { - _, err := s.fetch(asset, "") - return err + if _, err := s.fetch(asset, ""); err != nil { + return err + } + if err := s.save(); err != nil { + return errors.Wrapf(err, "failed to save state") + } + if wa, ok := asset.(WritableAsset); ok { + return errors.Wrapf(s.purge([]WritableAsset{wa}), "failed to purge asset") + } + return nil +} + +// Destroy removes the asset from all its internal state and also from +// disk if possible. +func (s *StoreImpl) Destroy(asset Asset) error { + if sa, ok := s.assets[reflect.TypeOf(asset)]; ok { + reflect.ValueOf(asset).Elem().Set(reflect.ValueOf(sa.asset).Elem()) + } else if s.isAssetInState(asset) { + if err := s.loadAssetFromState(asset); err != nil { + return err + } + } else { + // nothing to do + return nil + } + + if wa, ok := asset.(WritableAsset); ok { + if err := deleteAssetFromDisk(wa, s.directory); err != nil { + return err + } + } + + delete(s.assets, reflect.TypeOf(asset)) + delete(s.stateFileAssets, reflect.TypeOf(asset).String()) + return s.save() } // load retrieves the state from the state file present in the given directory // and returns the assets map -func (s *StoreImpl) load(dir string) error { - path := filepath.Join(dir, stateFileName) - assets := make(map[string]json.RawMessage) +func (s *StoreImpl) load() error { + path := filepath.Join(s.directory, stateFileName) + assets := map[string]json.RawMessage{} data, err := ioutil.ReadFile(path) if err != nil { if os.IsNotExist(err) { @@ -87,8 +117,8 @@ func (s *StoreImpl) load(dir string) error { return nil } -// LoadAssetFromState renders the asset object arguments from the state file contents. -func (s *StoreImpl) LoadAssetFromState(asset Asset) error { +// loadAssetFromState renders the asset object arguments from the state file contents. +func (s *StoreImpl) loadAssetFromState(asset Asset) error { bytes, ok := s.stateFileAssets[reflect.TypeOf(asset).String()] if !ok { return errors.Errorf("asset %q is not found in the state file", asset.Name()) @@ -96,24 +126,30 @@ func (s *StoreImpl) LoadAssetFromState(asset Asset) error { return json.Unmarshal(bytes, asset) } -// IsAssetInState tests whether the asset is in the state file. -func (s *StoreImpl) IsAssetInState(asset Asset) bool { +// isAssetInState tests whether the asset is in the state file. +func (s *StoreImpl) isAssetInState(asset Asset) bool { _, ok := s.stateFileAssets[reflect.TypeOf(asset).String()] return ok } -// Save dumps the entire state map into a file -func (s *StoreImpl) Save(dir string) error { - assetMap := make(map[string]Asset) +// save dumps the entire state map into a file +func (s *StoreImpl) save() error { + if s.stateFileAssets == nil { + s.stateFileAssets = map[string]json.RawMessage{} + } for k, v := range s.assets { - assetMap[k.String()] = v.asset + data, err := json.MarshalIndent(v.asset, "", " ") + if err != nil { + return err + } + s.stateFileAssets[k.String()] = json.RawMessage(data) } - data, err := json.MarshalIndent(&assetMap, "", " ") + data, err := json.MarshalIndent(s.stateFileAssets, "", " ") if err != nil { return err } - path := filepath.Join(dir, stateFileName) + path := filepath.Join(s.directory, stateFileName) if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { return err } @@ -159,7 +195,7 @@ func (s *StoreImpl) fetch(asset Asset, indent string) (bool, error) { } // Try to find the asset from the state file. - foundInStateFile := s.IsAssetInState(asset) + foundInStateFile := s.isAssetInState(asset) if foundInStateFile { logrus.Debugf("%sFound %q in state file", indent, asset.Name()) } @@ -174,7 +210,7 @@ func (s *StoreImpl) fetch(asset Asset, indent string) (bool, error) { } if foundOnDisk { logrus.Infof("Consuming %q from target directory", asset.Name()) - s.onDiskAssets = append(s.onDiskAssets, as) + s.markedForPurge = append(s.markedForPurge, as) } } @@ -191,7 +227,7 @@ func (s *StoreImpl) fetch(asset Asset, indent string) (bool, error) { logrus.Debugf("%sLoading %q from both state file and target directory", indent, asset.Name()) stateAsset := reflect.New(reflect.TypeOf(asset).Elem()).Interface().(Asset) - if err := s.LoadAssetFromState(stateAsset); err != nil { + if err := s.loadAssetFromState(stateAsset); err != nil { return false, errors.Wrapf(err, "failed to load asset %q from state file", asset.Name()) } @@ -202,7 +238,7 @@ func (s *StoreImpl) fetch(asset Asset, indent string) (bool, error) { } } else if foundInStateFile { logrus.Debugf("%sLoading %q from state file", indent, asset.Name()) - if err := s.LoadAssetFromState(asset); err != nil { + if err := s.loadAssetFromState(asset); err != nil { return false, errors.Wrapf(err, "failed to load asset %q from state file", asset.Name()) } } else if foundOnDisk { @@ -214,13 +250,12 @@ func (s *StoreImpl) fetch(asset Asset, indent string) (bool, error) { return dirty, nil } -// Purge deletes the on-disk assets that are consumed already. +// purge deletes the on-disk assets that are consumed already. // E.g., install-config.yml will be deleted after fetching 'manifests'. // The target assets are excluded. -func (s *StoreImpl) Purge(excluded []WritableAsset) error { +func (s *StoreImpl) purge(excluded []WritableAsset) error { var toPurge []WritableAsset - - for _, asset := range s.onDiskAssets { + for _, asset := range s.markedForPurge { var found bool for _, as := range excluded { if reflect.TypeOf(as) == reflect.TypeOf(asset) { @@ -234,12 +269,10 @@ func (s *StoreImpl) Purge(excluded []WritableAsset) error { } for _, asset := range toPurge { - logrus.Debugf("Purging asset %q", asset.Name()) - for _, f := range asset.Files() { - if err := os.Remove(filepath.Join(s.directory, f.Filename)); err != nil { - return errors.Wrapf(err, "failed to remove file %q", f.Filename) - } + if err := deleteAssetFromDisk(asset, s.directory); err != nil { + return err } } + s.markedForPurge = []WritableAsset{} return nil } From b6865885001580c7e8bcc983cff46146c715540a Mon Sep 17 00:00:00 2001 From: Abhinav Dahiya Date: Fri, 2 Nov 2018 15:21:27 -0700 Subject: [PATCH 2/2] cmd/destroy: delete cluster asset after destroying cluster regarding changes to cmd/create.go, each target is a variable to preserve the order when creating subcommands and still allow other functions to directly access each target individually. If could have stored them in maps, but maps returns in random order causing the cli options to look different on each run --- cmd/openshift-install/create.go | 94 +++++++++++++++++--------------- cmd/openshift-install/destroy.go | 11 ++++ 2 files changed, 62 insertions(+), 43 deletions(-) diff --git a/cmd/openshift-install/create.go b/cmd/openshift-install/create.go index 3b97d0a475e..b5db6fce8c3 100644 --- a/cmd/openshift-install/create.go +++ b/cmd/openshift-install/create.go @@ -34,41 +34,58 @@ type target struct { assets []asset.WritableAsset } -var targets = []target{{ - name: "Install Config", - command: &cobra.Command{ - Use: "install-config", - Short: "Generates the Install Config asset", - Long: "", - }, - assets: []asset.WritableAsset{&installconfig.InstallConfig{}}, -}, { - name: "Manifests", - command: &cobra.Command{ - Use: "manifests", - Short: "Generates the Kubernetes manifests", - Long: "", - }, - assets: []asset.WritableAsset{&manifests.Manifests{}, &manifests.Tectonic{}}, -}, { - name: "Ignition Configs", - command: &cobra.Command{ - Use: "ignition-configs", - Short: "Generates the Ignition Config asset", - Long: "", - }, - assets: []asset.WritableAsset{&bootstrap.Bootstrap{}, &machine.Master{}, &machine.Worker{}}, -}, { - name: "Cluster", - command: &cobra.Command{ - Use: "cluster", - Short: "Create an OpenShift cluster", - PostRunE: func(_ *cobra.Command, _ []string) error { - return destroyBootstrap(context.Background(), rootOpts.dir) +// each target is a variable to preserve the order when creating subcommands and still +// allow other functions to directly access each target individually. +var ( + installConfigTarget = target{ + name: "Install Config", + command: &cobra.Command{ + Use: "install-config", + Short: "Generates the Install Config asset", + // FIXME: add longer descriptions for our commands with examples for better UX. + // Long: "", }, - }, - assets: []asset.WritableAsset{&cluster.TerraformVariables{}, &kubeconfig.Admin{}, &cluster.Cluster{}}, -}} + assets: []asset.WritableAsset{&installconfig.InstallConfig{}}, + } + + manifestsTarget = target{ + name: "Manifests", + command: &cobra.Command{ + Use: "manifests", + Short: "Generates the Kubernetes manifests", + // FIXME: add longer descriptions for our commands with examples for better UX. + // Long: "", + }, + assets: []asset.WritableAsset{&manifests.Manifests{}, &manifests.Tectonic{}}, + } + + ignitionConfigsTarget = target{ + name: "Ignition Configs", + command: &cobra.Command{ + Use: "ignition-configs", + Short: "Generates the Ignition Config asset", + // FIXME: add longer descriptions for our commands with examples for better UX. + // Long: "", + }, + assets: []asset.WritableAsset{&bootstrap.Bootstrap{}, &machine.Master{}, &machine.Worker{}}, + } + + clusterTarget = target{ + name: "Cluster", + command: &cobra.Command{ + Use: "cluster", + Short: "Create an OpenShift cluster", + // FIXME: add longer descriptions for our commands with examples for better UX. + // Long: "", + PostRunE: func(_ *cobra.Command, _ []string) error { + return destroyBootstrap(context.Background(), rootOpts.dir) + }, + }, + assets: []asset.WritableAsset{&cluster.TerraformVariables{}, &kubeconfig.Admin{}, &cluster.Cluster{}}, + } + + targets = []target{installConfigTarget, manifestsTarget, ignitionConfigsTarget, clusterTarget} +) // Deprecated: Use 'create' subcommands instead. func newTargetsCmd() []*cobra.Command { @@ -128,15 +145,6 @@ func runTargetCmd(targets ...asset.WritableAsset) func(cmd *cobra.Command, args return err } } - - if err := assetStore.Save(rootOpts.dir); err != nil { - return errors.Wrapf(err, "failed to write to state file") - } - - if err := assetStore.Purge(targets); err != nil { - return errors.Wrapf(err, "failed to delete existing on-disk files") - } - return nil } } diff --git a/cmd/openshift-install/destroy.go b/cmd/openshift-install/destroy.go index f77306b1b04..b2c07cefcd8 100644 --- a/cmd/openshift-install/destroy.go +++ b/cmd/openshift-install/destroy.go @@ -5,6 +5,7 @@ import ( "github.com/sirupsen/logrus" "github.com/spf13/cobra" + "github.com/openshift/installer/pkg/asset" "github.com/openshift/installer/pkg/destroy" "github.com/openshift/installer/pkg/destroy/bootstrap" _ "github.com/openshift/installer/pkg/destroy/libvirt" @@ -49,6 +50,16 @@ func runDestroyCmd(cmd *cobra.Command, args []string) error { return errors.Wrap(err, "Failed to destroy cluster") } + + store, err := asset.NewStore(rootOpts.dir) + if err != nil { + return errors.Wrapf(err, "failed to create asset store") + } + for _, asset := range clusterTarget.assets { + if err := store.Destroy(asset); err != nil { + return errors.Wrapf(err, "failed to destroy asset %q", asset.Name()) + } + } return nil }