Save assets in a state file on disk #388

rajatchopra · 2018-10-01T22:26:49Z

pkg/asset:

Implement new funtions Save/Load for saving and loading the asset states

cmd/openshift-install:

Call out on the Save function to persist the state file of all assets in a constant state file

crawford · 2018-10-01T22:35:02Z

pkg/asset/store.go

Let's use JSON instead. Go's YAML implementation has a lot of trouble serializing and then deserializing accurately and YAML is a little too inviting for end users.

rajatchopra · 2018-10-02T18:37:27Z

Have to redo the PR. Closing this one out.

crawford · 2018-10-02T20:43:04Z

Please don't close out these pull requests. There is valuable discussion (maybe not so much in this case) that gets lost. Unless you are fundamentally changing the approach, I would reuse existing pull requests.

yifan-gu · 2018-10-03T19:13:40Z

@rajatchopra Any updates on this one?

rajatchopra · 2018-10-04T22:54:11Z

@yifan-gu @staebler PTAL
This PR uses Name() for the keys, and I can successfully marshal/unmarshal the asset map. Do we actually ever need to restore the asset objects? I am guessing no. Good riddance.

Try this PR with a fresh install. Then repeat without deleting the target directory. No more asset re-generation.

abhinavdahiya · 2018-10-05T17:11:14Z

cmd/openshift-install/main.go

This leaves the state file save if we error out in generating any assets above.

abhinavdahiya · 2018-10-05T17:11:15Z

cmd/openshift-install/main.go

Fatal already os.Exit(1)

abhinavdahiya · 2018-10-05T17:11:25Z

pkg/asset/stock/stock.go

rebase relic :). Some similar code might be if we ever need to be able to construct the asset object from its name. Removed.

abhinavdahiya · 2018-10-05T17:13:01Z

pkg/asset/store.go

can we not camel case. maybe this: .openshift_install_state.json

.openshift_install_state.json now.

abhinavdahiya · 2018-10-05T17:13:17Z

pkg/asset/store.go

drop this extra line.

abhinavdahiya · 2018-10-05T17:14:06Z

pkg/asset/userprovided.go

this change seems orthogonal.

Only part of it. The map in the argument does need to change. Fixed.

abhinavdahiya · 2018-10-05T17:19:09Z

pkg/asset/store.go

https://golang.org/pkg/os/#IsNotExist

abhinavdahiya · 2018-10-11T22:41:17Z

pkg/asset/store.go

This should either be dropped or be something like looking up asset from state file

abhinavdahiya · 2018-10-11T22:42:15Z

cmd/openshift-install/targets.go

also you can return error here; why Fatal?

1. Calls out on the Save function to persist the state file of all assets in a constant state file 2. Calls out on the Load function to partially load the contents of the state file into a new field in StoreImpl 3. In Save, marshaling as done as a map of type.String() to asset bytes: i.e. stateMap[reflect.TypeOf(assetObject).String()] = marshalledBytes(assetObject) reflect.TypeOf().String() is used as against reflect.Type.Name() function because Name returns the type name only, without scoping the package path. See the implementation of type.Name() function where type.String() is used within: https://golang.org/src/reflect/type.go?#L874 4. Support for 'deferred unmarshal' for assets from state file: Before a target is worked upon, the state file is loaded into the memory as partial asset state map. The key of the map is the string representation of the asset type and the value is raw bytes that are left as is. The idea is that 'fetch' will finally get the asset from the state file, only when needed. A utility function GetStateAsset has been provided to allow for deferred unmarshaling. See example code to use the util function (as in the store's fetch function): ``` func (s *StoreImpl) fetch(asset Asset, indent string) error { ... ... ok, err := s.GetStateAsset(asset) if err != nil { return errors.Wrapf("failed to unmarshal asset from state file: %v. Remove the state file and continue..", err) } if ok { logrus.Debugf("%sAsset found in state file %v", indent, asset) if s.assets == nil { s.assets = make(map[reflect.Type]Asset) } s.assets[reflect.TypeOf(asset)] = asset return nil } ... ... ``` Alternatively, instead of passing the empty asset object, one can make a copy of the asset object and render it with contents from the state file: ``` newAsset := reflect.ValueOf(reflect.New(reflect.TypeOf(asset))).Elem().Interface() ok, err := s.GetStateAsset(newAsset) // now compare newAsset with asset itself ... // and set the contents of asset from newAsset if needed: reflect.ValueOf(asset).Elem().Set(reflect.ValueOf(newAsset).Elem()) ``` Other notes: The utility function GetStateAsset used in this commit such that if an asset is found in the state file, then its used directly. Further work will need to modify this behaviour so that a three way merge can happen between an asset found in the state file, found on disk, rendered by the Generate function.

Expose the fields of all Asset/WritableAsset objects so that we can Marshal/Unmarshal them from the state file. Some of the fields clash with a function name implemented for the struct, so we have to change the field names themselves e.g. files cannot just be Files because there is a Files() function expected by the Asset interface. Not all fields need be exported actually, because some fields can be constructed from the 'Files' field anyway. This optimization has not been done in this commit, it would perhaps need a custom UnmarshalJSON function to do that. With this commit, all assets can be saved and loaded back properly. So, an example run of the openshift-install binary will look like this: $ openshift-install manifests --dir=test --log-level=debug $ # the above command generates the folders test/{manifests,tectonic} with all the files within $ # it also genrates the state file in test/.openshift_install_state.json $ # let's remove the manifest files and generate them again.. $ rm -rf test/{manifests,tectonic} $ openshift-install manifests --dir=test --log-level=debug $ # check that everything has been restored without actually generating anything $ # run the install-config command even to create the install-config.yaml from state file: $ openshift-install install-config --dir=test --log-level=debug

abhinavdahiya · 2018-10-12T01:02:55Z

/approve

@staebler @crawford can you take aquick look

crawford

/lgtm

openshift-ci-robot · 2018-10-12T01:53:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford, rajatchopra

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya,crawford,rajatchopra]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2018-10-12T07:49:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-10-12T09:50:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-10-12T11:51:20Z

/retest

Please review the full test history for this PR and help us cut down flakes.

This saves us a few characters and gives us better handling when the string values themselves contain quotes (although only the former matters much in this case). The old manual quoting is from 971eea9 (pkg/asset: Save/Load functinality for assets into a state file, 2018-10-11, openshift#388).

Through f25e53a (Merge pull request openshift#388 from rajatchopra/state_file, 2018-10-12).

wking · 2018-10-16T05:36:15Z

Do we need to poke a hole in this for the not-really-an-asset cluster target? As it stands:

$ openshift-install version
openshift-install v0.2.0-14-g7cc49c8a92a86dd13ac66d1fd0560f679a36d1c0
Terraform v0.11.8
$ openshift-install --log-level=debug cluster
...
DEBUG Looking up asset from state file: *cluster.Cluster 
DEBUG Asset found in state file

So I have to manually blow away .openshift_install_state.json in order to launch a new cluster.

staebler · 2018-10-16T14:22:45Z

Do we need to poke a hole in this for the not-really-an-asset cluster target? As it stands:

@wking A few things come to mind for me about this.

Should the assets being targeted always be regenerated when running openshift-install. For example, if the user runs openshift-install manifests, then the manifests should always be regenerated regardless of whether they exist in the state file or on disk already. The user directed the asset to be generated, so it should be.
Should there be a "clear" sub-command? The normal user shouldn't need to know or even be concerned about the state file. But the user may need a way to start over.
If the cluster is successfully installed, then the installation process is done. At that point, the state file should be deleted by the installer.

wking · 2018-10-16T18:48:24Z

The user directed the asset to be generated, so it should be.

Or maybe they just directed that the asset be written to disk (which it is with the current master). The thing that's missing is that assets which have important side effects beside being written to disk (e.g. launching the cluster ;) aren't directly addressed by "generate this asset" semantics.

If the cluster is successfully installed, then the installation process is done. At that point, the state file should be deleted by the installer.

Maybe? It's not actually clear to me at what point we'd want to remove the state file. Once cluster completes, we'd no longer need the state file for launching clusters, but we might need the Terraform state for removing the bootstrap assets, and we'll need metadata.json for destroy-cluster. It feels a bit odd to remove the state file while leaving those.

On the other hand, if you remove the cluster through some other path (e.g. virsh-cleanup.sh), then you do want to blow away the cluster state (or at least re-run the cluster asset) on the next cluster call. So I think the issue is just with the special-side-effects cluster "asset", and that we don't need a generic fix. But I'm not clear on what the cluster-specific fix should be ;).

rajatchopra · 2018-10-16T19:13:14Z

@wking @staebler Created PR #476 as a proposed fix. Builds on Matthew's idea that assets that are targeted should always be rendered. Comment over in that PR, if it looks like the right direction.

crawford · 2018-10-16T19:27:38Z

We should instead check the asset store to see if the assets in the requested target have already been realized. If they have, we should do something different (e.g. tell the user that this operation is a no-op) rather than try to solve this down in the asset graph.

openshift-ci-robot requested review from abhinavdahiya and staebler October 1, 2018 22:26

crawford reviewed Oct 1, 2018

View reviewed changes

rajatchopra force-pushed the state_file branch from d3c6003 to 52bbb09 Compare October 2, 2018 18:33

rajatchopra closed this Oct 2, 2018

rajatchopra reopened this Oct 2, 2018

rajatchopra force-pushed the state_file branch from 52bbb09 to 98eb6b5 Compare October 3, 2018 22:42

yifan-gu mentioned this pull request Oct 4, 2018

pkg/asset: Introduce Load() into the Asset interface that loads assets (from disk) #374

Merged

abhinavdahiya mentioned this pull request Oct 4, 2018

Use string as the asset map key instead of the object itself #416

Closed

rajatchopra force-pushed the state_file branch from 98eb6b5 to 993b454 Compare October 4, 2018 22:41

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 4, 2018

rajatchopra changed the title ~~[WIP] Save assets in a state file on disk~~ Save assets in a state file on disk Oct 4, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 4, 2018

rajatchopra force-pushed the state_file branch 2 times, most recently from cb9a5ef to 240df82 Compare October 5, 2018 17:01

abhinavdahiya reviewed Oct 5, 2018

View reviewed changes

cmd/openshift-install/main.go Outdated

Copy link

Contributor

abhinavdahiya Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leaves the state file save if we error out in generating any assets above.

abhinavdahiya reviewed Oct 5, 2018

View reviewed changes

cmd/openshift-install/main.go Outdated

Copy link

Contributor

abhinavdahiya Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fatal already os.Exit(1)

abhinavdahiya reviewed Oct 5, 2018

View reviewed changes

abhinavdahiya reviewed Oct 11, 2018

View reviewed changes

pkg/asset/store.go Outdated

Copy link

Contributor

abhinavdahiya Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should either be dropped or be something like looking up asset from state file

abhinavdahiya reviewed Oct 11, 2018

View reviewed changes

cmd/openshift-install/targets.go Outdated

Copy link

Contributor

abhinavdahiya Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also you can return error here; why Fatal?

Rajat Chopra added 2 commits October 11, 2018 20:04

rajatchopra force-pushed the state_file branch from 9315071 to 10f4ee5 Compare October 12, 2018 00:05

crawford approved these changes Oct 12, 2018

View reviewed changes

openshift-ci-robot assigned crawford Oct 12, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2018

openshift-merge-robot merged commit f25e53a into openshift:master Oct 12, 2018

wking mentioned this pull request Oct 12, 2018

pkg/asset/store: Use '%q' for formatting quoted strings #460

Merged

wking mentioned this pull request Oct 12, 2018

CHANGELOG: Document changes since v0.1.0 #461

Merged

wking added a commit to wking/openshift-installer that referenced this pull request Oct 12, 2018

CHANGELOG: Document changes since v0.1.0

ec34840

Through f25e53a (Merge pull request openshift#388 from rajatchopra/state_file, 2018-10-12).

rajatchopra mentioned this pull request Oct 16, 2018

Create a new type of Asset called 'TargetableAsset' #476

Closed

rajatchopra deleted the state_file branch November 12, 2018 18:52

Save assets in a state file on disk #388

Save assets in a state file on disk #388

Uh oh!

Conversation

rajatchopra commented Oct 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatchopra commented Oct 2, 2018

Uh oh!

crawford commented Oct 2, 2018

Uh oh!

yifan-gu commented Oct 3, 2018

Uh oh!

rajatchopra commented Oct 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Oct 12, 2018

Uh oh!

crawford left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Oct 12, 2018

Uh oh!

openshift-bot commented Oct 12, 2018

Uh oh!

openshift-bot commented Oct 12, 2018

Uh oh!

openshift-bot commented Oct 12, 2018

Uh oh!

wking commented Oct 16, 2018

Uh oh!

staebler commented Oct 16, 2018

Uh oh!

wking commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajatchopra commented Oct 16, 2018

Uh oh!

crawford commented Oct 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

wking commented Oct 16, 2018 •

edited

Loading