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

gather: update the state handling for terraform 0.12 #1763

Merged
merged 3 commits into from
May 28, 2019

Conversation

abhinavdahiya
Copy link
Contributor

  • pkg/terraform: utilize the vendored terraform to read tfstate

    Utilizing the vendored statefile serialize/deserialize helps installer to no worry about the statefile
    version conversions. Installer can always assume to use the latest/chosen version of state file from vendored code.

    For example, terraform 0.12 moves the terraform state to version 4, compared to version 3. With this change installer can assume the terraform state
    deserialized for use will always be version 4, as it will be upconverted by vendored terraform statefile package.

  • pkg/terraform: add gather package to store platform specific code from terraform

    This adds bootstrap and control plane hosts ip gather from state file for platforms.

  • cmd/openshift-install: update gather code to utilize new terraform packages

/cc @wking @jstuever

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 20, 2019
@jstuever
Copy link
Contributor

Looks solid to me. I'll defer lgtm to Trevor.

@jstuever
Copy link
Contributor

/test e2e-aws-scaleup-rhel7

Utilizing the vendored statefile serialize/deserialize helps installer to no worry about the statefile
version conversions. Installer can always assume to use the latest/chosen version of state file from vendored code.

For example, terraform 0.12 moves the terraform state to version 4, compared to version 3. With this change installer can assume the terraform state
deserialized for use will always be version 4, as it will be upconverted by vendored terraform statefile package.
…m terraform

This adds bootstrap and control plane hosts ip gather from state file for platforms.
@abhinavdahiya
Copy link
Contributor Author

friendly ping @jstuever and @wking

@trown
Copy link

trown commented May 22, 2019

/test e2e-openstack

@abhinavdahiya
Copy link
Contributor Author

/retest

@wking
Copy link
Member

wking commented May 24, 2019

For example, terraform 0.12 moves the terraform state to version 4, compared to version 3. With this change installer can assume the terraform state deserialized for use will always be version 4, as it will be upconverted by vendored terraform statefile package.

Do we need this? Running two different installer versions on the same Terraform state file seems like a bigger problem than just "what version is the state file?". For example, maybe we've shuffled some modules around and whatever you were trying to access (the bootstrap IP address?) is now in a different place?

@abhinavdahiya
Copy link
Contributor Author

For example, terraform 0.12 moves the terraform state to version 4, compared to version 3. With this change installer can assume the terraform state deserialized for use will always be version 4, as it will be upconverted by vendored terraform statefile package.

Do we need this? Running two different installer versions on the same Terraform state file seems like a bigger problem than just "what version is the state file?".

Yes, we need to try our best to not break people. Keep things backward compatible. This makes sure we can at-least read the terraform state file.

For example, maybe we've shuffled some modules around and whatever you were trying to access (the bootstrap IP address?) is now in a different place?

In that case, we'll need to make sure we makes changes to gather to understand old and new locations. If we can keep backward compatible we should.

@wking
Copy link
Member

wking commented May 24, 2019

Yes, we need to try our best to not break people. Keep things backward compatible. This makes sure we can at-least read the terraform state file.

But why would you want to read the Terraform state file written by a different installer? The only story I can think of is:

  1. User uses installer-A attempts to create a cluster and fails in Terraform.
  2. User uses installer-B to openshift-install gather bootstrap.

But that's a pretty short window in which to be switching installers. And other actions like changing installers between an openshift-install create manifests and an openshift-install create cluster seem even riskier. I'd rather have the only compat contract between different installer versions reading eachother's assets be install-config.yaml (multi-cluster lifespan) and metadata.json (full cluster lifespan). The other assets, and especially the Terraform state, seem like "cluster-install lifespan" where requiring a consistent installer version is not a hardship.

@abhinavdahiya
Copy link
Contributor Author

Yes, we need to try our best to not break people. Keep things backward compatible. This makes sure we can at-least read the terraform state file.

But why would you want to read the Terraform state file written by a different installer? The only story I can think of is:

  1. User uses installer-A attempts to create a cluster and fails in Terraform.
  2. User uses installer-B to openshift-install gather bootstrap.

But that's a pretty short window in which to be switching installers. And other actions like changing installers between an openshift-install create manifests and an openshift-install create cluster seem even riskier. I'd rather have the only compat contract between different installer versions reading eachother's assets be install-config.yaml (multi-cluster lifespan) and metadata.json (full cluster lifespan). The other assets, and especially the Terraform state, seem like "cluster-install lifespan" where requiring a consistent installer version is not a hardship.

I'm not sure why the point of contention is depending on terraform code itself to read the state file.
The reasoning is it's one less thing Installer needs to think about in terms of maintaining, Installer shouldn't need to care what version was written at, it cares I know and expect it to look like this.

@jstuever
Copy link
Contributor

/test e2e-aws-scaleup-rhel7

@jstuever
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, jstuever

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

The pull request process is described 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

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit cc44c74 into openshift:master May 28, 2019
@openshift-ci-robot
Copy link
Contributor

@abhinavdahiya: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 3c89497 link /test e2e-aws-scaleup-rhel7

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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.

8 participants