Skip to content
This repository was archived by the owner on Feb 5, 2020. It is now read-only.

Conversation

@squeed
Copy link

@squeed squeed commented Apr 27, 2018

This introduces basic libvirt support for the following steps:

  • Assets
  • Topology
  • Bootstrap
  • Etcd
  • tnc_dns

@coreosbot
Copy link

Can one of the admins verify this patch?

@squeed squeed force-pushed the topology-libvirt branch from b117489 to 344f930 Compare April 27, 2018 16:45
@squeed
Copy link
Author

squeed commented Apr 27, 2018

ok to test

@squeed squeed force-pushed the topology-libvirt branch from 344f930 to 22541be Compare April 27, 2018 16:47
@@ -0,0 +1,9 @@
variable "coreos_qow_path" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: variables are usually in variables.tf

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Yes they are.

@crawford
Copy link
Contributor

I tried running this, but it ultimately failed while trying to create AWS resources (while realizing data.terraform_remote_state.bootstrap).

@squeed
Copy link
Author

squeed commented Apr 30, 2018

@crawford I'm not sure where that's happening. Right now, we only support the bootstrap phase, so you have to manually run

tectonic install assets
tectonic install bootstrap

I tried this without a valid AWS configuration and it worked.

@squeed squeed force-pushed the topology-libvirt branch from 22541be to ed52540 Compare April 30, 2018 09:25
@squeed
Copy link
Author

squeed commented Apr 30, 2018

retest this please

@squeed squeed force-pushed the topology-libvirt branch from ed52540 to 1c6068d Compare April 30, 2018 09:29
@squeed
Copy link
Author

squeed commented Apr 30, 2018

all green! don't merge until the operator changes are merged.

@enxebre
Copy link
Contributor

enxebre commented Apr 30, 2018

retest this please

@squeed
Copy link
Author

squeed commented Apr 30, 2018

Failing on container linux version - should I skip this test?

@squat
Copy link
Contributor

squat commented Apr 30, 2018

@enxebre I thought you had disabled the check CL version test, no?

@enxebre
Copy link
Contributor

enxebre commented Apr 30, 2018

@squat @squeed the test disabled is for the cl channel, the cl version should succeed, this probably needs to be updated as this PR is changing the paths https://github.com/coreos/tectonic-installer/blob/master/tests/rspec/lib/aws_cluster.rb#L269

@squeed squeed force-pushed the topology-libvirt branch from 1c6068d to 8002aab Compare April 30, 2018 14:14
@squeed
Copy link
Author

squeed commented Apr 30, 2018

@enxebre ah, good find! let's see if this does it.

@squeed squeed force-pushed the topology-libvirt branch from 8002aab to 588369e Compare May 2, 2018 08:56
@squeed
Copy link
Author

squeed commented May 2, 2018

rebased.
The TNC changes have made master, so this can be merged on green.

@squeed
Copy link
Author

squeed commented May 3, 2018

The tests are failing, and I don't know how they ever worked in UT2: it's trying to get the worker instance ids from the bootstrap.tfstate file. Now that we have split tfstates...
Thoughts, @spangenberg?

@enxebre
Copy link
Contributor

enxebre commented May 3, 2018

@squeed tests are failing because they can't determine the bootkube service has run successfully because they can't ssh into the machines. Have you deployed this locally successfully? I'll try to give it a go

@squeed squeed force-pushed the topology-libvirt branch from 588369e to 24594bc Compare May 7, 2018 13:20
… libvirt

This does the following:
* Move most of asset generation to assets/base
* Consumes this "base" module in assets/aws
* Moves aws-specific variables to variables-aws.tf
* Adds the beginning of libvirt support
@squeed squeed force-pushed the topology-libvirt branch from 24594bc to 2e77d37 Compare May 7, 2018 14:50
@squeed squeed force-pushed the topology-libvirt branch from 2e77d37 to d9e2de8 Compare May 7, 2018 22:07
@squeed
Copy link
Author

squeed commented May 7, 2018

It's red because I typo'd the tnc image name. Let's try this again.

@squeed
Copy link
Author

squeed commented May 7, 2018

All green! ready to merge for real, this time.

Copy link
Contributor

@squat squat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@squeed the PR looks great to me. The only thing I'm wondering is, since this is a 3rd party provider, should we document where/how to get the libvirt provider?

@squat squat removed the do-not-merge label May 8, 2018
@squat squat merged commit 2f6db46 into coreos:master May 8, 2018
@squeed squeed deleted the topology-libvirt branch May 8, 2018 23:07
wking added a commit to wking/openshift-installer that referenced this pull request Nov 13, 2018
We'd been defaulting it to ClusterName in InstallConfig.Generate, and
I see no reason for the user to want to create a separate name for the
network alone.  The variable dates back to 4a08942 (steps: bootstrap
/ etcd / topology support for libvirtm 2018-04-24,
coreos/tectonic-installer#3213), where it is not explicitly motivated.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants