New Resource: azurerm_sql_failover_group
#3901
Conversation
|
I am very interested in this as a native resource. I currently have this functionality in place using ARM templates, but a proper resource would be much nicer. It looks like you’ve covered all the key points. The only questions I have are:
|
|
Failover groups do create a secondary if they don't exist, but there is no way with the API to prevent this or as far as I can tell make TF create a resource for the secondary. If you have this requirement, you can create the primary & secondary, then add the failover group. You're then tracking everything. Doing this also gives you control over the sizing etc of the secondary. Elastic pools shouldn't affect the databases. Certainly the docs make no mention of this, but then the docs are fairly light on detail. With regard to moving or sizing the databases, failover groups have no control over this. if you need this control, it's best to create & manage the resources as specific items. This seems to be the intent of failover groups as defined in Azure. I'm not sure that making it do more is the right thing, but then I'm not always right ;-) |
|
I think I've addressed your comments |
| for _, server := range *input { | ||
| info := make(map[string]interface{}) | ||
| info["id"] = *server.ID | ||
| info["location"] = *server.Location |
There was a problem hiding this comment.
This still needs to be nil checked
|
It causes an error if you set the grace minutes & the mode to manual |
|
I've renamed the funcs as suggested. I'm not sure I follow where the nil check you mentioned needs to go. Partner Server is required & you always need at least 1. |
| for _, server := range *input { | ||
| info := make(map[string]interface{}) | ||
| info["id"] = *server.ID | ||
| info["location"] = *server.Location |
There was a problem hiding this comment.
ie
| info["location"] = *server.Location | |
| if v := server.Location; v != nil { | |
| info["location"] = *v | |
| } |
|
Gotacha, thanks! I've addressed those comments & rebased to the most recent master, fixing the merge conflict. |
There was a problem hiding this comment.
Thanks for the revisions @simongh,
This LGTM now hwoever the tests are failing, i pushed a small change to try and fix them (hope you don't mind) but now i am getting:
------- Stdout: -------
=== RUN TestAccAzureRMSqlFailoverGroup_basic
=== PAUSE TestAccAzureRMSqlFailoverGroup_basic
=== CONT TestAccAzureRMSqlFailoverGroup_basic
--- FAIL: TestAccAzureRMSqlFailoverGroup_basic (502.57s)
testing.go:568: Step 0 error: errors during apply:
Error: Error issuing create/update request for SQL Failover Group "acctest190809180127363915" (Resource Group "acctestRG-190809180127363915", Server "acctestsqlserver190809180127363915-primary"): sql.FailoverGroupsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="InvalidFailoverGroupRegion" Message="Secondary server specified in a Failover Group cannot reside in the same region."
on /opt/teamcity-agent/temp/buildTmp/tf-test378278636/main.tf line 36:
(source code not available)
FAIL
|
Don't mind at all. Failover groups require the partner servers to be in different regions, ideally region replication partners, but any 2 region will do. The tests use testLocation() to get the location in which to create the resources. I'm not sure how to get 2 different locations for the test, other than hard-coding in 2 different regions. |
|
we can use |
|
@leewilson86 I don't think changing the tests is a hiding anything. Having tested it myself, a secondary db gets created, which isn't tracked by TF, but it's existence doesn't prevent the server from being deleted. @katbyte could the tests be picking up on the fact that an untracked db was created on the secondary server? I've spotted & fixed another issue with the read-write policy. I've also removed the tags support, since although the API supports them, they don't appear anywhere in the UI. |
|
First, even if the tags don't show up in the portal i think we should leave them in? I think that test is failing because it is the only test that attempts to update the resource? and for some reason it is trying to create a new DB. If you create another test that calls update i imagine you will see the same error? for @leewilson86, could you suggest a terraform config/test that we could write to ensure we have implemented it correctly? |
|
It could be. I've tried doing updates, both by changing the resource & by changing the HCL, all without failure. If you delete a server it also appears to remove the failover group. I'm at a loss as to what's causing the errors. I'll add the tags support back in though. I'm running the acceptance tests now & I'll see what I can figure out. |
|
I found a couple of things - typos in the tests. The update now passes. The delete is still failing, even though I thought I found the cause. I'm fairly certain it's something that the test is doing, since it works when I manually run the same HCL. Since I'm only half certain of what the test is actually doing, would you mind casting an eye over the test? I've likely missed something simple. |
|
👋 @simongh, So i consulted core internally, and it sounds like that error means upon read, we got zeropartnert servers instead of the expected 1. Looking at the requests i see a delete, and then a So it seems like the API starts a delete and gets a: and when terraform does a get on it next, it still returns the resource as its not done deleting yet. You may have to add a poll there so the delete call doesn't return until it succeeds? |
Do you some existing resource that does this that I can look at it & get the test working? |
|
the cosmos DB resources do this: //the SDK now will return a `WasNotFound` response even when still deleting
stateConf := &resource.StateChangeConf{
Pending: []string{"Deleting"},
Target: []string{"NotFound"},
Timeout: 60 * time.Minute,
MinTimeout: 30 * time.Second,
Refresh: func() (interface{}, string, error) {
resp, err2 := client.Get(ctx, resourceGroup, name)
if err2 != nil {
if utils.ResponseWasNotFound(resp.Response) {
return resp, "NotFound", nil
}
return nil, "", fmt.Errorf("Error reading CosmosDB Account %q after delete (Resource Group %q): %+v", name, resourceGroup, err2)
}
return resp, "Deleting", nil
},
}
if _, err = stateConf.WaitForState(); err != nil {
return fmt.Errorf("Waiting forCosmosDB Account %q to delete (Resource Group %q): %+v", name, resourceGroup, err)
} |
katbyte
changed the title
azurerm_sql_failover_group
|
I think I've found the problem. The resource does a I've also updated the documentation to outline what happens with secondary databases |
|
I hope you don't mind but i pushed some changes to fix the merge errors. |
|
That's great. Thanks for your help.
…On Tue, 20 Aug 2019, 16:24 kt, ***@***.***> wrote:
Merged #3901
<#3901>
into master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3901>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALHWT5537QX2RSILHCJFWDQFQEENANCNFSM4IFXWNJA>
.
|
|
This has been released in version 1.33.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 1.33.0"
}
# ... other configuration ... |
|
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
I've created a resource for adding SQL Failover Groups. I've added tests & documentation. Hopefully there's not much that I've missed. Still a novice when it comes to Go
(fixes #2989)