OpenShift on VMware Cloud on AWS

[jcpowermac]

Reuses existing AWS IPI terraform and vSphere UPI
terraform to support VMware Cloud on AWS which
will be the replacement of Packet for CI.

[jcpowermac]

This most certainly will not pass CI. Would like to start getting review while I am still testing and modifying.
Needs documentation updates as well.

[DanyC97]

@jcpowermac just a heads up in case you haven't seen #2057 which i was trying to get it in for a long time but it was put on hold by the installer team lead.

While i understand from your work that you are focusing on extending the work done initially to run the whole VMware deployment in AWS, i do worry that you are changing the scope which will affect folks who were using this upi sample to bootstrap OCP on a standalone VMware env.

If the installer team desire is to have this upi examples used for CI only then may i suggest you add a bold note so everyone knows that they are not consumable outside RH CI ?

[jcpowermac]

@jcpowermac just a heads up in case you haven't seen #2057 which i was trying to get it in for a long time but it was put on hold by the installer team lead.

While i understand from your work that you are focusing on extending the work done initially to run the whole VMware deployment in AWS, i do worry that you are changing the scope which will affect folks who were using this upi sample to bootstrap OCP on a standalone VMware env.

If the installer team desire is to have this upi examples used for CI only then may i suggest you add a bold note so everyone knows that they are not consumable outside RH CI ?

Hi @DanyC97 if you have any suggestions I am certainly open to it. Though atm my priority is to have CI green which is why we are moving to VMC. I teased apart the IPAM and Route53 terraform from the virtual machine creation so that someone could actually use it without requiring either. That is how I initially tested VMC: ran the AWS terraform that created DNS and LB then ran the terraform that created the virtual machines. Theoretically as long as you had DNS configured correctly with whatever you use it should work.

[patrickdillon]

Left a few comments, granted that I don't have background in vsphere. have not had a chance to look at the changes in machine, which it looks like are substantial

[patrickdillon]

Are these variables private- and public_subnet_ids used? I can't find subsequent references to them. Where do aws_subnet variables come from?

[patrickdillon]

are these cidr ranges used? I can't find subsequent references to them but perhaps I am missing something.

[patrickdillon]

nit: new_vpc makes me think we are creating a vpc, when we are reading it in. cluster?

[patrickdillon]

remove

[patrickdillon]

remove

[patrickdillon]

nit: I find it curious to have a hard coded value as a module output

[jcpowermac]

@patrickdillon /shrug I took the AWS terraform from aws IPI and modified it for VMC. Even in 4.3 its a hard coded output:

installer/data/data/aws/vpc/outputs.tf

Line 48 in 3b221a9

value = "3"

[patrickdillon]

is this being used?

[patrickdillon]

is this being used?

[wking]

nit: "terraform" -> "Terraform", here and later.

[wking]

nit: openshift-installer -> openshift-install

[wking]

nit: "ignition" -> "Ignition", here and later.

[wking]

nit: We can probably drop "for OpenShift RHCOS virtual machines" while remaining sufficiently specific. For comparison, AWS and GCP docs have "Create Ignition configs" (they disagree on Configs vs. configs, I don't think we have a clear title vs. sentence case policy for h2+ ;).

[wking]

nit: no need for the leading ./ or trailing /. Not that it matters much, for folks copy/pasting.

[wking]

nit: brace expansion is a Bashism. We'd be portable across all POSIX shells with:

cp install-config.yaml.example install-config.yaml

[wking]

nit: doesn't really add much in the way of syntax highlighting in most cases, but it's nice to explicitly declare this to be a shell block with ```sh.

[wking]

nit: no need for this leading document separator, and most of our other YAML examples omit it.

[wking]

nit: did we want to fill these in with placeholders/examples like these examples?

[wking]

nit: maybe rephrase to:

This is an internal Terraform module for installing OpenShift in a VMware Cloud on AWS.

That makes implicitly not for other uses, so we don't have to talk about how not-useful it is going to be for those off-label uses ;)

[wking]

Do you want to address this before merging? I don't see a problem with a single zone for Terraform modules we use for internal testing. Folks who want a vSphere cluster are probably not going to be using AWS, because if you want a cluster on AWS you'd just use our native AWS providers, right?

[wking]

you can drop this if you go with a single zone.

[wking]

I know the AWS Terraform uses master/worker, but if you already have control-plane/compute in the vSphere Terraform I'd rather stay with it instead of backsliding. See also #1330.

[wking]

nit: backticks around paths and other literals: terraform.tfvars.

[wking]

nit: trailing empty line.

[jcpowermac]

Thanks @wking and @patrickdillon for taking the time to review. I am in process of cleaning it up. Will need to retest.

[jcpowermac]

@wking and @patrickdillon I think I addressed all the concerns. Though I wonder if we shouldn't move to terraform 0.12? From the template it looks like only vSphere is using terraform. The only other UPI that is still terraform 0.11 is Metal but it doesn't look like that template is used with it.

[jcpowermac]

/hold
Started having issues with injecting ignition into extra config.

[openshift-ci-robot]

[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:

• OWNERS

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

[jstuever]

/uncc @jstuever

[openshift-ci-robot]

@jcpowermac: PR needs rebase.

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.

[openshift-ci-robot]

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-upgrade	9abafe8	link	/test e2e-aws-upgrade
ci/prow/e2e-aws-fips	9abafe8	link	/test e2e-aws-fips
ci/prow/tf-lint	9abafe8	link	/test tf-lint
ci/prow/shellcheck	9abafe8	link	/test shellcheck
ci/prow/yaml-lint	9abafe8	link	/test yaml-lint

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.

[jcpowermac]

/close

[openshift-ci-robot]

@jcpowermac: Closed this PR.

Details

In response to this:

/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.