-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix an issue with resuming a failed container cluster creation #7121
Conversation
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 3 files changed, 226 insertions(+), 3 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_failedCreation |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
/gcbrun |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 3 files changed, 226 insertions(+), 3 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_failedCreation|TestAccRegionInstanceGroupManager_stateful |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
reassigning to a random team member. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this also fix hashicorp/terraform-provider-google#11431?
I had a few questions but overall solution looks good.
@@ -2247,11 +2249,43 @@ func resourceContainerClusterRead(d *schema.ResourceData, meta interface{}) erro | |||
} | |||
waitErr := containerOperationWait(config, op, project, location, "resuming GKE cluster", userAgent, d.Timeout(schema.TimeoutRead)) | |||
if waitErr != nil { | |||
// Check if the create operation failed because Terraform was prematurely terminated. If it was we can persist the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For anyone else reading, it looks like this block is an exact copy from what is done in the *Create
function above.
select { | ||
case <-config.context.Done(): | ||
log.Printf("[DEBUG] Persisting %s so this operation can be resumed \n", op.Name) | ||
if err := d.Set("operation", op.Name); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this result in an infinite loop situation? It seems like a user might be able to get stuck in some situation where the operation never terminates properly and they can't get a terraform plan
to succeed. IMO, one retry probably makes sense, and I think that's why the operation
field is unset immediately above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the test still passes without this part.
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccContainerCluster_failedCreation(clusterName, project.ProjectId), | ||
ExpectError: regexp.MustCompile(".*timeout while waiting for state to become 'DONE'.*"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need the .*
s for these regexes? My understanding was that matching a subsection was valid, not necessarily the whole string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the .*
s.
@@ -358,6 +359,136 @@ func BootstrapServicePerimeterProjects(t *testing.T, desiredProjects int) []*clo | |||
return projects | |||
} | |||
|
|||
func removeContainerServiceAgentRoleFromContainerEngineRobot(t *testing.T, project *cloudresourcemanager.Project) { | |||
config := BootstrapConfig(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are using BootstrapConfig
, I don't believe the requests being made in this function will get recorded or replayed by the VCR. That means the requests will be made for every replay (ie. multiple per PR). You should be able to test with multiple GCB runs in this PR.
If the requests aren't recorded, it might be worth checking the policy first and only calling "Set" if it needs to be updated. That would also make it more like our other bootstrap functions, so you could potentially use that pattern more explicitly, ie. BootstrapProjectWithInvalidContainerEngineRobot()
. This assumes that you never need to reset this project to the original state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made this function only set the policy when changes are required.
96c308a
to
0429be8
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 3 files changed, 239 insertions(+), 3 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccContainerCluster_failedCreation|TestAccRegionInstanceGroupManager_stateful |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still add "Fixes hashicorp/terraform-provider-google#11431" to the PR description if it will be fixed to get the auto-close behavior, but otherwise LGTM
@trodge: I've been seeing this pop up on VCR runs through the day. Is it possible that the bootstrap functions mean we need to exclude this from VCR? |
…eCloudPlatform#7121) * Fix an issue with resuming a failed container cluster creation and add a test * Do not persist operation when resuming during read.
fixes b/228111747
fixes hashicorp/terraform-provider-google#11431
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)