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

Update UPI vSphere terraform code for v0.12 #2057

Closed
wants to merge 1 commit into from

Conversation

DanyC97
Copy link
Contributor

@DanyC97 DanyC97 commented Jul 22, 2019

This PR updates the UPI vSphere terraform templates to v0.12.

Ref #1936 (comment)

@abhinavdahiya @staebler @vrutkovs please help review it.

@openshift-ci-robot
Copy link
Contributor

Hi @DanyC97. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 22, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found 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


5. Ensure that you have you AWS profile set and a region specified. The installation will use create AWS route53 resources for routing to the OpenShift cluster.
Copy link
Member

Choose a reason for hiding this comment

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

Route53 is still being used for DNS, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is correct @vrutkovs is still used hence why i've moved it up in the pre-req section so folks know upfront what is required from the existing code base.

I can extend if y'all okay with to mention why R53 provider is used and not DNS provider, let me know

@vrutkovs
Copy link
Member

/test e2e-vsphere

@vrutkovs
Copy link
Member

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 22, 2019
@DanyC97
Copy link
Contributor Author

DanyC97 commented Jul 22, 2019

job failed due to

govc: Post https://vcsa-ci.vmware.devcluster.openshift.com/sdk: dial tcp 139.178.73.3:443: i/o timeout
2019/07/22 15:21:01 Container setup in pod e2e-vsphere failed, exit code 1, reason Error

sounds like you not able to import the template into your vCenter env

@abhinavdahiya
Copy link
Contributor

/hold

we are close (almost in) feature freeze, cannot make these changes until there's an exception.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2019
@DanyC97
Copy link
Contributor Author

DanyC97 commented Jul 22, 2019

/hold

we are close (almost in) feature freeze, cannot make these changes until there's an exception.

no problem at all, totally understand ! let me know when you are out the freeze mode, happy to carry on from there

path = "${var.cluster_id}"
datacenter_id = "${data.vsphere_datacenter.dc.id}"
//path = "${var.cluster_id}"
path = "Dani/${var.vm_folder}"

Choose a reason for hiding this comment

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

Is 'Dani/' prepended to the folder path why the tests fail? You probably don't want this included in the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @fherbert , fixed

}

resource "aws_route53_record" "control_plane_nodes" {
count = "${var.control_plane_count}"
count = var.control_plane_count}

Choose a reason for hiding this comment

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

need to remove this trailing '}'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @fherbert , fixed

@DanyC97
Copy link
Contributor Author

DanyC97 commented Sep 24, 2019

@abhinavdahiya i saw you started to work on #2396 , can i assume we can re-activate this PR ?

@DanyC97
Copy link
Contributor Author

DanyC97 commented Oct 19, 2019

@abhinavdahiya gentle reminder, please let me know if you are still interested in this PR now that 4.2 is out.

If not then please close this PR.

@mkelnermishal
Copy link

Hey,
Is there any update on this code ? we need it in order to release 4.2 to our customers (VMware).
Thanks.

@DanyC97
Copy link
Contributor Author

DanyC97 commented Nov 11, 2019

Hey,
Is there any update on this code ? we need it in order to release 4.2 to our customers (VMware).
Thanks.

@mkelnermishal happy to continue the work if @abhinavdahiya confirm he is ready/ happy to accept the PR, haven't received any feedback on my previous attempts to get an answer hence my assumption is that is not needed

@abhinavdahiya
Copy link
Contributor

/test e2e-vsphere

@openshift-ci-robot
Copy link
Contributor

@DanyC97: PR needs rebase.

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 openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2019
@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Nov 22, 2019

Thanks for the changes! And I think it's important that we move to 0.12 terraform..

So here's the pickle, the baremetal using packet.net here also uses the terraform..

and the image we use to test both is 0.11

ENV TERRAFORM_VERSION=0.11.11

so we will have to either make sure:
A) update the CI image used for terraform to 0.12, make sure baremetal has terraform version set to <=0.11 i think that should allow 0.12 terraform binary to run in 0.11 mode and then we can merge this change.

B) we update all the changes in one PR.

@DanyC97
Copy link
Contributor Author

DanyC97 commented Nov 23, 2019

A) update the CI image used for terraform to 0.12, make sure baremetal has terraform version set to <=0.11 i think that should allow 0.12 terraform binary to run in 0.11 mode and then we can merge this change.

B) we update all the changes in one PR.

@abhinavdahiya thank you for taking the time to respond and refocus attention on this PR. I'm easy going with either of the 2 suggestions.

What does the baremetal team prefer to go with?

@DanyC97
Copy link
Contributor Author

DanyC97 commented Nov 23, 2019

maybe merge the effort in one PR with #2479 or chain them one after another or just be aware of it ;)

@abhinavdahiya
Copy link
Contributor

A) update the CI image used for terraform to 0.12, make sure baremetal has terraform version set to <=0.11 i think that should allow 0.12 terraform binary to run in 0.11 mode and then we can merge this change.
B) we update all the changes in one PR.

@abhinavdahiya thank you for taking the time to respond and refocus attention on this PR. I'm easy going with either of the 2 suggestions.

What does the baremetal team prefer to go with?

I would prefer the option B. if you can help migrate the metal work with vsphere work to 0.12. I can help review and get it merged...

@openshift-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 f3b6f8c link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-aws-disruptive f3b6f8c link /test e2e-aws-disruptive
ci/prow/e2e-vsphere f3b6f8c link /test e2e-vsphere
ci/prow/tf-lint f3b6f8c link /test tf-lint
ci/prow/shellcheck f3b6f8c link /test shellcheck
ci/prow/yaml-lint f3b6f8c 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.

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.

@DanyC97
Copy link
Contributor Author

DanyC97 commented Apr 13, 2020

closing in favour of #3235 and #3429

@DanyC97 DanyC97 closed this Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. version/4.5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants