*: include terraform in the installer binary #797

crawford · 2018-12-05T23:15:25Z

Instead of requiring that the user has Terraform installed, this now
includes the pre-built Terraform binary within the installer. At
runtime, the installer unpacks Terraform into the temporary directory
and executes it from that location. This also simplifies the version
output - since the Terraform version is known at build-time, there is
no need to show the user. Lastly, this cleans up some of the log
messages so that they don't explicitly refer to Terraform.

Including Terraform in the installer adds 18MB to the binary size,
bringing the total up to 82MB.

openshift-ci-robot · 2018-12-05T23:15:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford

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~~ [crawford]

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

smarterclayton · 2018-12-05T23:20:49Z

What's the final size?

crawford · 2018-12-05T23:24:20Z

Size after this change:

$ size bin/openshift-install
   text    data     bss     dec     hex filename
55288484         375497  253360 55917341        3553b1d bin/openshift-install

$ ls -lah bin/openshift-install
-rwxr-xr-x 1 alex users 82M Dec  5 15:21 bin/openshift-install

v.s. before:

$ size bin/openshift-install
   text    data     bss     dec     hex filename
37110376         375497  253360 37739233        23fdae1 bin/openshift-install

$ ls -lah bin/openshift-install
-rwxr-xr-x 1 alex users 64M Dec  5 15:23 bin/openshift-install

smarterclayton · 2018-12-05T23:27:07Z

Well within reason. I assume having a vendor tree you splice in as an switch filepath.Base(os.Args[0]) { case "terraform": terraform.Entrypoint(); default: installer.ENtrypoint() } was deemed too painful right now?

crawford · 2018-12-05T23:29:11Z

I didn't bother looking at vendoring Terraform because I was almost certain that was going to be painful and would take much longer to accomplish. We can always revisit this approach later.

smarterclayton · 2018-12-05T23:29:46Z

Yeah that's fine. I only bring it up because of previous discussion around OSBS, all I care about right now is something that works.

crawford · 2018-12-05T23:32:35Z

Curious if this actually passes the tests...

/retest

wking · 2018-12-05T23:33:18Z

I'm fine with this approach in the short term. 👍

crawford · 2018-12-06T01:07:01Z

/retest

hack/build.sh

pkg/terraform/executor.go

pkg/terraform/terraform.go

wking · 2018-12-06T03:11:29Z

release.sh will need terraform for the target OS/arch. Not sure off the top of my head how to address that. Shifting hardlinks?

Instead of requiring that the user has Terraform installed, this now includes the pre-built Terraform binary within the installer. At runtime, the installer unpacks Terraform into the temporary directory and executes it from that location. This also simplifies the version output - since the Terraform version is known at build-time, there is no need to show the user. Lastly, this cleans up some of the log messages so that they don't explicitly refer to Terraform. Including Terraform in the installer adds 18MB to the binary size, bringing the total up to 82MB.

crawford · 2018-12-06T18:00:09Z

release.sh will need terraform for the target OS/arch. Not sure off the top of my head how to address that. Shifting hardlinks?

We also need to address the "caching" logic. It's currently just checking to see that terraform exists. It doesn't verify that it is the correct version or architecture. I think if we solve that, it also solves the issue you brought up.

crawford · 2018-12-06T19:19:56Z

hack/get-terraform.sh

+fi &&
+mkdir -p data/data/bin &&
+rm -f data/data/bin/terraform &&
+ln -s "../../../${TERRAFORM_BINARY}" data/data/bin/terraform


@wking do you know a good way of calculating "../../.." from "data/data/bin/terraform"?

I came up with the following, but it may be more effort than it is worth:

diff --git a/hack/get-terraform.sh b/hack/get-terraform.sh index aae1b69cd..c47aaa11e 100755 --- a/hack/get-terraform.sh +++ b/hack/get-terraform.sh @@ -25,6 +25,7 @@ fi && TERRAFORM_VERSION="0.11.8" && TERRAFORM_URL="https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/terraform_${TERRAFORM_VERSION}_${OS}_${ARCH}.zip" && TERRAFORM_BINARY=".cache/terraform_${TERRAFORM_VERSION}_${OS}_${ARCH}" +TERRAFORM_SYMLINK="data/data/bin/terraform" cd "$(dirname "$0")/.." && if [ ! -x "${TERRAFORM_BINARY}" ] then @@ -33,6 +34,6 @@ then curl -L "${TERRAFORM_URL}" | "${FUNZIP}" >"${TERRAFORM_BINARY}" && chmod +x "${TERRAFORM_BINARY}" fi && -mkdir -p data/data/bin && -rm -f data/data/bin/terraform && -ln -s "../../../${TERRAFORM_BINARY}" data/data/bin/terraform +mkdir -p "$(dirname "${TERRAFORM_SYMLINK}")" && +rm -f "${TERRAFORM_SYMLINK}" && +ln -s "$(dirname "${TERRAFORM_SYMLINK}" | sed 's_[^/]*_.._g')/${TERRAFORM_BINARY}" "${TERRAFORM_SYMLINK}"

do you know a good way of calculating "../../.." from "data/data/bin/terraform"?

Well, one way would be to use hardlinks instead of symlinks:

$ rm -f data/data/bin/terraform $ ls -l .cache/terraform_0.11.8_linux_amd64 -rwxr-xr-x. 1 trking trking 76422048 Dec 6 21:54 .cache/terraform_0.11.8_linux_amd64 $ ln .cache/terraform_0.11.8_linux_amd64 data/data/bin/terraform $ ls -l .cache/terraform_0.11.8_linux_amd64 -rwxr-xr-x. 2 trking trking 76422048 Dec 6 21:54 .cache/terraform_0.11.8_linux_amd64 $ ls -l data/data/bin/terraform -rwxr-xr-x. 2 trking trking 76422048 Dec 6 21:54 data/data/bin/terraform

See the <number of links> field changing from 1 to 2 after the ln call.

I assume all of this will always be on the same filesystem, so a hardlink should work. I'll go with that instead.

openshift-ci-robot · 2018-12-06T20:12:45Z

@crawford: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`4014f34`	link	`/test e2e-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wking · 2018-12-07T05:37:53Z

pkg/terraform/terraform.go

+		return errors.Wrap(err, "failed to unpack Terraform binary")
+	}
+
+	err = os.Chmod(filepath.Join(dir, executablePath), 0555)


This will need FromSlash too, and so will exec.Command(filepath.Join(clusterDir, executablePath), args...). In fact, why are we bothering with bin at all? This would be easier if we just had executableFilename = "terraform", or some such. And since we have CI to catch us if we typo an instance, I'd be fine hard-coding "terraform"everywhere and skipping the variable altogether (although I'm also fine with a slash-lessexecutableFilename`).

wking · 2018-12-07T05:39:09Z

pkg/terraform/executor.go

 // Terraform call itself failed, in which case, details can be found in the
 // output.
-func (ex *executor) execute(clusterDir string, args ...string) error {
+func Execute(clusterDir string, args ...string) error {


No need to make this public. Can we change this (and the name in the godoc above) to execute?

wking · 2018-12-07T06:02:03Z

I left a few minor suggestions inline, but I'm fine landing this as-is (after you squash the WIP commit in) and iterating afterwards if you don't want to pick up the suggestions now.

crawford · 2018-12-10T20:43:25Z

Closing in favor of #822.

/close

openshift-ci-robot · 2018-12-10T20:43:27Z

@crawford: Closed this PR.

Details

In response to this:

Closing in favor of #822.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

crawford requested review from abhinavdahiya and wking December 5, 2018 23:15

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 5, 2018

openshift-ci-robot requested review from russellb and staebler December 5, 2018 23:15

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2018

crawford changed the title ~~WIP: move terraform into the installer~~ *: include terraform in the installer binary Dec 5, 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 Dec 5, 2018

wking reviewed Dec 6, 2018

View reviewed changes

hack/build.sh Outdated Show resolved Hide resolved

wking reviewed Dec 6, 2018

View reviewed changes

pkg/terraform/executor.go Outdated Show resolved Hide resolved

wking reviewed Dec 6, 2018

View reviewed changes

pkg/terraform/terraform.go Outdated Show resolved Hide resolved

WIP

4014f34

crawford commented Dec 6, 2018

View reviewed changes

wking reviewed Dec 7, 2018

View reviewed changes

wking mentioned this pull request Dec 7, 2018

Mention that terraform is needed before you actually need it #811

Closed

abhinavdahiya mentioned this pull request Dec 7, 2018

Switch to vendored terraform codebase rather than invoking separate binary #822

Merged

openshift-ci-robot closed this Dec 10, 2018

crawford deleted the terraform branch December 10, 2018 20:43

*: include terraform in the installer binary #797

*: include terraform in the installer binary #797

Uh oh!

Conversation

crawford commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Dec 5, 2018

Uh oh!

smarterclayton commented Dec 5, 2018

Uh oh!

crawford commented Dec 5, 2018

Uh oh!

smarterclayton commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crawford commented Dec 5, 2018

Uh oh!

smarterclayton commented Dec 5, 2018

Uh oh!

crawford commented Dec 5, 2018

Uh oh!

wking commented Dec 5, 2018

Uh oh!

crawford commented Dec 6, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

crawford commented Dec 6, 2018

Uh oh!

crawford Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

crawford Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

wking Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

crawford Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Dec 6, 2018

Uh oh!

wking Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

wking Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

wking commented Dec 7, 2018

Uh oh!

crawford commented Dec 10, 2018

Uh oh!

openshift-ci-robot commented Dec 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crawford commented Dec 5, 2018 •

edited

Loading

smarterclayton commented Dec 5, 2018 •

edited

Loading

wking commented Dec 6, 2018 •

edited

Loading