[CC-30640] allow manual version control for standard clusters #252

fantapop · 2024-12-03T02:43:20Z

STANDARD clusters now accept the cockroach_version attribute on cluster update for manual upgrades control. The serverless.upgrade_type attribute must be set to MANUAL.

As part of this an API call to validate cockraoch_version for ADVANCED clusters prior to the update was removed. This logic provided some nicer error messages for advanced clusters but the logic was complex, it required an additional api call to fetch the valid versions and standard tier version checking is not supported. We now rely on the serverside validation for checking versions.

Commit checklist

internal/provider/cluster_resource_test.go

internal/provider/cluster_resource.go

fantapop · 2024-12-03T02:54:00Z

internal/provider/cluster_resource_test.go

@@ -975,6 +982,17 @@ func TestIntegrationServerlessClusterResource(t *testing.T) {
 			},
 			finalCluster: singleRegionClusterWithUnlimited("BASIC"),
 		},
+		{
+			name: "upgrade cockroach_version for STANDARD cluster",


This is the only real automated test for a successful standard tier upgrade. Our api doesn't allow setting up these tests since we need to be able to build a host cluster and then upgrade it. For that reason, we're currently limited to integration tests here. Given the complication of upgrades it would be great to figure out a way to build an acceptance test for this. @andy-kimball Do you have any ideas for that?

@alyshanjahani-crl, do we support creating a Serverless cluster at the previous version? If so, we could do that, and then upgrade it to the next version, all without touching the host cluster version.

We're currently limited creating serverless clusters at the latest version. This is why cockroach_version is not accepted for serverless clusters during the Create phase. But some kind of ability in the future would be welcome. We could hide it behind a feature flag if need be.

andy-kimball · 2024-12-03T03:46:18Z

internal/provider/cluster_resource.go

@@ -439,6 +438,23 @@ func (r *clusterResource) Create(
 			}
 		}

+		if IsKnown(plan.CockroachVersion) {
+			var planType client.PlanType
+			if clusterSpec.Plan != nil {


This plan type derivation logic seems like something that should be a utility function.

Okay, I've moved it out to its own function and added a test for it.

andy-kimball · 2024-12-03T03:51:46Z

internal/provider/cluster_resource.go

-					break
+		if state.ServerlessConfig != nil {
+			stateUpgradeTypeString := state.ServerlessConfig.UpgradeType.ValueString()
+			planUpgradeTypeString := plan.ServerlessConfig.UpgradeType.ValueString()


Is it guaranteed that plan.ServerlessConfig will be non-nil here?

I didn't know this so I wrote a test to try it out. Apparently we're protected against this here. ModifyPlan is run before the update should it should never be the case that state.ServerlessConfig is set but plan.ServerlessConfig is not. I went ahead and left the test in.

andy-kimball · 2024-12-03T03:52:39Z

internal/provider/cluster_resource.go

+			if stateUpgradeTypeString != planUpgradeTypeString {
+				resp.Diagnostics.AddError("Error updating cluster", "upgrade_type and version must not be changed during the same plan.")
+				return
+			} else {


NIT: I'd just get rid of the else since you're already returning from the if.

andy-kimball · 2024-12-03T03:56:40Z

internal/provider/cluster_resource_test.go

@@ -975,6 +982,17 @@ func TestIntegrationServerlessClusterResource(t *testing.T) {
 			},
 			finalCluster: singleRegionClusterWithUnlimited("BASIC"),
 		},
+		{
+			name: "upgrade cockroach_version for STANDARD cluster",


@alyshanjahani-crl, do we support creating a Serverless cluster at the previous version? If so, we could do that, and then upgrade it to the next version, all without touching the host cluster version.

internal/provider/cluster_resource_test.go

kathancox

LGTM! One tiny nit, and a small comment/question.

internal/provider/cluster_resource.go

kathancox · 2024-12-03T16:07:12Z

internal/provider/cluster_resource.go

+			} else {
+				stateUpgradeTypePtr, err := client.NewUpgradeTypeTypeFromValue(stateUpgradeTypeString)
+				if err != nil {
+					resp.Diagnostics.AddError("Invalid value for upgrade_type found in state", err.Error())


Suggested change

resp.Diagnostics.AddError("Invalid value for upgrade_type found in state", err.Error())

resp.Diagnostics.AddError("Invalid value for upgrade_type found in state.", err.Error())

looking closer at this, I noticed this is the "error summary" field. We generally don't add periods for the summary. But it does seem like this summary is probably too specific. Here is what would be returned by err.Error():

"invalid value 'BAD_VALUE' for UpgradeTypeType: valid values are ['AUTOMATIC', 'MANUAL']

So the error would should up as:

"Invalid value for upgrade_type found in state"
"invalid value 'BAD_VALUE' for UpgradeTypeType: valid values are ['AUTOMATIC', 'MANUAL']

This is repetitive. I'll update the summary to be "Error updating cluster" like the rest of the errors in this file. It would be nice to eventually do an audit of the full provider and make a strategy for what to use for the summaries.

Great thanks!

kathancox · 2024-12-03T16:16:17Z

internal/provider/cluster_resource.go

+
+		if client.PlanType(state.Plan.ValueString()) == client.PLANTYPE_BASIC {
+			if client.PlanType(plan.Plan.ValueString()) != client.PLANTYPE_BASIC {
+				resp.Diagnostics.AddError("Error updating cluster", "plan and version must not be changed during the same plan.")


Two of the error messages say "... during the same plan." Does this mean ... must not be changed when plan` remains the same."? This was the only thing that stood out to me, I may not have the context here, and "during the same plan" is fine!

The plan I'm referring to is basically a batch of updates. Not to be confused with the cluster plan (i.e. basic, standard, advanced).

With terraform you make changes to your configuration, then you can run a "plan" in which terraform will spit out what the changes will be. Or you can run an "apply", which includes a "plan" but then asks if the plan looks good before proceeding with the actually update.

If this is a point of confusion I'm happy to change it. We could say something like "must not be changed at the same time" or "... at once" or, "please apply changes to the upgrade_type before upgrading", or many other things...

I forgot about the plan command and the concept generally within Terraform. I think must not be changed at the same time, or maybe the original is fine. I kind of like plan and version must not be changed during the same terraform plan. But maybe that is too heavy handed. I'll leave it to you.

STANDARD clusters now accept the cockroach_version attribute on cluster update for manual upgrades control. The serverless.upgrade_type attribute must be set to MANUAL. As part of this an API call to validate cockraoch_version for ADVANCED clusters prior to the update was removed. This logic provided some nicer error messages for advanced clusters but the logic was complex, it required an additional api call to fetch the valid versions and standard tier version checking is not supported. We now rely on the serverside validation for checking versions.

To clean up some of the testing, we add a method for building the serverless test configuration which accepts a struct for providing the values.

rgcase

LGTM

internal/provider/cluster_resource.go

fantapop requested review from rgcase, andy-kimball and kathancox December 3, 2024 02:43

fantapop commented Dec 3, 2024

View reviewed changes

internal/provider/cluster_resource_test.go Outdated Show resolved Hide resolved

fantapop commented Dec 3, 2024

View reviewed changes

internal/provider/cluster_resource.go Show resolved Hide resolved

fantapop commented Dec 3, 2024

View reviewed changes

internal/provider/cluster_resource.go Show resolved Hide resolved

fantapop commented Dec 3, 2024

View reviewed changes

andy-kimball reviewed Dec 3, 2024

View reviewed changes

kathancox approved these changes Dec 3, 2024

View reviewed changes

fantapop force-pushed the CC-30640-allow-control-of-cluster-version-for-standard-clusters-in-terraform branch 2 times, most recently from 0b15bb6 to 4914afc Compare December 5, 2024 01:34

fantapop requested a review from andy-kimball December 5, 2024 01:42

fantapop added 3 commits December 5, 2024 08:53

factor out method for building serverless test steps

922139a

To clean up some of the testing, we add a method for building the serverless test configuration which accepts a struct for providing the values.

Add test showing swap from advanced to standard not allowed

55d2b94

fantapop force-pushed the CC-30640-allow-control-of-cluster-version-for-standard-clusters-in-terraform branch from d54a3d0 to 55d2b94 Compare December 5, 2024 16:53

rgcase approved these changes Dec 5, 2024

View reviewed changes

internal/provider/cluster_resource.go Show resolved Hide resolved

fantapop merged commit ba8bd3c into main Dec 5, 2024
4 checks passed

fantapop deleted the CC-30640-allow-control-of-cluster-version-for-standard-clusters-in-terraform branch December 5, 2024 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CC-30640] allow manual version control for standard clusters #252

[CC-30640] allow manual version control for standard clusters #252

fantapop commented Dec 3, 2024 •

edited

Loading

fantapop Dec 3, 2024

andy-kimball Dec 3, 2024

fantapop Dec 4, 2024

andy-kimball Dec 3, 2024

fantapop Dec 5, 2024

andy-kimball Dec 3, 2024

fantapop Dec 5, 2024 •

edited

Loading

andy-kimball Dec 3, 2024

andy-kimball Dec 3, 2024

kathancox left a comment

kathancox Dec 3, 2024

fantapop Dec 4, 2024

kathancox Dec 5, 2024

kathancox Dec 3, 2024

fantapop Dec 4, 2024

kathancox Dec 5, 2024

rgcase left a comment

	resp.Diagnostics.AddError("Invalid value for upgrade_type found in state", err.Error())
	resp.Diagnostics.AddError("Invalid value for upgrade_type found in state.", err.Error())

[CC-30640] allow manual version control for standard clusters #252

[CC-30640] allow manual version control for standard clusters #252

Conversation

fantapop commented Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fantapop Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathancox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgcase left a comment

Choose a reason for hiding this comment

fantapop commented Dec 3, 2024 •

edited

Loading

fantapop Dec 5, 2024 •

edited

Loading