Migrate AzureSqlDatabase controller to async generic controller #525
Conversation
|
/azp run |
|
No pipelines are associated with this pull request. |
jakiefermsft
changed the title
|
Tests pass so removing WIP. 🤷♂ |
| if err != nil { | ||
| catch := []string{ | ||
| errhelp.ResourceGroupNotFoundErrorCode, | ||
| errhelp.AsyncOpIncompleteError, |
There was a problem hiding this comment.
what happens if you try to create a database for a server that doesn't exist?
There was a problem hiding this comment.
What happened in the old code?
There was a problem hiding this comment.
its different now...the old controller relied on instance.Status.Provisioning to gate the interaction with ARM...now there is a reliance on the Ensure and Delete methods being idempotent
There was a problem hiding this comment.
There is another issue here stemming from this change...
after the first reconciliation where the CreateorUpdate is called the reconciliation we start getting these errors:
2019-12-18T14:37:06.457-0700 ERROR controller-runtime.controller Reconciler error {"controller": "azuresqldatabase", "request": "default/azuresqldatabase-sampleh", "error": "1 error occurred:\n\t* AzureSqlDb GetDB error sql.DatabasesClient#Get: Failure responding to request: StatusCode=404 -- Original Error: json: cannot unmarshal array into Go struct field serviceError2.details of type map[string]interface {}\n\n"}
need to catch the error NotFoundErrorCode = "NotFound"
| } | ||
| } | ||
|
|
||
| return true, fmt.Errorf("AzureSqlDb CreateOrUpdate error %v", err) |
There was a problem hiding this comment.
should set the create error in the instance.Status.Message
There was a problem hiding this comment.
Should I set the error when I am ignoring an error or when it's a real error? I checked out eventhub namespace and it looks like you only set that when it's a caught error. Just want to make sure.
There was a problem hiding this comment.
I think we probably want both in the end. I'm thinking that a user might rather check the status of their resource to see what is wrong then tail the logs of the operator.
| // Database does not exist | ||
| return false, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
might want to update instance.Status.Message here in case the delete error wasn't temporary.
There was a problem hiding this comment.
The error here is that the database is already gone. What should I set instance.Status.Message to in that case?
There was a problem hiding this comment.
im talking about the errors that are not "already gone"...like "Resource Group does not exist"....most of these things seem to throw special errors for that. What about when the Serverr doesn't exist? What about network timeouts?
There was a problem hiding this comment.
I think I see what you are saying. You're not saying this logic is incorrect but that I should just put err into instance.Status.Message on line 96?
| // DBEdition - wraps: https://godoc.org/github.com/Azure/azure-sdk-for-go/services/preview/sql/mgmt/2015-05-01-preview/sql#DatabaseEdition | ||
| type DBEdition byte | ||
| type DBEdition = v1alpha1.DBEdition | ||
| type ReadWriteEndpointFailoverPolicy = v1alpha1.ReadWriteEndpointFailoverPolicy |
There was a problem hiding this comment.
these seem a bit odd...why not just use v1alpha.whatever?
There was a problem hiding this comment.
I moved these into v1alpha to avoid an import cycle. I exported them out of this package as to not have to change all the places that reference these. I think there should be a separate ticket that figures out what type of things belong in the v1alpha package and what code belong elsewhere.
| return true, fmt.Errorf("AzureSqlDb delete error %v", err) | ||
| } | ||
|
|
||
| return true, nil |
There was a problem hiding this comment.
I think this needs to be false, nil because the DeleteDb doesn't return an error on first delete and it doesn't return an error when it's gone.
There was a problem hiding this comment.
Okay I think I follow. I wonder if we can write some documentation or make a graphic that could help illustrate the reasoning behind the return values in the ARMClient. I don't think they are super intuitive.
Returning a true here doesn't signal that things were successful, but that we want to be called again, right?
There was a problem hiding this comment.
makes sense to add comments to the interface and the async controller and also maybe add it to a dev guide
|
this work is being continued in another branch |
This pull request moves all the AzureSqlDatabase controller logic into the generic controller model.
How does this PR make you feel:
closes #465