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

yifan-gu · 2018-09-28T22:27:33Z

Fix https://jira.coreos.com/browse/CORS-847

Add Load() to Asset interface that returns files of the asset.
Make all assets implement the Load() function to read files from disk.

abhinavdahiya · 2018-09-28T23:02:22Z

pkg/asset/cluster/cluster.go

i think this can move out of this if ?

abhinavdahiya · 2018-09-28T23:10:32Z

pkg/asset/asset.go

can the load provide basic read/stat on files. So each asset doesn't have to remember to prefix dir

what do you mean? assets are not remembering dir, that's why the dir is passed in.

assets are not remembering dir, that's why the dir is passed in.

I think @abhinavdahiya's suggesting we handle the file open and read in centralized code, and then pass the data down into the individual assets. I agree that this is a good way to go. Basically, what we're doing here is reversing the looped PersistToFile. How about making that a Store.Save(dir) method, and adding a new Store.Load(dir) to reverse? The difficulty would be that the Store would need a way to discover filenames used by the asset. I'd rather make that public instead. Maybe with a Filenames() []string method here on Asset? Then Store.Load goes through the required assets, calls their Filenames(), reads those files, and pushes the read map[string][]bytes (filename -> content) into Asset.Load to get turned into a *State. In the other hand, once you've loaded the content from disk by filename, you've pretty much got a State instance already. Maybe all we need from Asset is Asset.Filenames()?

@wking Going to take care of the state of the whole store in the next PR. This one is only going to handle reading individual assets from the disk, the reason is that when run openshift-install <target> only the assets in the <target> are written to the disk, the state of other assets will be stored in a hidden state file.
That's why we also want individual asset to have a Load() function. The implementation of the Load() is centralized in the state.go, only the master-ignition and network-config are special because they will only look for some specific files under a dir.

cc @crawford @staebler this might have to change as generate might drop returning state?

abhinavdahiya · 2018-09-28T23:11:35Z

pkg/asset/ignition/machine/master.go

ioutil.ReadDir()

wking · 2018-09-29T21:38:12Z

e2e:

time="2018-09-29T00:05:22Z" level=fatal msg="failed to generate asset: unexpected error when loading asset \"Master Ignition Config(s)\": failed to read dir \"/tmp/artifacts/installer\": open /tmp/artifacts/installer: no such file or directory"

wking

How does this interact with #344? I haven't wrapped my head around that one well enough to know.

wking · 2018-09-29T23:58:40Z

pkg/asset/asset.go

assets are not remembering dir, that's why the dir is passed in.

I think @abhinavdahiya's suggesting we handle the file open and read in centralized code, and then pass the data down into the individual assets. I agree that this is a good way to go. Basically, what we're doing here is reversing the looped PersistToFile. How about making that a Store.Save(dir) method, and adding a new Store.Load(dir) to reverse? The difficulty would be that the Store would need a way to discover filenames used by the asset. I'd rather make that public instead. Maybe with a Filenames() []string method here on Asset? Then Store.Load goes through the required assets, calls their Filenames(), reads those files, and pushes the read map[string][]bytes (filename -> content) into Asset.Load to get turned into a *State. In the other hand, once you've loaded the content from disk by filename, you've pretty much got a State instance already. Maybe all we need from Asset is Asset.Filenames()?

yifan-gu · 2018-10-01T21:25:41Z

/test e2e-aws

yifan-gu · 2018-10-01T21:48:04Z

How does this interact with #344? I haven't wrapped my head around that one well enough to know.

@wking I looked at the #344, it seems the biggest change is that tls asets are split from two types (certkey, keypair) to a number of types to match all assets. So basically we will add Load() function to all those new asset types, and in the Load(), we will call the LoadFromFile()

yifan-gu · 2018-10-01T22:00:35Z

ok, let me try something different than Load(dir string)

yifan-gu · 2018-10-02T00:33:54Z

@abhinavdahiya @wking @rajatchopra Updated, ptal.

yifan-gu · 2018-10-02T19:06:52Z

/retest

abhinavdahiya · 2018-10-18T22:56:34Z

pkg/asset/asset.go

/s/will/should ?

pkg/asset/store.go

abhinavdahiya · 2018-10-18T23:08:27Z

pkg/asset/store.go

IsAssetInState

abhinavdahiya · 2018-10-18T23:12:16Z

pkg/asset/store.go

you can return dirty, <error>

abhinavdahiya · 2018-10-18T23:12:33Z

pkg/asset/store.go

you can return dirty, <error>

abhinavdahiya · 2018-10-18T23:12:40Z

pkg/asset/store.go

you can return dirty, <error>

abhinavdahiya · 2018-10-18T23:13:03Z

pkg/asset/store.go

you can return dirty, <error>

- Add Loadable interface to load assets from disk. - Load on-disk assets in Fetch() and use it to overwrite the state file. - Add FileFetcher interface to help reading the files from disk.

Implement Load() for all loadable assets to read files from disk and generate the in-memory asset.

abhinavdahiya · 2018-10-18T23:54:39Z

/lgtm

openshift-ci-robot · 2018-10-18T23:54:47Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,yifan-gu]

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

openshift-merge-robot · 2018-10-19T09:45:43Z

/retest

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

deads2k · 2018-10-19T12:28:39Z

I run with libvirt and after this pull merged I see

[deads@deads-02 installer]$ 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

resetting to bf0fa6d gives me a working binary

deads2k · 2018-10-19T12:33:33Z

@yifan-gu @abhinavdahiya fyi

@stevekuznetsov how do I make a revert of this multicommit pull locally?

sttts · 2018-10-23T13:03:30Z

After this PR /var/opt/tectonic/auth/kubeconfig on the bootstrap node stopped working against the local kube-apiserver. Moreover, I see plenty of the following messages in the kube-apiserver logs:

I1023 13:03:11.355477       5 logs.go:49] http: TLS handshake error from 127.0.0.1:36586: remote error: tls: bad certificate

sttts · 2018-10-23T14:06:16Z

~~Before this PR, apiserver.crt (the kube-apiserver serving cert) was signed by kube-ca. So the kube-ca cert was enough to validate apiserver.crt.~~

staebler · 2018-10-23T14:24:29Z

@sttts Isn't the second CA output the root CA, not the apiserver CA?

Subject: OU=openshift, CN=root-ca

sttts · 2018-10-23T14:25:20Z

Yep, copied the wrong piece. Recreating a cluster to get the right one.

Avoiding: $ openshift-install --dir=wking create install-config $ emacs wking/install-config.yaml $ openshift-install --dir=wking create manifests FATAL Error executing openshift-install: remove install-config.yml: no such file or directory Fixing a bug from 116dd64 (pkg/asset: Load on-disk files in Fetch(), 2018-10-18, openshift#374).

sttts · 2018-10-25T08:22:24Z

To close the discussion: the issue was #522, i.e. the installer picking up state from the working dir leading to a cert mismatch.

stevekuznetsov · 2018-11-02T21:03:57Z

@stevekuznetsov how do I make a revert of this multicommit pull locally?

@deads2k are you asking how to revert this merge?

Find the merge commit SHA and

git revert $MERGE_SHA -m 1

Is that what you need?

openshift-ci-robot requested review from abhinavdahiya and wking September 28, 2018 22:27

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 28, 2018

yifan-gu requested review from crawford and staebler September 28, 2018 22:27

abhinavdahiya reviewed Sep 28, 2018

View reviewed changes

pkg/asset/cluster/cluster.go Outdated

Copy link

Contributor

abhinavdahiya Sep 28, 2018

Choose a reason for hiding this comment

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

i think this can move out of this if ?

abhinavdahiya reviewed Sep 28, 2018

View reviewed changes

pkg/asset/ignition/machine/master.go Outdated

Copy link

Contributor

abhinavdahiya Sep 28, 2018

Choose a reason for hiding this comment

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

ioutil.ReadDir()

yifan-gu force-pushed the load_assets branch 2 times, most recently from 8fdca37 to 5d11dc3 Compare September 29, 2018 00:03

wking reviewed Sep 30, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2018

yifan-gu force-pushed the load_assets branch from 5d11dc3 to 7a37777 Compare October 1, 2018 18:49

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2018

yifan-gu force-pushed the load_assets branch from c5f084f to 41c3e8f Compare October 1, 2018 19:06

yifan-gu force-pushed the load_assets branch from 41c3e8f to f0cfbe2 Compare October 2, 2018 00:23

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2018

yifan-gu force-pushed the load_assets branch from f0cfbe2 to 412113a Compare October 2, 2018 00:24

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2018

yifan-gu force-pushed the load_assets branch from 412113a to 8b9a4a8 Compare October 2, 2018 00:30

yifan-gu force-pushed the load_assets branch 2 times, most recently from 220be45 to 1390176 Compare October 2, 2018 19:02

wking mentioned this pull request Oct 2, 2018

install-config.yml edits don't affect manifests/cluster-config.yaml #394

Closed