Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 2, 2018

Conversation

wking
Copy link
Member

@wking wking commented Oct 1, 2018

Docs for Go's build constraints are here. This pull request allows folks with local libvirt C libraries to compile our libvirt deletion logic (and get a dynamically-linked executable), while release binaries and folks without libvirt C libraries can continue to get statically-linked executables that lack libvirt deletion.

/assign @abhinavdahiya

Testing this locally, I'm getting:

$ env | grep OPENSHIFT_INSTALL_ | sort
OPENSHIFT_INSTALL_BASE_DOMAIN=installer.testing
OPENSHIFT_INSTALL_CLUSTER_NAME=wking
OPENSHIFT_INSTALL_DATA=/home/trking/src/openshift/installer/data/data
[email protected]
OPENSHIFT_INSTALL_LIBVIRT_IMAGE=http://aos-ostree.rhev-ci-vms.eng.rdu2.redhat.com/rhcos/images/cloud/latest/rhcos-qemu.qcow2.gz
OPENSHIFT_INSTALL_LIBVIRT_URI=qemu+tcp://192.168.122.1/system
OPENSHIFT_INSTALL_PASSWORD=asdf
OPENSHIFT_INSTALL_PLATFORM=libvirt
OPENSHIFT_INSTALL_PULL_SECRET_PATH=/home/trking/src/openshift/installer/pull-secret.json
OPENSHIFT_INSTALL_SSH_PUB_KEY_PATH=/home/trking/.ssh/id_rsa.pub
$ openshift-install --log-level=debug cluster
...
DEBU[0021] Generating Cluster...                        
FATA[0021] failed to generate asset: failed to generate asset "Cluster": failed to run Terraform: signal: segmentation fault 

Dunno what's going on there yet.

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 1, 2018
@wking
Copy link
Member Author

wking commented Oct 1, 2018

I get the segfault in master too, so it's probably orthogonal. I'm digging into it now.

@wking wking force-pushed the libvirt-delete-tag branch 2 times, most recently from e65f8f7 to 70a37ae Compare October 1, 2018 21:51
@wking
Copy link
Member Author

wking commented Oct 1, 2018

FATA[0021] failed to generate asset: failed to generate asset "Cluster": failed to run Terraform: signal: segmentation fault

This was from a corrupted terraform:

$ sha1sum bin/terraform ~/bin/terraform-0.11.8 
5aa0067541c0273bb650ed89be3b1b1f4dc83944  bin/terraform
06339cdd7a39657355e5c4b04e12efc91672a450  /home/trking/bin/terraform-0.11.8

Pulling a fresh copy with hack/get-terraform.sh got me the 0633... hash in bin/terraform and fixed the issue.

@wking wking force-pushed the libvirt-delete-tag branch from 70a37ae to d0feed5 Compare October 1, 2018 22:38
@crawford
Copy link
Contributor

crawford commented Oct 1, 2018

Can we hide all of this behind the dev mode? Developers are the only ones that will use libvirt and since it's likely that they are compiling the installer locally, cgo should play nicely.

@wking
Copy link
Member Author

wking commented Oct 1, 2018

Can we hide all of this behind the dev mode? Developers are the only ones that will use libvirt and since it's likely that they are compiling the installer locally, cgo should play nicely.

@steveej, at least, had issues with dynamic linking (so he might want dev-mode's lack of compiled-in Terraform without libvirt deletion). And @abhinavdahiya would prefer not to have to mess with OPENSHIFT_INSTALL_DATA (so he might want release-mode's compiled-in Terraform with libvirt deletion). But if you can convince either of them, I'm happy to update the code ;).

@crawford
Copy link
Contributor

crawford commented Oct 1, 2018

Looks good. Can you add a quick note to the docs regarding the build tag?

@wking wking force-pushed the libvirt-delete-tag branch from d0feed5 to c5fb068 Compare October 1, 2018 23:45
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2018
@wking
Copy link
Member Author

wking commented Oct 1, 2018

Can you add a quick note to the docs regarding the build tag?

Done with d0feed5 -> c5fb068.

Docs for Go's build constraints are in [1].  This commit allows folks
with local libvirt C libraries to compile our libvirt deletion logic
(and get a dynamically-linked executable), while release binaries and
folks without libvirt C libraries can continue to get
statically-linked executables that lack libvirt deletion.

I've also simplified the public names (e.g. NewDestroyer -> New),
dropping information which is already encoded in the import path.

Pulling the init() registration out into separate files is at
Abhinav's request [2].

[1]: https://golang.org/pkg/go/build/#hdr-Build_Constraints
[2]: openshift#387 (comment)
@wking wking force-pushed the libvirt-delete-tag branch from c5fb068 to 4d940ce Compare October 1, 2018 23:47
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2018
@wking
Copy link
Member Author

wking commented Oct 1, 2018

And rebased around #384 with c5fb068 -> 4d940ce.

@crawford
Copy link
Contributor

crawford commented Oct 1, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, 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:

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 openshift-merge-robot merged commit bbf5256 into openshift:master Oct 2, 2018
@wking wking deleted the libvirt-delete-tag branch October 2, 2018 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants