data: Allow Terraform module bundling via vfsgen #332

wking · 2018-09-26T07:02:12Z

The new data package works two ways:

For devs, it loads the Terraform modules (now under data/data) from the disk. If you execute your installer from a directory other than data (i.e. most of the time), you should set OPENSHIFT_INSTALL_DATA to point at the data/data directory. That makes it easy to adjust the static files during development, because there's no need to rebuild after a tweak.
For users, we compile the Terraform modules into the executable. That makes it easy to distribute the file (no separate pieces or wrapping tarball).

This is very similar to #329. The differences are:

asset/cluster: include templates in binary #329 always extracts from the tarball. There's no "load from the disk at runtime" workflow.
I'm vendoring a different package ;).

The compiled-in binaries are slightly different sizes. As of my b740463d688:

$ go generate ./data
writing assets_vfsdata.go
$ CGO_ENABLED=0 go build -tags release ./cmd/openshift-install
$ ls -l openshift-install 
-rwxr-xr-x. 1 trking trking 37933716 Sep 25 23:59 openshift-install

Compared with asset/cluster: include templates in binary #329's:

$ ./hack/build.sh
$ ls -l bin/openshift-install 
-rwxr-xr-x. 1 trking trking 39625166 Sep 26 00:00 bin/openshift-install

But <2 MB isn't a large percentage change, so this is probably a wash.

I've broken the old installer by shifting its Terraform modules around, deleting symlinks, etc.

Dev builds look like:

$ CGO_ENABLED=0 go build ./cmd/openshift-install

with invocations like:

$ OPENSHIFT_INSTALL_DATA=data/data OPENSHIFT_INSTALL_CLUSTER_NAME=wking-testing ... ./openshift-install --dir wking --log-level debug cluster

Release builds look like:

$ go generate ./data
writing assets_vfsdata.go
$ CGO_ENABLED=0 go build -tags release ./cmd/openshift-install

and the invocation is the same except you don't need to bother with OPENSHIFT_INSTALL_DATA.

CC @crawford

Based on [1]. Generate at release-time with: $ go generate ./data/ I've swapped [1]'s tag from 'dev' to 'release' (with the opposite sense) because we're not going to commit the autogenerated content to the repository ('go get' support is not worth version-controlling machine-generated code). Because dev builds load the data at runtime (instead of compiling it in at build time), I've added an environment variable to fill the role of the previous terraform.BaseLocation. [1]: https://github.com/shurcooL/vfsgen/blob/33ae1944be3fe078a54c597f107e0867da19c713/README.md#go-generate-usage

wking · 2018-09-26T09:05:02Z

I've broken the old installer by shifting its Terraform modules around, deleting symlinks, etc.

I've pushed 6af70ec7bab0b21aa69a495d97ae6d6b4a0df4c9 as an ugly hack to work around this until something like openshift/release#1677 lands.

@Non-Git

When we build releases. Generated with: $ glide get --strip-vendor github.com/shurcooL/vfsgen $ glide-vc --use-lock-file --no-tests --only-code $ bazel run //:gazelle ... gazelle: /home/trking/.local/lib/go/src/github.com/openshift/installer/vendor/github.com/libvirt/libvirt-go/callbacks_wrapper.go: error reading go file: /home/trking/.local/lib/go/src/github.com/openshift/installer/vendor/github.com/libvirt/libvirt-go/callbacks_wrapper.go: pkg-config not supported: #cgo pkg-config: libvirt ... using: $ glide --version glide version 0.13.2-dev $ (cd $GOPATH/src/github.com/Masterminds/glide && git describe) v0.13.1-7-g3e13fd1 $ (cd $GOPATH/src/github.com/sgotti/glide-vc && git describe) v0.1.0-2-g6ddf6ee $ bazel version Build label: 0.17.2- (@Non-Git) Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar Build time: Fri Sep 21 15:04:25 2018 (1537542265) Build timestamp: 1537542265 Build timestamp as int: 1537542265 I'm ignoring the pkg-config issues for now, because we're about to drop Bazel.

Default to 'release' to make life easier for openshift/release, and because a release build will work more reliably (the Terraform assets are always there, they're just harder to change).

This will keep the old installer working until we can get openshift/release switched over to openshift-install. Once we do, we can revert this commit.

wking · 2018-09-26T14:31:27Z

e2e-aws:

Waiting for router to be created ...
NAME STATUS ROLES AGE VERSION
ip-10-0-12-62.ec2.internal Ready master 4m
v1.11.0+d4cacc0 ip-10-0-17-43.ec2.internal Ready master 4m
v1.11.0+d4cacc0 ip-10-0-2-231.ec2.internal Ready bootstrap 4m
v1.11.0+d4cacc0 ip-10-0-34-168.ec2.internal Ready master 4m
v1.11.0+d4cacc0
NAME DESIRED CURRENT UP-TO-DATE AVAILABLE AGE
router 1 1 1 0 5s
Installation failed error: timed out waiting for the condition
2018/09/26 09:56:59 Container setup in pod e2e-aws failed,

But smoke passed, so

/retest

abhinavdahiya · 2018-09-26T16:05:08Z

@wking

For devs, it loads the Terraform modules (now under data/data) from the disk. If you execute your installer from a directory other than data (i.e. most of the time), you should set OPENSHIFT_INSTALL_DATA

So this is required always? :/

abhinavdahiya · 2018-09-26T16:08:23Z

data/data/config.tf

@@ -0,0 +1,336 @@
+terraform {


data/data smells :) can it be data/terraform ?

data/data smells :) can it be data/terraform ?

It could, but anything that doesn't need to be dynamically templated can live in here. I thought we might want to use this approach for some of our manifests and such, as long as they didn't need templating. But I'm fine calling the directory whatever for the short-term, and sorting the rest out later. Thoughts?

abhinavdahiya · 2018-09-26T16:10:30Z

data/assets.go

+var Assets http.FileSystem
+
+func init() {
+	dir := os.Getenv("OPENSHIFT_INSTALL_DATA")


can we use GOPATH to set reasonable default?

can we use GOPATH to set reasonable default?

Maybe? But that will break in Go 1.11 when we move to modules and toss out GOPATH ;). Maybe export a default in your ~/.bashrc or whatever? Or compile it in?

wking · 2018-09-26T16:10:39Z

For devs, it loads the Terraform modules (now under data/data) from the disk. If you execute your installer from a directory other than data (i.e. most of the time), you should set OPENSHIFT_INSTALL_DATA

So this is required always? :/

No, you could run from the data package ;). But in general, yes. If it bothers you, build with the compiled-in data? I've updated build.sh to build that style by default.

abhinavdahiya · 2018-09-26T16:12:06Z

data/unpack.go

+	}
+
+	if info.IsDir() {
+		os.Mkdir(base, 0777)


does it have to be 0777?

does it have to be 0777?

It doesn't have to be, but this is generic, and your umask will reduce it to whatever you feel is reasonable.

abhinavdahiya · 2018-09-26T16:13:04Z

steps/assets/aws/config.tf

@@ -1 +0,0 @@
-../../../config.tf


Why can't we keep the symlink?

Why can't we keep the symlink?

We might be able to, but we don't need it for openshift-install (only one step :), and I wasn't sure how well http.FileSystem handled symlinks.

abhinavdahiya · 2018-09-26T16:15:00Z

pkg/asset/cluster/cluster.go

 	}

+	// take advantage of the new installer only having one step.
+	err = os.Rename(filepath.Join(tmpDir, "config.tf"), filepath.Join(templateDir, "config.tf"))


shouldn't this happen in Unpack?

shouldn't this happen in Unpack?

Unpack is generic, just dump the http.FileSystem to disk. This is specific to the content that we happen to stick into Unpack. And I was going to shuffle this around later, and not bother to write the step that we don't need (e.g. only unpack modules and steps/infra/aws for AWS). But I hadn't worked that up yet.

abhinavdahiya · 2018-09-26T16:15:53Z

modules/aws/bootstrap/README.md

@@ -0,0 +1,51 @@
+# Bootstrap Module


can you combine this with 5c536a2 ?

can you combine this with 5c536a2 ?

Yes. I made 2ac5913 a separate commit so I could revert it once openshift/release#1677 lands. But I can squash them if you like. Thoughts?

abhinavdahiya · 2018-09-26T16:20:08Z

/lgtm

crawford · 2018-09-26T16:20:09Z

Let's just get this thing in to remove the deadlock between this repo and openshift/release.

openshift-ci-robot · 2018-09-26T16:20:18Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,wking]

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

wking · 2018-09-26T16:27:07Z

Dunno why GitHub says golint is sad; Prow says it's happy.

/test golint

abhinavdahiya · 2018-09-26T16:27:07Z

/refresh

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

openshift-ci-robot requested review from rajatchopra and steveej September 26, 2018 07:02

wking force-pushed the vfsgen branch 2 times, most recently from f193a29 to a40d944 Compare September 26, 2018 09:19

wking added 4 commits September 26, 2018 02:21

asset/cluster: Port to bundled Terraform assets

4529399

hack/build: Add MODE to switch between dev and release modes

7ef3e01

Default to 'release' to make life easier for openshift/release, and because a release build will work more reliably (the Terraform assets are always there, they're just harder to change).

*: Temporarily restore the old Terraform content

2ac5913

This will keep the old installer working until we can get openshift/release switched over to openshift-install. Once we do, we can revert this commit.

wking force-pushed the vfsgen branch from a40d944 to 2ac5913 Compare September 26, 2018 09:21

crawford mentioned this pull request Sep 26, 2018

asset/cluster: include templates in binary #329

Closed

abhinavdahiya reviewed Sep 26, 2018

View reviewed changes

openshift-ci-robot assigned abhinavdahiya Sep 26, 2018

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

wking mentioned this pull request Sep 26, 2018

ci-operator/config/openshift/installer/master: Move to openshift-install openshift/release#1677

Merged

openshift-merge-robot merged commit 343e37c into openshift:master Sep 26, 2018

wking deleted the vfsgen branch September 26, 2018 19:10

wking mentioned this pull request Oct 1, 2018

pkg/destroy/libvirt: Add behind a libvirt_destroy build tag #387

Merged

		@@ -1 +0,0 @@
		../../../config.tf No newline at end of file

data: Allow Terraform module bundling via vfsgen #332

data: Allow Terraform module bundling via vfsgen #332

Uh oh!

Conversation

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

Uh oh!

Uh oh!

wking commented Sep 26, 2018

Uh oh!

wking commented Sep 26, 2018

Uh oh!

abhinavdahiya commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

wking commented Sep 26, 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!

abhinavdahiya commented Sep 26, 2018

Uh oh!

crawford commented Sep 26, 2018

Uh oh!

openshift-ci-robot commented Sep 26, 2018

Uh oh!

wking commented Sep 26, 2018

Uh oh!

abhinavdahiya commented Sep 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wking commented Sep 26, 2018 •

edited

Loading

abhinavdahiya commented Sep 26, 2018 •

edited

Loading