use the allowed upgrades list when validating version upgrades #240
Conversation
dcrosta
force-pushed
the
support-skippable-innovation-releases
branch
2 times, most recently
from
f7cd25d
to
713fe76
Compare
dcrosta
force-pushed
the
support-skippable-innovation-releases
branch
2 times, most recently
from
85fb7c5
to
1ffb909
Compare
| resp.Diagnostics.AddError("Invalid CockroachDB version", | ||
| fmt.Sprintf( | ||
| "Cannot roll back to to '%s'.", | ||
| planVersion)) | ||
| return |
There was a problem hiding this comment.
I was going to ask if we can list the valid rollback version here but... do we actually not know it at this point? If we let this pass through, it seems like we can give a better error. WDYT?
There was a problem hiding this comment.
We don't include the rollback version in the CC API clusters response. We could, there's probably no harm in doing so. Do you think that's valuable to add?
There was a problem hiding this comment.
It feels more like noise to me. I'd rather not add it to the api response unless we have a more compelling use case. I was suggesting here that we could let the api call happen here which would let the ccapi validate the version and return the better error message. But we could also merge this as is. It's a slight improvement on an unlikely case.
There was a problem hiding this comment.
Without all this error handling, we get:
cockroach_cluster.example: Modifying... [id=2dc9a589-1f20-412c-952b-522d5184a3f3]
cockroach_cluster.example: Still modifying... [id=2dc9a589-1f20-412c-952b-522d5184a3f3, 10s elapsed]
cockroach_cluster.example: Still modifying... [id=2dc9a589-1f20-412c-952b-522d5184a3f3, 20s elapsed]
╷
│ Error: Error updating cluster
│
│ with cockroach_cluster.example,
│ on main.tf line 49, in resource "cockroach_cluster" "example":
│ 49: resource "cockroach_cluster" "example" {
│
│ cluster can only be rolled back to the previous version, expected: v24.1
I don't know why this took 20+ seconds to get this? But it seems like a reasonable error, so I'll remove the checks in the tf provider
There was a problem hiding this comment.
Here is the request: https://us5.datadoghq.com/apm/trace/8538d437c1868d2218b1a60d611750c1?graphType=flamegraph&shouldShowLegend=true&sort=time&spanID=1655416649021547938&timeHint=1728402657004.241
350ms for the actual api request. I think it probably took 20s because of all the other requests that happen during a tf run. It looks like there are about 20 requests to the same cluster endpoint. API caching via terraform isn't a good story currently.
Beginning with v24.2, some releases are innovation releases which can be skipped during upgrading. The ListClusterMajorVersions API returns information on allowed upgrade paths, which we now use to validate whether an upgrade is possible in `apply`. Additional validation (cluster state, etc) is performed server-side within the API, so this validation is mostly to catch the most common type of error with a faster, more helpful error message, and not to be exhaustive.
dcrosta
force-pushed
the
support-skippable-innovation-releases
branch
from
afb2c01
to
0e694e0
Compare
Beginning with v24.2, some releases are innovation releases which can be skipped during upgrading. The ListClusterMajorVersions API returns information on allowed upgrade paths, which we now use to validate whether an upgrade is possible in
apply. Additional validation (cluster state, etc) is performed server-side within the API, so this validation is mostly to catch the most common type of error with a faster, more helpful error message, and not to be exhaustive.Commit checklist
make generate)