-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
azurerm_storage_account
- prevent service endpoint error 404 durring re-create
#23002
azurerm_storage_account
- prevent service endpoint error 404 durring re-create
#23002
Conversation
azurerm_storage_account
- prevent service endpoint error 404 durring re-create
Hi @magodo if you would like to format the INFO log messages, please let me know. Thank you |
Hi @petr-stupka, thank you for this PR! For the retrying logic, can we use the terraform-provider-azurerm/internal/services/compute/dedicated_host_resource.go Lines 287 to 294 in fe7093c
|
Hi @magodo, In one of my test, two endpoints took time to get available, so that looks good |
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.
@petr-stupka, this is looking pretty good so far. I left a few comments if you can take a look and let me know what you think. 🚀
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.
Hi @petr-stupka, thanks for pushing those changes. I have left a few more comments and I don't think that the resourceStorageAccountRead
function is the correct place for doing these waits. I think it would be much better if you could relocate that logic to the bottom of resourceStorageAccountCreate
and the resourceStorageAccountUpdate
functions, if it makes sense to put it in the update function.
Hi @petr-stupka, no worries... we have not forgotten about this we are currently discussing the PR internally and will address it shortly, sorry for the delay or lack of communication to that effect, but we are looing at this... 🙂 |
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.
hey @petr-stupka
Thanks for pushing those changes - I've taken a look through and left some comments inline, but if we can fix those up then we should be able to take another look at get this one in 👍
Thanks!
Refresh: func() (interface{}, string, error) { | ||
properties, err := queueClient.GetServiceProperties(ctx, id.ResourceGroupName, id.StorageAccountName) | ||
if err != nil { | ||
return handleStorageServiceError("queue", err) |
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 think we can simplify this to:
return handleStorageServiceError("queue", err) | |
return "Error", "Error", fmt.Errorf("waiting for the Blob %s to become available: %+v", id, err") |
Since the handleStorageServiceError
function below assumes that this is Completed if it's not a 404 (which would mean we mark this as Completed when it's an error?)
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.
If the queueClient.GetServiceProperties
function returns error (this may be the 404 error or any other error storage API may return)
This error (generic error type) is handled by handleStorageServiceError
function. It return Pending
, in case of 404, however any other error is returned imidiately with Completed
to brake the loop.
This has been requested by @magodo to avoid consuming all errors.
If i understand the logic correctly, in your example the loop will continue also in case of non 404 error?
And one exception here for queue endpoint > there is no error in case of 404 and therefore i put there the workaround below if properties == nil
You the owners, you can say the final word how the code should looks like. From my point of view the function handleStorageServiceError
make sense and therefore asking for confirmation.
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.
@tombuildsstuff Could you please review above comment?
Refresh: func() (interface{}, string, error) { | ||
_, err = fileServiceClient.GetServiceProperties(ctx, id.ResourceGroupName, id.StorageAccountName) | ||
if err != nil { | ||
return handleStorageServiceError("file", err) |
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.
(as above) we should be able to just return Pending
here instead?
Refresh: func() (interface{}, string, error) { | ||
_, err = accountsClient.GetServiceProperties(ctx, id.StorageAccountName) | ||
if err != nil { | ||
return handleStorageServiceError("static website", err) |
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.
(as above) we should be able to just return Pending
here instead?
func handleStorageServiceError(service string, err error) (interface{}, string, error) { | ||
// if the error is a DetailedError type, check the status code and return the appropriate state | ||
var respErr azautorest.DetailedError | ||
if errors.As(err, &respErr) { | ||
log.Printf("[DEBUG] error fetching %s service properties: statusCode=%d, message=%s, error=%s\n", service, respErr.StatusCode.(int), respErr.Message, err) | ||
// if the status code is 404 (not found), retry the request | ||
if respErr.StatusCode.(int) == http.StatusNotFound { | ||
// if the status code is 404 (not found), retry the request | ||
log.Printf("[DEBUG] %s service is not available, retrying...\n", service) | ||
return false, "Pending", nil | ||
} | ||
} | ||
// if the error is unhandled type or status, log the error and return the error state | ||
log.Printf("[DEBUG] unexpected error while fetching %s service properties: %v\n", service, err) | ||
return nil, "Completed", err | ||
} |
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.
as such I think this function can be removed?
{ | ||
// destroy the account and recreate it | ||
Config: r.recreate(data), | ||
Destroy: true, |
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.
this would destroy the resource group too, instead we can delete just the account via:
{ | |
// destroy the account and recreate it | |
Config: r.recreate(data), | |
Destroy: true, | |
{ | |
// destroy the account so that we can recreate it below | |
Config: r.template(data), |
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.
fixed. also for basic account
Co-authored-by: Tom Harvey <[email protected]>
sorry for the delay, i reviewed your suggested changes, i would like to ask for confirmation the error handling is not required. See #23002 (comment) |
@petr-stupka, I have brought this up in our sync, we will get to it asap 🙂 |
Hi @WodansSon just a kindly reminder |
@petr-stupka Has there been any further progress in getting this PR merged? |
Hi guys @magodo @WodansSon can you please let me know how to proceed with the PR? Should i modify it like @tombuildsstuff proposed please? |
Hi @magodo @WodansSon do you think we can make a progress on this? AS i said i can do what Tom requested or adjust it with the error handling described above |
@petr-stupka I've ping tom and hopefully he'll look at that comment when he has bandwidth. |
This PR is being labeled as "stale" because it has not been updated for 30 or more days. If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone. If you need some help completing this PR, please leave a comment letting us know. Thank you! |
hey @petr-stupka Thanks for this PR and apologies for the delayed review/response here. There’s been a number of changes to the Storage resources since this PR was first opened, namely the Data Plane SDK and it’s base layer have been updated to make use of Since this PR is currently relying on the error behaviour of the older base layer, the logic in this PR would need adapting to account for that - however there’s also some larger changes needed for the Storage Account resource as a part of moving it to I’ve recently been looking into porting the remaining Storage resources over to using Whilst I took a look into updating this PR to account for that work, when I pulled this down locally to test/reproduce this, I realised the test case doesn’t recreate the Storage Account at present and as such it would also require updating. As such I ended up including a fix for #22992 in #26218 - which both updates this logic to use the new base layer (and the As such whilst I’d like to thank you for this contribution and apologise that this PR has been sitting for a while - I hope you don’t mind but I’m going to close this PR in favour of #26218 which incorporates a fix for #22992. Thanks! |
Cool, i don't mind closing this PR, important is the bug has been fixed! Thank you for the update! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
resolve #22992
resolve #19055
resolve #18897
resolve #15609
resolve #12627
May be solution for #20257
Added loop for reading blob, file, queue and static website properties with timeout 5 min for each service
log level INFO: