assets: add tests for validating asset fetching of targets #890

staebler · 2018-12-13T00:35:27Z

This adds tests around creating the targeted assets. The tests validate the following.

An asset that is generated and then fetched again from a new store has equivalent state.
A targeted asset restored from the state file and from on-disk has equivalent state.

This PR also includes a few changes to the assets to get the tests passing.

Fixes https://jira.coreos.com/browse/CORS-940

staebler · 2018-12-13T00:48:42Z

This PR include the changes in #877 as well.

abhinavdahiya · 2019-01-16T19:48:01Z

pkg/asset/store/data

@@ -0,0 +1 @@
+../../../data/data


what is this file?

This is a link to that directory. TestCreatedAssetsAreNotDirty needs it to be able to generate the manifest templates.

wking · 2019-01-30T05:40:06Z

pkg/asset/manifests/dns.go

@@ -91,31 +89,5 @@ func (d *DNS) Files() []*asset.File {

 // Load loads the already-rendered files back from disk.
 func (d *DNS) Load(f asset.FileFetcher) (bool, error) {


Why gut this method instead of removing it completely? Can't we get away with just Files() for use in (*Manifests).Generate(...) and turn *DNS from a WritableAsset into an Asset? Maybe we want to split interfaces:

// WritableAsset is an Asset that has files that can be written to disk. type WritableAsset interface { Asset // Files returns the files to write. Files() []*File } // ReadWriteableAsset is a WriteableAsset that can be loaded from disk. type ReadWritableAsset interface { WriteableAsset // Load returns the on-disk asset if it exists. // The asset object should be changed only when it's loaded successfully. Load(FileFetcher) (found bool, err error) }

I had those separate interfaces when I wrote the asset store originally. I'll have to dig back into past PRs to refresh my memory about who had what objections to that approach.

I would like to pass on making any change along these lines for this PR. I agree with you that it is ugly to have asset with Load functions that do nothing. However, the problem is more widespread than just these manifest assets. I think that changes that we make to remove unused Load functions are worthy of their own PR.

pkg/asset/targets/targets.go

wking · 2019-01-30T05:50:01Z

meta: Can we pull out the SVG rebuilds to a single commit at the end of the series? That would make it easier to read the commit history for human-generated changes.

wking · 2019-01-30T05:57:10Z

I've left a few minor nits inline, but I'm also fine landing this as-is and addressing those in follow-up work. I skimmed over the tests and a7ebfaa very quickly, but while it would be nice to go over those more carefully, I'm also fine with "the tests are green, if anything slipped through we can fix it up later" ;). Everything the commit messages said made sense to me.

staebler · 2019-01-30T16:24:58Z

meta: Can we pull out the SVG rebuilds to a single commit at the end of the series? That would make it easier to read the commit history for human-generated changes.

Done.

staebler · 2019-01-30T16:25:10Z

This looks like it could be squashed down into 5dc9d18.

Done.

wking · 2019-01-30T21:00:45Z

/lgtm
/retest

wking · 2019-01-31T03:46:59Z

e2e-aws:

level=info msg="Waiting up to 10m0s for the openshift-console route to be created..."
level=fatal msg="waiting for openshift-console URL: context deadline exceeded"

/tetest

wking · 2019-01-31T04:44:36Z

/tetest

Sigh.

/retest

The Common Templates asset is not truly an asset. It does not own any on-disk files, and no other assets depend upon it. The problem with the Common Templates asset is that it writes files to disk as a targeted asset but then does not load any assets back in. So a test that verifies that the loaded-in asset matches the written asset fails. Changes for https://jira.coreos.com/browse/CORS-940

Tests are being created that use the asset store implementation to validate restoring assets from the state file and disk. The tests are much easier when they have access to the private fields of the asset store. Rather than exposing more public fields and methods on the store, the tests will be placed in the same package as the store. Unfortunately, this creates cyclic imports between the base asset package and the various packages containing the targeted assets. To rectify this, the concrete asset store implementation has been moved to a new pkg/asset/store package. Changes for https://jira.coreos.com/browse/CORS-940

This adds tests around creating the targeted assets. The tests validate the following. 1) An asset that is generated and then fetched again from a new store has equivalent state. 2) A targeted asset restored from the state file and from on-disk has equivalent state. To keep from duplicating the code defining the assets including in the targets, the definitions have been moved to pkg/asset/targets/targets.go. Fixes https://jira.coreos.com/browse/CORS-940

The files lists in the manifests assets need to be sorted to ensure that the assets are equivalent when read from the state file and from on disk. Changes for https://jira.coreos.com/browse/CORS-940

The Common Templates asset was removed so the dependency graph needed to be updated. Generated with: $ openshift-install graph | dot -Tsvg >docs/design/resource_dep.svg using: $ dot -V dot - graphviz version 2.40.1 (20161225.0304)

staebler · 2019-02-04T16:13:57Z

@wking CI is green once again on this. Can you reapply your lgtm?

wking · 2019-02-04T18:17:08Z

/lgtm

openshift-ci-robot · 2019-02-04T18:37:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler, wking

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [staebler,wking]

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

Through da6d45b (Merge pull request openshift#890 from staebler/asset_loading_tests, 2019-02-04). Background for the networking.type validation entry is in this thread [1]. [1]: openshift#1013 (comment)

Through da6d45b (Merge pull request openshift#890 from staebler/asset_loading_tests, 2019-02-04). Background for the networking.type validation entry is in this thread [1]. Forwarding static-pod longs to systemd is still in flight with [2]. [1]: openshift#1013 (comment) [2]: openshift/cluster-bootstrap#11

Through da6d45b (Merge pull request openshift#890 from staebler/asset_loading_tests, 2019-02-04). Background for the networking.type validation entry is in this thread [1]. The OpenStack HAProxy entry has wording based on [2] and Russell's out-of-band suggestions. Forwarding static-pod longs to systemd is still in flight with [3]. [1]: openshift#1013 (comment) [2]: https://github.com/openshift/installer/pull/1185/files#r253714521 [3]: openshift/cluster-bootstrap#11

Otherwise Terraform failures will lead to exits without metadata.json, which makes it harder for users to reap any resources from the possibly-partially-created cluster. Before this commit: $ git diff -U1 | cat diff --git a/pkg/terraform/terraform.go b/pkg/terraform/terraform.go index f566d34..bad05e5 100644 --- a/pkg/terraform/terraform.go +++ b/pkg/terraform/terraform.go @@ -53,2 +53,3 @@ func Apply(dir string, platform string, extraArgs ...string) (path string, err e + return sf, errors.New("faking a Terraform failure") if exitCode := texec.Apply(dir, args, lpDebug, lpError); exitCode != 0 { $ hack/build.sh $ openshift-install --dir=wking create cluster INFO Consuming "Install Config" from target directory INFO Creating cluster... ERROR Failed to read tfstate: open /tmp/openshift-install-655768368/terraform.tfstate: no such file or directory FATAL failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: faking a Terraform failure $ ls wking auth terraform.tfvars tls With this commit, we get metadata.json too. Fixes a bug introduced by the re-order in 75ab106 (assets: add tests for validating asset fetching of targets, 2018-12-12, openshift#890, v0.12.0).

Since 3326b6b (pkg/tfvars: Pull from []Machine instead of InstallConfig, 2018-11-28, openshift#792), we've been consuming structured master configurations to generate Terraform variables. But before this commit, the Openshift asset was the WriteableAsset responsible for the master configs, giving you the following issue: 1. openshift-install create manifests i. Master asset populated with structured data ii. Openshift asset pulls in Master, flattens to YAML, and writes to disk. 2. openshift-install create cluster i. Openshift asset reads master YAML from disk. ii. TerraformVariables asset pulls in Master, but it's empty. iii. Panic [1]. With this commit, responsibility for reading and writing master manifests to disk gets shifted to the Master asset itself. Unpacking the YAML data into the appropriate structures is a bit hairy. I'm not familiar with this code though, maybe there's a better way. It will help once we complete the shift to the OpenShift machine-API types started in cf60daa (Pivot aws from cluster.k8s.io into machine.openshift.io, 2019-02-01, openshift#1175). I've also taken the opportunity to drop the list. It saves us a few lines of code for (de)serializing, and there's no upside to collecting the Machine configs together in a single file. The "glob, but not the master files" logic in the Openshift loader is also a bit ugly. Moving forward, I expect us to push the remaining Openshift assets out into their own assets as well, which would allow us to tighten down on the wildcard there. The empty-array sets (vs. using nil zero values): m.Machines = []machineapi.Machine{} m.MachinesDeprecated = []clusterapi.Machine{} avoid issues with round-tripping through the asset store. To support that, I also had to change some: if m.Machines == nil { to: if len(m.Machines) == 0 { which we will be able to drop once we get rid of MachinesDeprecated. For similar changes in earlier work, see the comments about generate-load tests in 75ab106 (assets: add tests for validating asset fetching of targets, 2018-12-12, openshift#890). [1]: openshift#1205

Since 3326b6b (pkg/tfvars: Pull from []Machine instead of InstallConfig, 2018-11-28, openshift#792), we've been consuming structured master configurations to generate Terraform variables. But before this commit, the Openshift asset was the WriteableAsset responsible for the master configs, giving you the following issue: 1. openshift-install create manifests i. Master asset populated with structured data ii. Openshift asset pulls in Master, flattens to YAML, and writes to disk. 2. openshift-install create cluster i. Openshift asset reads master YAML from disk. ii. TerraformVariables asset pulls in Master, but it's empty. iii. Panic [1]. With this commit, responsibility for reading and writing master manifests to disk gets shifted to the Master asset itself. Unpacking the YAML data into the appropriate structures is a bit hairy. I'm not familiar with this code though, maybe there's a better way. It will help once we complete the shift to the OpenShift machine-API types started in cf60daa (Pivot aws from cluster.k8s.io into machine.openshift.io, 2019-02-01, openshift#1175). I've also taken the opportunity to drop the list. It saves us a few lines of code for (de)serializing, and there's no upside to collecting the Machine configs together in a single file. The "glob, but not the master files" logic in the Openshift loader is also a bit ugly. Moving forward, I expect us to push the remaining Openshift assets out into their own assets as well, which would allow us to tighten down on the wildcard there. The empty-array sets (vs. using nil zero values): m.Machines = []machineapi.Machine{} m.MachinesDeprecated = []clusterapi.Machine{} avoid issues with round-tripping through the asset store. To support that, I also had to change some: if m.Machines == nil { to: if len(m.Machines) == 0 { which we will be able to drop once we get rid of MachinesDeprecated. For similar changes in earlier work, see the comments about generate-load tests in 75ab106 (assets: add tests for validating asset fetching of targets, 2018-12-12, openshift#890). There's also a bit of dancing to ensure our Machine filenames are ordered by increasing index. I'm padding the filenames with %02d (for example) to get ...-01.yaml, etc. so they come back in the right order on load (filepath.Glob sorts its output [2]). To avoid hard-coding a pad size, I format the largest index, measure its length, and use that length to construct padFormat. [1]: openshift#1205 [2]: https://github.com/golang/go/blob/go1.11.5/src/path/filepath/match.go#L325

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 13, 2018

openshift-ci-robot requested review from crawford and rajatchopra December 13, 2018 00:35

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2018

This was referenced Dec 13, 2018

asset/templates: remove fileName field #885

Closed

assets/templates: remove common templates asset #884

Closed

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2018

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2019

staebler mentioned this pull request Jan 11, 2019

asset/manifest: do not load network nor ingress from disk #877

Closed

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 14, 2019

abhinavdahiya reviewed Jan 16, 2019

View reviewed changes

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 22, 2019

staebler mentioned this pull request Jan 23, 2019

data/aws: use azs for master set in manifests #1121

Merged

wking reviewed Jan 30, 2019

View reviewed changes

pkg/asset/targets/targets.go Show resolved Hide resolved

abhinavdahiya mentioned this pull request Jan 30, 2019

network: move from operator-specific CRD to cluster config #1013

Merged

openshift-ci-robot assigned wking Jan 30, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2019

staebler added 5 commits February 1, 2019 12:37

assets/manifests: sort files list in manifests assets

ef1bae2

The files lists in the manifests assets need to be sorted to ensure that the assets are equivalent when read from the state file and from on disk. Changes for https://jira.coreos.com/browse/CORS-940

docs: rebuild dependency graph

17a3965

The Common Templates asset was removed so the dependency graph needed to be updated. Generated with: $ openshift-install graph | dot -Tsvg >docs/design/resource_dep.svg using: $ dot -V dot - graphviz version 2.40.1 (20161225.0304)

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 1, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2019

openshift-merge-robot merged commit da6d45b into openshift:master Feb 4, 2019

wking mentioned this pull request Feb 5, 2019

CHANGELOG: Document changes since 0.11.0 #1191

Merged

rajatchopra mentioned this pull request Feb 6, 2019

A new asset called user-manifests that just picks up user defined manifests #1176

Closed

wking mentioned this pull request Feb 6, 2019

pkg/asset/targets: Render the metadata asset before cluster #1199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assets: add tests for validating asset fetching of targets #890

assets: add tests for validating asset fetching of targets #890

staebler commented Dec 13, 2018

staebler commented Dec 13, 2018 •

edited

Loading

abhinavdahiya Jan 16, 2019

staebler Jan 17, 2019

wking Jan 30, 2019 •

edited

Loading

staebler Jan 30, 2019

staebler Jan 30, 2019

wking commented Jan 30, 2019

wking commented Jan 30, 2019

staebler commented Jan 30, 2019

staebler commented Jan 30, 2019

wking commented Jan 30, 2019

wking commented Jan 31, 2019

wking commented Jan 31, 2019

staebler commented Feb 4, 2019

wking commented Feb 4, 2019

openshift-ci-robot commented Feb 4, 2019

		@@ -91,31 +89,5 @@ func (d DNS) Files() []asset.File {

		// Load loads the already-rendered files back from disk.
		func (d *DNS) Load(f asset.FileFetcher) (bool, error) {

		@@ -0,0 +1 @@
		../../../data/data

assets: add tests for validating asset fetching of targets #890

assets: add tests for validating asset fetching of targets #890

Conversation

staebler commented Dec 13, 2018

staebler commented Dec 13, 2018 • edited Loading

abhinavdahiya Jan 16, 2019

Choose a reason for hiding this comment

staebler Jan 17, 2019

Choose a reason for hiding this comment

wking Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

staebler Jan 30, 2019

Choose a reason for hiding this comment

staebler Jan 30, 2019

Choose a reason for hiding this comment

wking commented Jan 30, 2019

wking commented Jan 30, 2019

staebler commented Jan 30, 2019

staebler commented Jan 30, 2019

wking commented Jan 30, 2019

wking commented Jan 31, 2019

wking commented Jan 31, 2019

staebler commented Feb 4, 2019

wking commented Feb 4, 2019

openshift-ci-robot commented Feb 4, 2019

staebler commented Dec 13, 2018 •

edited

Loading

wking Jan 30, 2019 •

edited

Loading