Skip to content

[wip] Libvirt plugin via golang plugin instead of TAGS=libvirt#3225

Closed
dcbw wants to merge 2 commits intoopenshift:masterfrom
dcbw:libvirt-plugin
Closed

[wip] Libvirt plugin via golang plugin instead of TAGS=libvirt#3225
dcbw wants to merge 2 commits intoopenshift:masterfrom
dcbw:libvirt-plugin

Conversation

@dcbw
Copy link
Contributor

@dcbw dcbw commented Mar 3, 2020

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 3, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign smarterclayton
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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

Details 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

@stbenjam
Copy link
Member

stbenjam commented Mar 3, 2020

It's an interesting idea but having the installer be a single binary is pretty convenient, how would we distribute the libvirt plugin?

When we talked about this before, we discussed having terraform-provider-libvirt not use C-bindings at all, e.g. https://github.com/digitalocean/go-libvirt. Then even a mac could run the baremetal or libvirt installers to install on a remote linux host.

@dcbw
Copy link
Contributor Author

dcbw commented Mar 3, 2020

It's an interesting idea but having the installer be a single binary is pretty convenient, how would we distribute the libvirt plugin?

When we talked about this before, we discussed having terraform-provider-libvirt not use C-bindings at all, e.g. https://github.com/digitalocean/go-libvirt. Then even a mac could run the baremetal or libvirt installers.

@stbenjam yeah, that seems like a bunch more work, but would certainly be nicer.

@openshift-ci-robot
Copy link
Contributor

@dcbw: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/gofmt 02ce98d link /test gofmt
ci/prow/govet 02ce98d link /test govet
ci/prow/verify-vendor 02ce98d link /test verify-vendor
ci/prow/unit 02ce98d link /test unit
ci/prow/tf-fmt 02ce98d link /test tf-fmt
ci/prow/images 02ce98d link /test images
ci/prow/e2e-aws-upgrade 02ce98d link /test e2e-aws-upgrade
ci/prow/e2e-aws 02ce98d link /test e2e-aws
ci/prow/e2e-ovirt 02ce98d link /test e2e-ovirt
ci/prow/e2e-aws-scaleup-rhel7 02ce98d link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-aws-fips 02ce98d link /test e2e-aws-fips
ci/prow/e2e-openstack 02ce98d link /test e2e-openstack
ci/prow/e2e-libvirt 02ce98d link /test e2e-libvirt
ci/prow/golint 02ce98d link /test golint

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.

@dcbw
Copy link
Contributor Author

dcbw commented Mar 4, 2020

@stbenjam so unfortunately we can't just convert over go go-libvirt in the installer, because github.com/dmacvicar/terraform-provider-libvirt still uses libvirt-go C bindings. And that's a heavy thing to lift.

But the installer is already downloaded as a tarball with the binary and a README (eg https://openshift-release-artifacts.svc.ci.openshift.org/4.4.0-0.nightly-2020-03-04-191159/). Would it be such a big problem to ship a shared object in the Linux build of the installer too?

@dcbw dcbw changed the title Libvirt plugin via golang plugin instead of TAGS=libvirt [wip] Libvirt plugin via golang plugin instead of TAGS=libvirt Mar 4, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2020
@stbenjam
Copy link
Member

stbenjam commented Mar 4, 2020

@stbenjam so unfortunately we can't just convert over go go-libvirt in the installer, because github.com/dmacvicar/terraform-provider-libvirt still uses libvirt-go C bindings. And that's a heavy thing to lift.

But the installer is already downloaded as a tarball with the binary and a README (eg https://openshift-release-artifacts.svc.ci.openshift.org/4.4.0-0.nightly-2020-03-04-191159/). Would it be such a big problem to ship a shared object in the Linux build of the installer too?

We'd need to update oc to understand extracting multiple files (not a big deal). It also affects automation which will be a bigger deal. For libvirt alone it's probably not bad, but I'm more concerned about baremetal which uses a libvirt bootstrap VM.

If libvirt folks want this, could we keep the TAGS for building openshift-baremetal-install? I really find a single binary more convenient (it's extractable with oc as well, openshift/oc#52)

What does the plugin buy us? I guess we could stop shipping multiple installer images.

/cc @vrutkovs

@dcbw
Copy link
Contributor Author

dcbw commented Apr 2, 2020

Not going to have time to pursue this further...

@dcbw dcbw closed this Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants