Skip to content
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.

Sign up for GitHub

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_network_manager_deployment - tolerate serveral times of NotFound when waiting for creation. #24330

Merged
merged 4 commits into from
Jan 10, 2024
Merged

azurerm_network_manager_deployment - tolerate serveral times of NotFound when waiting for creation. #24330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5f56356
network manager deployment - add not found check for state wait in cr…
teowa Dec 25, 2023
421c094
fix msg
teowa Dec 25, 2023
3fdb8b9
format code
teowa Jan 4, 2024
2fcb3d1
link issue
teowa Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions internal/services/network/network_manager_deployment_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package network
import (
"context"
"fmt"
"log"
"time"

"github.com/hashicorp/go-azure-helpers/lang/response"
Expand Down Expand Up @@ -334,25 +335,34 @@ func resourceManagerDeploymentWaitForDeleted(ctx context.Context, client *networ

_, err := state.WaitForStateContext(ctx)
if err != nil {
return fmt.Errorf("waiting for the Deployment %s: %+v", *managerDeploymentId, err)
return fmt.Errorf("waiting for the %s: %+v", *managerDeploymentId, err)
}

return nil
}

func resourceManagerDeploymentWaitForFinished(ctx context.Context, client *networkmanagers.NetworkManagersClient, managerDeploymentId *parse.ManagerDeploymentId, d time.Duration) error {
state := &pluginsdk.StateChangeConf{
MinTimeout: 30 * time.Second,
Delay: 10 * time.Second,
Pending: []string{"NotStarted", "Deploying"},
Target: []string{"Deployed"},
Refresh: resourceManagerDeploymentResultRefreshFunc(ctx, client, managerDeploymentId),
Timeout: d,
MinTimeout: 30 * time.Second,
Delay: 10 * time.Second,
Pending: []string{"NotStarted", "Deploying"},
Target: []string{"Deployed"},
NotFoundChecks: 20,
Timeout: d,
Refresh: func() (interface{}, string, error) {
result, state, err := resourceManagerDeploymentResultRefreshFunc(ctx, client, managerDeploymentId)()
if state == "NotFound" {
Comment on lines +352 to +354
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason to wrap this function instead of adding this to that function?

Copy link
Contributor Author

@teowa teowa Jan 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refresh function is used in two place , another is by resourceManagerDeploymentWaitForDeleted function to wait until the resource is surely deleted, waiting deletion does not need NotFoundCheck so we can't add this wrap to the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NotFoundCheck is to error out if it is not found for several times.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is different and unique from other endpoints this sounds like a potential bug?

could we please open a bug and link to it / detail why this is different/special compared to the other uses of the funciton?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

submit API issue for this Azure/azure-rest-api-specs#27327

// the deployment might not found after initial commit, https://github.com/Azure/azure-rest-api-specs/issues/27327
// to serve NotFoundChecks, return nil result
return nil, state, err
}
return result, state, err
},
}

_, err := state.WaitForStateContext(ctx)
if err != nil {
return fmt.Errorf("waiting for the Deployment %s: %+v", *managerDeploymentId, err)
return fmt.Errorf("waiting for the %s: %+v", *managerDeploymentId, err)
}

return nil
Expand Down Expand Up @@ -380,6 +390,7 @@ func resourceManagerDeploymentResultRefreshFunc(ctx context.Context, client *net
}

if resp.Model.Value == nil || len(*resp.Model.Value) == 0 || *(*resp.Model.Value)[0].ConfigurationIds == nil || len(*(*resp.Model.Value)[0].ConfigurationIds) == 0 {
log.Printf("[DEBUG] retrieving %s: listing deployments succeeds however the specific deployment was not found", *id)
return resp, "NotFound", nil
}

Expand Down
2 changes: 1 addition & 1 deletion internal/services/network/network_manager_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ type ManagerResource struct{}

func TestAccNetworkManager(t *testing.T) {
// NOTE: this is a combined test rather than separate split out tests due to
// Azure only being happy about provisioning one network manager per subscription at once
// Azure only being happy about provisioning one (connectivity or securityAdmin) network manager per subscription at once
// (which our test suite can't easily work around)

testCases := map[string]map[string]func(t *testing.T){
Expand Down
Loading