*: implement new bootstrap process #162

crawford · 2018-08-22T23:30:41Z

This implements the new bootstrap process, which uses a dedicated bootstrap node instead of overloading the master nodes. This also moves the etcd members to the master nodes and removes the etcd nodes.

NOTE: This requires the use of a fork of terraform-provider-libvirt.
Once this new scheme has been validated, the changes will be
submitted upstream.

abhinavdahiya · 2018-08-23T00:10:09Z

/ok to test

abhinavdahiya · 2018-08-23T00:21:00Z

installer/pkg/config-generator/ignition.go

This needs to be based on master index.

abhinavdahiya · 2018-08-23T00:25:16Z

modules/ignition/outputs.tf

This leaves the bootstrap s3 bucket behind ??

This leaves the bootstrap s3 bucket behind ??

Maybe we can have the bootstrap node serve these directly? But even if that's useful, we may want to punt it to follow-up work.

Yes, the bootstrap node and the S3 bucket are left behind. The follow-up work will take care of that.

abhinavdahiya · 2018-08-23T00:29:05Z

Documentation/dev/libvirt-howto.md

might be worth mentioning in the PR description.

It would also be good to link the upstream PR once you fike one for your fixup commits.

I don't know if these have made it into a PR or not, but #214 took us back to the upstream repo. I'm not sure what the upstream changes were, maybe dmacvicar/terraform-provider-libvirt#376 and dmacvicar/terraform-provider-libvirt#382? Although we may be reverting #214 in #219.

abhinavdahiya

amazing work!!

wking · 2018-08-23T04:57:08Z

installer/pkg/config/libvirt/libvirt.go

This return works if and only if setting up WorkerIPs is the last thing this function does. That's true as of this commit, but its brittle (this commit breaks that assumption for MasterIPs above; wheb MasterIPs is set before this function, this function will erroneously ignore WorkerIPs). I'd rather use a pattern like:

if len(l.WorkerIPs) > 0 { if len(l.WorkerIPs) != workerCount { return fmt.Errorf("length of WorkerIPs doesn't match worker count") } } else { // build `WorkerIPs }

Also, with this logic being used for maters and workers, it may be worth spinning out into a helper function. Something like:

func generateIPs(ips []string, count int, network SomeType, offset int) error

Yes, this logic is very broken. I didn't put much effort into it because we are going to throw this all away pretty soon.

wking · 2018-08-23T05:06:36Z

installer/pkg/workflow/utils.go

nit: the rest of these constants seem to be listed in alphabetical order. Can we shift this up?

wking · 2018-08-23T05:18:19Z

steps/assets/aws/resources/rm-assets.sh

This may have been the last awscli consumer. Can you drop it from config.tf?

wking · 2018-08-23T05:32:14Z

steps/bootstrap/aws/main.tf

nit: ${a == b ? false : true } could (I think) be written more compactly as ${a != b}.

lol, yeah. I copied that from elsewhere. There are a lot of strange boolean expressions in this codebase....

wking · 2018-08-23T05:36:09Z

modules/ignition/outputs.tf

This leaves the bootstrap s3 bucket behind ??

Maybe we can have the bootstrap node serve these directly? But even if that's useful, we may want to punt it to follow-up work.

wking · 2018-08-23T05:56:21Z

steps/etcd/aws/etcd.tf

Why aren't we dropping steps/etcd completely?

That comes later. In order to drop it completely, I would have to break all of our tests. I'd rather get this merged, update the config in the release repo, and then drop etcd (I have the commit queued up).

wking · 2018-08-23T06:02:56Z

Documentation/dev/libvirt-howto.md

It would also be good to link the upstream PR once you fike one for your fixup commits.

wking · 2018-08-23T06:05:00Z

steps/topology/aws/main.tf

nit: You can drop this line.

wking · 2018-08-23T06:06:48Z

steps/topology/aws/main.tf

nit: No need to add new "Tectonic" namespacing. Can this be just tnc_a?

wking · 2018-08-23T06:09:15Z

installer/pkg/config-generator/ignition.go

Is this TODO stale now that there are no more etcd nodes?

wking · 2018-08-23T18:32:23Z

images/tectonic-builder/Dockerfile

This awscli is distinct from the awscli image you're dropping from config.tf. I think we should drop the config.tf entry, since you're dropping its rm-assets.sh consumer. This entry was added in 084ff71 (coreos/tectonic-installer#980), and the motivation was supporting role changes in smoke tests (now over here). So I think we want to leave this line alone in this PR.

👍 Sorry, I just get so excited when I can delete things.

wking · 2018-08-23T18:49:21Z

tests/smoke/aws/README.md

nit: We can drop jq and remove the wrapping [] with:

$ aws ec2 describe-instances --query "Reservations[].Instances[] | [?Tags[? Key == 'Name' && Value == '${CLUSTER_NAME}-master-0']].PublicIpAddress" --output text 52.15.184.15

lol, this is from #88.

wking · 2018-08-23T19:05:33Z

installer/pkg/config/libvirt/libvirt.go

nit: this "do we already have some, and if so, do we have the right number?" check could move into generateIPs to save you some repetition here.

crawford · 2018-08-23T22:27:14Z

/hold

crawford · 2018-08-23T23:36:06Z

/hold cancel

abhinavdahiya · 2018-08-24T00:15:17Z

/lgtm

yifan-gu · 2018-08-24T02:12:13Z

LGTM! Massive progress!

yifan-gu · 2018-08-24T02:12:55Z

/lgtm re-applying the label.

openshift-ci-robot · 2018-08-24T02:13:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford, yifan-gu

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,crawford,yifan-gu]

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

openshift-ci-robot · 2018-08-24T02:21:19Z

New changes are detected. LGTM label has been removed.

Instead of generating the worker IP addresses in Terraform, generate them alongside the master IP addresses and pass them in as a variable.

Instead of generating the etcd IP addresses in Terraform, generate them alongside the other IP addresses and pass them in as a variable.

Instead of shelling out to `virsh net-update` repeatedly, define all of the DNS records upfront in the network config. This has two major advantages: 1. This removes the race between network creation and network modification. 2. This allows for RRDNS. There is a bug in `virsh net-update` that prevents two hosts from sharing the same hostname even though the network config allows it. NOTE: This requires the use of a fork of terraform-provider-libvirt. Once this new scheme has been validated, the changes will be submitted upstream.

This will allow users to reboot their host and have the cluster come back up automatically.

This also removes the rm-assets service since the bootstrap node itself will be removed after the cluster is bootstrapped. Because rm-assets was the only service that was actually enabled, tectonic.service instead needs to be disabled. config.tf is not bumped here because there is no corresponding change in TNC. Even if the master config was served from TNC, there would be a race condition between the etcd cluster becoming healthy and the master requesting its config. Rather than attempting to address this race, I'm ignoring it since the etcd members are going to be moved to the master soon, which will remove the race.

openshift-ci-robot · 2018-08-24T03:39:44Z

New changes are detected. LGTM label has been removed.

abhinavdahiya · 2018-08-24T05:14:16Z

@crawford
e2e-aws uses this https://github.com/openshift/release/blob/master/ci-operator/templates/cluster-launch-installer-e2e.yaml#L214-L244 to decide when cluster is booted completely.

https://github.com/coreos-inc/tectonic-operators/pull/437, moved router from tectonic-ingress to openshift-ingress, was merged before your etcd change. You are bringing those changes too here in this PR.

The router never runs as ingress operator is unable to install components as #149 is still pending. maybe try to cherry pick cdda678 commit and see if that installs router.. :/

Makes installer consistent with 4.0 direction.

Now that all of the records are created upfront in the topology step and the bootstrap node uses a static location for its config (no more CNAMEs), there is no need to explicitly manipulate the records later.

This puts the etcd members on the master nodes instead of the dedicated etcd nodes.

Instead of failing with an extremely opaque error message, this will report which pool was unrecognized. Users are likely to hit this now that etcd has been removed.

Now that the etcd members have been moved to the master nodes, there is no need for the etcd nodes. The installer still understands the etcd config even though it isn't used. This is to prevent the tests from breaking. This code will be removed in the near future.

506ca08 (*: move etcd members to the master nodes, 2018-08-16, openshift#162) converted tectonic_ignition_master to tectonic_ignition_masters (among many other things). But one of the purposes of the bootstrap node was to allow us to avoid special-casing individual master nodes. This commit moves us back towards identical masters. I'm not clear on the purpose of the etcd_index query, which dates back to 5ce1215 (add tf support for running etcd from tnc, 2018-03-27, coreos/tectonic-installer#3079). We'll see how the CI tests do without it.

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 Aug 22, 2018

openshift-ci-robot requested review from aaronlevy and wking August 22, 2018 23:30

crawford requested review from abhinavdahiya and yifan-gu August 22, 2018 23:31

abhinavdahiya added the run-smoke-tests label Aug 23, 2018

abhinavdahiya reviewed Aug 23, 2018

View reviewed changes

installer/pkg/config-generator/ignition.go Outdated

Copy link

Contributor

abhinavdahiya Aug 23, 2018

Choose a reason for hiding this comment

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

This needs to be based on master index.

abhinavdahiya reviewed Aug 23, 2018

View reviewed changes

wking reviewed Aug 23, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2018

openshift-ci-robot assigned abhinavdahiya Aug 24, 2018

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

abhinavdahiya approved these changes Aug 24, 2018

View reviewed changes

crawford mentioned this pull request Aug 24, 2018

tests/run: Use one Python invocation (no jq) for editing the config #166

Merged

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2018

openshift-ci-robot assigned yifan-gu Aug 24, 2018

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

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2018

crawford added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2018

crawford added 5 commits August 23, 2018 20:32

*: generate worker addresses in installer

dca0138

Instead of generating the worker IP addresses in Terraform, generate them alongside the master IP addresses and pass them in as a variable.

*: generate etcd addresses in installer

ba5baf0

Instead of generating the etcd IP addresses in Terraform, generate them alongside the other IP addresses and pass them in as a variable.

steps/topology: autostart the tectonic network

806d30b

This will allow users to reboot their host and have the cluster come back up automatically.

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2018

smarterclayton and others added 5 commits August 23, 2018 22:36

Move tectonic-ingress to openshift-ingress

0f0ffee

Makes installer consistent with 4.0 direction.

*: remove dns steps

d8fa662

Now that all of the records are created upfront in the topology step and the bootstrap node uses a static location for its config (no more CNAMEs), there is no need to explicitly manipulate the records later.

*: move etcd members to the master nodes

506ca08

This puts the etcd members on the master nodes instead of the dedicated etcd nodes.

installer: report error on an unrecognized pool

53e58ce

Instead of failing with an extremely opaque error message, this will report which pool was unrecognized. Users are likely to hit this now that etcd has been removed.

*: remove etcd nodes

6317e0e

Now that the etcd members have been moved to the master nodes, there is no need for the etcd nodes. The installer still understands the etcd config even though it isn't used. This is to prevent the tests from breaking. This code will be removed in the near future.

crawford added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2018

openshift-merge-robot merged commit baedda4 into openshift:master Aug 24, 2018

crawford deleted the bootstrap branch August 24, 2018 06:07

crawford mentioned this pull request Aug 24, 2018

WIP - Move tectonic-ingress to openshift-ingress #149

Closed

This was referenced Aug 24, 2018

tests/smoke/BUILD.bazel: Drop the 'smoke' target #143

Merged

steps/bootstrap/libvirt: add pty to bootstrap node #176

Merged

wking mentioned this pull request Aug 27, 2018

*: Use the same ignition for all masters #187

Closed

wking mentioned this pull request Sep 19, 2018

config/validator/s3bucket: remove TNC references #285

Merged

*: implement new bootstrap process #162

*: implement new bootstrap process #162

Uh oh!

Conversation

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

Uh oh!

Uh oh!

abhinavdahiya commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 left a comment

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!

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!

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!

crawford commented Aug 23, 2018

Uh oh!

crawford commented Aug 23, 2018

Uh oh!

abhinavdahiya commented Aug 24, 2018

Uh oh!

yifan-gu commented Aug 24, 2018

Uh oh!

yifan-gu commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 24, 2018

crawford commented Aug 22, 2018 •

edited

Loading

abhinavdahiya Aug 23, 2018 •

edited

Loading

yifan-gu commented Aug 24, 2018 •

edited

Loading

abhinavdahiya commented Aug 24, 2018 •

edited

Loading