Fix cluster version upgrades #577

danawillow · 2017-10-12T01:16:16Z

This PR does two things (each in a separate commit):

Adds a retry when reading the cluster to make sure it's in the RUNNING state. This (sort of) solves the issue of trying to upgrade a cluster while it's already upgrading. Though it would make sense to have the retry be on the upgrade, I don't want users to get confused when they run plan, see a certain version, and then run upgrade and find out they were at a different version. Let me know your thoughts on whether this should actually be in read or in update (or in both).
Adds a new field, min_master_version and sets master_version to computed. As per discussions with @rosbo and @paddycarver, having just one master_version field doesn't work when the master gets upgraded automatically. This allows the users to set the minimum master version while also using the current master version for interpolation in other resources.

@paddycarver is in charge of running tests and adding documentation because I have to go catch my bus :)

paddycarver · 2017-10-12T05:06:06Z

I'm seeing this still fail the updateVersion test. I'm going to look into why/what it takes to fix it.

paddycarver · 2017-10-12T05:12:05Z

This appears to be happening because get-server-config is returning 1.7.6-gke.1 as the most recent available version for nodes, but then claiming it's not a valid version?

paddycarver · 2017-10-12T05:53:46Z

Switching from the latest and 2nd-latest master/node versions in the updateVersion to the 2nd-latest and 3rd-latest resolves the issue, which makes me suspect it really is a problem with the (mistakenly?) reported latest node version. @danawillow do we want to:

merge this as-is and release even though the test fails?
wait for the API to be fixed?
update the test to use the 2nd/3rd latest?

I'm updating the docs for this now, but I don't think I can push to the branch this is on. I'll push to the paddy_577 branch on this repo.

paddycarver · 2017-10-12T05:59:18Z

google/resource_container_cluster.go

-	if d.HasChange("master_version") {
-		desiredMasterVersion := d.Get("master_version").(string)
+	if d.HasChange("min_master_version") {
+		desiredMasterVersion := d.Get("min_master_version").(string)


This doesn't take into consideration if the current master_version is already greater than min_master_version. I think we want to ignore the version in that case, instead of downgrading?

Good point, fixed.

paddycarver · 2017-10-12T06:04:14Z

Sorry for the lots-of-comments on this one. :(

Let me know your thoughts on whether this should actually be in read or in update (or in both).

I'm in favor of doing it in read. That also ensures that other things that may depend on the cluster being in a RUNNING state also get the benefit of this.

rosbo · 2017-10-12T16:26:15Z

google/resource_container_cluster.go

-		return fmt.Errorf("cannot set node_version on create, use master_version instead")
+	if v, ok := d.GetOk("node_version"); ok {
+		// ignore -gke.X suffix for now. if it becomes a problem later, we can fix it.
+		mv := strings.Split(cluster.InitialClusterVersion, "-")[0]


If min_master_version is not specified, then cluster.InitialClusterVersion will be empty right? This means mv will be an empty string. Is that what we want? Does that mean that if we don't set min_master_version, we should also leave node_version empty?

That's what @paddycarver and I ended up deciding on yesterday but now I can't remember for sure why. I went and changed it because it seems reasonable to just specify the node_version (since that's what the nodes will be set to anyway) and let GKE handle the master.

danawillow · 2017-10-12T16:56:21Z

@paddycarver yeah, that's the bug I was talking about where I spoke to the person and it'll be fixed soon. In the meantime I just went ahead and changed it to the 2nd/third latest.

danawillow · 2017-10-12T17:23:02Z

Added docs based on @paddycarver's commit- I changed the wording around a little but it's the same basic idea.

$ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_basic -timeout 120m
=== RUN   TestAccContainerCluster_basic
--- PASS: TestAccContainerCluster_basic (366.49s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	366.677s

$ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_withVersion'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_withVersion -timeout 120m
=== RUN   TestAccContainerCluster_withVersion
--- PASS: TestAccContainerCluster_withVersion (367.59s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	367.756s

$ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_updateVersion'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_updateVersion -timeout 120m
=== RUN   TestAccContainerCluster_updateVersion
--- PASS: TestAccContainerCluster_updateVersion (690.49s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	690.673s

danawillow · 2017-10-12T17:30:17Z

Ok, I remembered why we weren't going to allow setting the node_version on create. InitialNodeVersion only accepts valid master versions. Changed it back.

paddycarver

LGTM

* wait for running status on a cluster on read * add min_master_version field * respond to comments * add docs * no node_version on create

Signed-off-by: Modular Magician <[email protected]>

ghost · 2020-03-30T14:29:35Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

danawillow added 2 commits October 11, 2017 17:09

wait for running status on a cluster on read

cce4ab1

add min_master_version field

cc7cc83

danawillow requested review from rosbo and paddycarver October 12, 2017 01:16

paddycarver reviewed Oct 12, 2017

View reviewed changes

paddycarver added a commit that referenced this pull request Oct 12, 2017

Add docs for #577.

5c26039

rosbo reviewed Oct 12, 2017

View reviewed changes

respond to comments

44abbf5

rosbo approved these changes Oct 12, 2017

View reviewed changes

add docs

f85ddea

no node_version on create

3cad14a

paddycarver approved these changes Oct 12, 2017

View reviewed changes

danawillow merged commit 811530f into hashicorp:master Oct 12, 2017

danawillow deleted the upgrade branch October 12, 2017 18:21

negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017

Fix cluster version upgrades (hashicorp#577)

311be7e

* wait for running status on a cluster on read * add min_master_version field * respond to comments * add docs * no node_version on create

negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017

Fix cluster version upgrades (hashicorp#577)

c3aa554

* wait for running status on a cluster on read * add min_master_version field * respond to comments * add docs * no node_version on create

danawillow mentioned this pull request Oct 30, 2018

GKE: min_master_version is empty #2348

Closed

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

Signed CDN urls for backend services (hashicorp#577)

c40f1f2

Signed-off-by: Modular Magician <[email protected]>

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cluster version upgrades #577

Fix cluster version upgrades #577

danawillow commented Oct 12, 2017

paddycarver commented Oct 12, 2017

paddycarver commented Oct 12, 2017

paddycarver commented Oct 12, 2017

paddycarver Oct 12, 2017

danawillow Oct 12, 2017

paddycarver commented Oct 12, 2017

rosbo Oct 12, 2017

danawillow Oct 12, 2017

danawillow commented Oct 12, 2017

danawillow commented Oct 12, 2017

danawillow commented Oct 12, 2017

paddycarver left a comment

ghost commented Mar 30, 2020

Fix cluster version upgrades #577

Fix cluster version upgrades #577

Conversation

danawillow commented Oct 12, 2017

paddycarver commented Oct 12, 2017

paddycarver commented Oct 12, 2017

paddycarver commented Oct 12, 2017

paddycarver Oct 12, 2017

Choose a reason for hiding this comment

danawillow Oct 12, 2017

Choose a reason for hiding this comment

paddycarver commented Oct 12, 2017

rosbo Oct 12, 2017

Choose a reason for hiding this comment

danawillow Oct 12, 2017

Choose a reason for hiding this comment

danawillow commented Oct 12, 2017

danawillow commented Oct 12, 2017

danawillow commented Oct 12, 2017

paddycarver left a comment

Choose a reason for hiding this comment

ghost commented Mar 30, 2020