diff --git a/cli/azd/pkg/infra/azure_resource_manager.go b/cli/azd/pkg/infra/azure_resource_manager.go index a3d6e5773fa..3732583aa52 100644 --- a/cli/azd/pkg/infra/azure_resource_manager.go +++ b/cli/azd/pkg/infra/azure_resource_manager.go @@ -9,9 +9,7 @@ import ( "fmt" "slices" "strings" - "time" - - "maps" + "sync" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" @@ -19,7 +17,6 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/azapi" "github.com/azure/azure-dev/cli/azd/pkg/azure" "github.com/azure/azure-dev/cli/azd/pkg/azureutil" - "github.com/azure/azure-dev/cli/azd/pkg/compare" "github.com/azure/azure-dev/cli/azd/pkg/output" ) @@ -29,8 +26,11 @@ type AzureResourceManager struct { } type ResourceManager interface { - GetDeploymentResourceOperations( - ctx context.Context, deployment Deployment, queryStart *time.Time) ([]*armresources.DeploymentOperation, error) + WalkDeploymentOperations( + ctx context.Context, + deployment Deployment, + fn WalkDeploymentOperationFunc, + ) error GetResourceTypeDisplayName( ctx context.Context, subscriptionId string, @@ -49,6 +49,19 @@ type ResourceManager interface { ) (string, error) } +// WalkDeploymentOperationFunc is invoked for each valid deployment operation encountered during traversal. +// +// Returning a non-nil error will halt the traversal and return that error. +// Returning SkipExpand will prevent traversal of any nested deployments within the current operation, but will continue +// traversal of sibling operations. +type WalkDeploymentOperationFunc func(ctx context.Context, operation *armresources.DeploymentOperation) error + +// SkipExpand can be returned by WalkDeploymentOperationFunc to skip expanding a nested deployment's children. +var SkipExpand = errors.New("skip deployment expansion") + +// maxConcurrentDeploymentFetches limits concurrent ARM API calls when fetching nested deployment operations. +const maxConcurrentDeploymentFetches = 10 + func NewAzureResourceManager( resourceService *azapi.ResourceService, deploymentService *azapi.StandardDeployments, @@ -59,31 +72,149 @@ func NewAzureResourceManager( } } -// GetDeploymentResourceOperations gets the list of all the resources created as part of the provided deployment. -// Each DeploymentOperation on the list holds a resource and the result of its deployment. -// One deployment operation can trigger new deployment operations, GetDeploymentResourceOperations traverses all -// operations recursively to find the leaf operations. -func (rm *AzureResourceManager) GetDeploymentResourceOperations( +// WalkDeploymentOperations traverses deployment operations and allows callers to skip nested expansion. +func (rm *AzureResourceManager) WalkDeploymentOperations( ctx context.Context, deployment Deployment, - queryStart *time.Time, -) ([]*armresources.DeploymentOperation, error) { - allOperations := []*armresources.DeploymentOperation{} - + fn WalkDeploymentOperationFunc, +) error { rootDeploymentOperations, err := deployment.Operations(ctx) if err != nil { - return nil, fmt.Errorf("getting root deployment operations: %w", err) + return fmt.Errorf("getting root deployment operations: %w", err) } - operationMap := map[string]*armresources.DeploymentOperation{} - if err := rm.appendDeploymentOperationsRecursive(ctx, queryStart, rootDeploymentOperations, operationMap); err != nil { - return nil, err + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + jobs := make(chan *arm.ResourceID, maxConcurrentDeploymentFetches) + results := make(chan []*armresources.DeploymentOperation, maxConcurrentDeploymentFetches) + errCh := make(chan error, 1) + + var workers sync.WaitGroup + worker := func() { + defer workers.Done() + + for resourceID := range jobs { + operations, err := rm.fetchNestedOperations(ctx, resourceID) + if err != nil { + select { + case errCh <- fmt.Errorf("getting deployment operations recursively: %w", err): + default: + } + cancel() + return + } + + select { + case <-ctx.Done(): + return + case results <- operations: + } + } } - recursiveOperations := slices.Collect(maps.Values(operationMap)) - allOperations = append(allOperations, recursiveOperations...) + for range maxConcurrentDeploymentFetches { + workers.Add(1) + go worker() + } + + queueNestedDeployments := func( + queue []*arm.ResourceID, + operations []*armresources.DeploymentOperation, + ) ([]*arm.ResourceID, error) { + for _, operation := range operations { + if operation.ID == nil || operation.Properties == nil { + continue + } + + if fn != nil { + // invoke the walk function for every deployment operation + walkErr := fn(ctx, operation) + if errors.Is(walkErr, SkipExpand) { + continue + } else if walkErr != nil { + return nil, walkErr + } + } - return allOperations, nil + // handle nested deployments + if !isNestedDeployment(operation) { + continue + } + if operation.Properties.TargetResource == nil || operation.Properties.TargetResource.ID == nil { + continue + } + + resourceID, err := arm.ParseResourceID(*operation.Properties.TargetResource.ID) + if err != nil { + return nil, fmt.Errorf("parsing deployment resource ID: %w", err) + } + + queue = append(queue, resourceID) + } + + return queue, nil + } + + queue, err := queueNestedDeployments(nil, rootDeploymentOperations) + if err != nil { + cancel() + close(jobs) + workers.Wait() + return err + } + + pending := 0 + + // we terminate when there are no more jobs and no more pending operations to fetch + for pending > 0 || len(queue) > 0 { + var nextJob *arm.ResourceID + var jobsCh chan *arm.ResourceID + if len(queue) > 0 { + nextJob = queue[0] + jobsCh = jobs + } + + select { + case jobsCh <- nextJob: + queue = queue[1:] + pending++ + case <-ctx.Done(): + close(jobs) + workers.Wait() + select { + case walkErr := <-errCh: + return walkErr + default: + return ctx.Err() + } + case walkErr := <-errCh: + close(jobs) + workers.Wait() + return walkErr + case nestedOperations := <-results: + pending-- + + queue, err = queueNestedDeployments(queue, nestedOperations) + if err != nil { + cancel() + close(jobs) + workers.Wait() + return err + } + } + } + + close(jobs) + workers.Wait() + + select { + case walkErr := <-errCh: + return walkErr + default: + } + + return nil } // GetResourceGroupsForEnvironment gets all resources groups for a given environment @@ -317,64 +448,44 @@ func (rm *AzureResourceManager) getRedisEnterpriseResourceTypeDisplayName( } } -// appendDeploymentResourcesRecursive gets the leaf deployment operations and adds them to resourceOperations -// if they are not already in the list. -func (rm *AzureResourceManager) appendDeploymentOperationsRecursive( - ctx context.Context, - queryStart *time.Time, - operations []*armresources.DeploymentOperation, - operationMap map[string]*armresources.DeploymentOperation, -) error { - for _, operation := range operations { - // Operations w/o target data can't be resolved. Ignoring them - if operation.Properties.TargetResource == nil || - // The time stamp is used to filter only records after the queryStart. - // We ignore the resource if we can't know when it was created - operation.Properties.Timestamp == nil || - // The resource type is required to resolve the name of the resource. - // If the dep-op is missing this, we can't resolve it. - compare.IsStringNilOrEmpty(operation.Properties.TargetResource.ResourceType) { - continue - } +func isNestedDeployment(operation *armresources.DeploymentOperation) bool { + if operation.Properties.TargetResource == nil || + operation.Properties.ProvisioningOperation == nil || + operation.Properties.TargetResource.ResourceType == nil { + return false + } - // Process any nested deployments - if *operation.Properties.TargetResource.ResourceType == string(azapi.AzureResourceTypeDeployment) && - *operation.Properties.ProvisioningOperation == armresources.ProvisioningOperationCreate { - deploymentResourceId, err := arm.ParseResourceID(*operation.Properties.TargetResource.ID) - if err != nil { - return fmt.Errorf("parsing deployment resource ID: %w", err) - } + return *operation.Properties.TargetResource.ResourceType == string(azapi.AzureResourceTypeDeployment) && + *operation.Properties.ProvisioningOperation == armresources.ProvisioningOperationCreate +} - var nestedOperations []*armresources.DeploymentOperation - var nestedError error - - if deploymentResourceId.ResourceGroupName == "" { - nestedOperations, nestedError = rm.deploymentService.ListSubscriptionDeploymentOperations( - ctx, - deploymentResourceId.SubscriptionID, - deploymentResourceId.Name) - } else { - nestedOperations, nestedError = rm.deploymentService.ListResourceGroupDeploymentOperations( - ctx, - deploymentResourceId.SubscriptionID, - deploymentResourceId.ResourceGroupName, - deploymentResourceId.Name, - ) - } +func isTerminalProvisioningState(state *string) bool { + if state == nil { + return false + } - if nestedError != nil { - return fmt.Errorf("getting deployment operations recursively: %w", nestedError) - } + switch *state { + case string(armresources.ProvisioningStateSucceeded), + string(armresources.ProvisioningStateFailed), + string(armresources.ProvisioningStateCanceled): + return true + default: + return false + } +} - if err = rm.appendDeploymentOperationsRecursive(ctx, queryStart, nestedOperations, operationMap); err != nil { - return err - } - } else if *operation.Properties.ProvisioningOperation == armresources.ProvisioningOperationCreate && - // Only append CREATE operations that started after our queryStart time - operation.Properties.Timestamp.After(*queryStart) { - operationMap[*operation.ID] = operation - } +func (rm *AzureResourceManager) fetchNestedOperations( + ctx context.Context, + resourceID *arm.ResourceID, +) ([]*armresources.DeploymentOperation, error) { + if resourceID.ResourceGroupName == "" { + return rm.deploymentService.ListSubscriptionDeploymentOperations(ctx, resourceID.SubscriptionID, resourceID.Name) } - return nil + return rm.deploymentService.ListResourceGroupDeploymentOperations( + ctx, + resourceID.SubscriptionID, + resourceID.ResourceGroupName, + resourceID.Name, + ) } diff --git a/cli/azd/pkg/infra/azure_resource_manager_test.go b/cli/azd/pkg/infra/azure_resource_manager_test.go index bf2a692b855..60bee15e15d 100644 --- a/cli/azd/pkg/infra/azure_resource_manager_test.go +++ b/cli/azd/pkg/infra/azure_resource_manager_test.go @@ -11,7 +11,9 @@ import ( "io" "net/http" "net/url" + "regexp" "strings" + "sync" "testing" "time" @@ -161,9 +163,152 @@ var mockSubDeploymentOperationsEmpty string = ` "value": [] } ` -var qStart = time.Now() -func TestGetDeploymentResourceOperationsSuccess(t *testing.T) { +func createDeploymentOperationResponse(t *testing.T, operations ...*armresources.DeploymentOperation) string { + t.Helper() + + response := armresources.DeploymentOperationsListResult{ + Value: operations, + } + + body, err := json.Marshal(response) + if err != nil { + panic(err) + } + + return string(body) +} + +func createNestedDeploymentOperation( + id string, + deploymentName string, + state armresources.ProvisioningState, +) *armresources.DeploymentOperation { + resourceID := fmt.Sprintf("/subscriptions/SUBSCRIPTION_ID/resourceGroups/resource-group-name/providers/"+ + "Microsoft.Resources/deployments/%s", deploymentName) + + return &armresources.DeploymentOperation{ + ID: to.Ptr(id), + Properties: &armresources.DeploymentOperationProperties{ + ProvisioningOperation: to.Ptr(armresources.ProvisioningOperationCreate), + ProvisioningState: to.Ptr(string(state)), + TargetResource: &armresources.TargetResource{ + ResourceType: to.Ptr(string(azapi.AzureResourceTypeDeployment)), + ID: to.Ptr(resourceID), + ResourceName: to.Ptr(deploymentName), + }, + Timestamp: to.Ptr(time.Now().UTC().Add(time.Hour)), + }, + } +} + +func createLeafOperation(id string, resourceType string, resourceName string) *armresources.DeploymentOperation { + resourceID := fmt.Sprintf( + "/subscriptions/SUBSCRIPTION_ID/resourceGroups/resource-group-name/providers/%s/%s", + resourceType, + resourceName, + ) + + return &armresources.DeploymentOperation{ + ID: to.Ptr(id), + Properties: &armresources.DeploymentOperationProperties{ + ProvisioningOperation: to.Ptr(armresources.ProvisioningOperationCreate), + ProvisioningState: to.Ptr(string(armresources.ProvisioningStateSucceeded)), + TargetResource: &armresources.TargetResource{ + ResourceType: to.Ptr(resourceType), + ID: to.Ptr(resourceID), + ResourceName: to.Ptr(resourceName), + }, + Timestamp: to.Ptr(time.Now().UTC().Add(time.Hour)), + }, + } +} + +func mockDeploymentOperationsByName( + t *testing.T, + m mocks.MockContext, + operationsByName map[string][]*armresources.DeploymentOperation, +) map[string]int { + t.Helper() + + callCounts := map[string]int{} + var callCountsMu sync.Mutex + deploymentPathPattern := regexp.MustCompile(`(?i)/deployments/([^/]+)/operations`) + + m.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodGet && deploymentPathPattern.MatchString(request.URL.Path) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + matches := deploymentPathPattern.FindStringSubmatch(request.URL.Path) + deploymentName := "" + if len(matches) > 1 { + deploymentName = strings.ToLower(matches[1]) + } + + callCountsMu.Lock() + callCounts[deploymentName]++ + callCountsMu.Unlock() + + operations, exists := operationsByName[deploymentName] + if !exists { + operations = []*armresources.DeploymentOperation{} + } + + body := createDeploymentOperationResponse(t, operations...) + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBuffer([]byte(body))), + Request: &http.Request{ + Method: http.MethodGet, + URL: request.URL, + }, + }, nil + }) + + return callCounts +} + +func mockDeploymentOperationsByNameWithGate( + t *testing.T, + m mocks.MockContext, + operationsByName map[string][]*armresources.DeploymentOperation, + release <-chan struct{}, +) { + t.Helper() + + deploymentPathPattern := regexp.MustCompile(`(?i)/deployments/([^/]+)/operations`) + + m.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodGet && deploymentPathPattern.MatchString(request.URL.Path) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + matches := deploymentPathPattern.FindStringSubmatch(request.URL.Path) + deploymentName := "" + if len(matches) > 1 { + deploymentName = strings.ToLower(matches[1]) + } + + // For non-root deployments, wait for the gate to open. + if deploymentName != "deployment_name" { + <-release + } + + operations, exists := operationsByName[deploymentName] + if !exists { + operations = []*armresources.DeploymentOperation{} + } + + body := createDeploymentOperationResponse(t, operations...) + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBuffer([]byte(body))), + Request: &http.Request{ + Method: http.MethodGet, + URL: request.URL, + }, + }, nil + }) +} + +func TestWalkDeploymentOperationsSuccess(t *testing.T) { subCalls := 0 groupCalls := 0 @@ -211,16 +356,338 @@ func TestGetDeploymentResourceOperationsSuccess(t *testing.T) { }) arm := NewAzureResourceManager(resourceService, deploymentService) - operations, err := arm.GetDeploymentResourceOperations(*mockContext.Context, deployment, &qStart) - require.NotNil(t, operations) - require.Nil(t, err) + operationCount := 0 + err := arm.WalkDeploymentOperations(*mockContext.Context, deployment, + func(ctx context.Context, operation *armresources.DeploymentOperation) error { + operationCount++ + return nil + }) + require.NoError(t, err) - require.Len(t, operations, 4) + require.Equal(t, 5, operationCount) require.Equal(t, 1, subCalls) require.Equal(t, 1, groupCalls) } -func TestGetDeploymentResourceOperationsFail(t *testing.T) { +func TestWalkDeploymentOperationsSkipExpand(t *testing.T) { + subCalls := 0 + groupCalls := 0 + + mockContext := mocks.NewMockContext(context.Background()) + resourceService := azapi.NewResourceService(mockContext.SubscriptionCredentialProvider, mockContext.ArmClientOptions) + deploymentService := mockazapi.NewStandardDeploymentsFromMockContext(mockContext) + scope := newSubscriptionScope(deploymentService, "SUBSCRIPTION_ID", "eastus2") + deployment := NewSubscriptionDeployment( + scope, + "DEPLOYMENT_NAME", + ) + + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodGet && strings.Contains( + request.URL.Path, + "/subscriptions/SUBSCRIPTION_ID/resourcegroups/resource-group-name/deployments/group-deployment-name/operations", + ) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + groupCalls++ + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBuffer([]byte(mockGroupDeploymentOperations))), + Request: &http.Request{ + Method: http.MethodGet, + URL: request.URL, + }, + }, nil + }) + + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodGet && strings.Contains( + request.URL.Path, + "/subscriptions/SUBSCRIPTION_ID/providers/Microsoft.Resources/deployments/DEPLOYMENT_NAME/operations", + ) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + subCalls++ + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBuffer([]byte(mockSubDeploymentOperations))), + Request: &http.Request{ + Method: http.MethodGet, + URL: request.URL, + }, + }, nil + }) + + arm := NewAzureResourceManager(resourceService, deploymentService) + err := arm.WalkDeploymentOperations(*mockContext.Context, deployment, + func(ctx context.Context, operation *armresources.DeploymentOperation) error { + if operation.ID != nil && *operation.ID == "deployment-id" { + return SkipExpand + } + + return nil + }) + require.NoError(t, err) + require.Equal(t, 1, subCalls) + require.Equal(t, 0, groupCalls) +} + +func TestWalkDeploymentOperationsTreeShapes(t *testing.T) { + tests := []struct { + name string + operationsByName map[string][]*armresources.DeploymentOperation + expectedNestedCalls []string + }{ + { + name: "deep-chain", + operationsByName: map[string][]*armresources.DeploymentOperation{ + "deployment_name": { + createNestedDeploymentOperation("nested-1", "dep1", armresources.ProvisioningStateRunning), + }, + "dep1": { + createNestedDeploymentOperation("nested-2", "dep2", armresources.ProvisioningStateRunning), + createLeafOperation("leaf-dep1", "Microsoft.Storage/storageAccounts", "storage-dep1"), + }, + "dep2": { + createNestedDeploymentOperation("nested-3", "dep3", armresources.ProvisioningStateRunning), + }, + "dep3": { + createLeafOperation("leaf-dep3", "Microsoft.KeyVault/vaults", "vault-dep3"), + }, + }, + expectedNestedCalls: []string{"dep1", "dep2", "dep3"}, + }, + { + name: "mixed-tree", + operationsByName: map[string][]*armresources.DeploymentOperation{ + "deployment_name": { + createNestedDeploymentOperation("nested-a", "depa", armresources.ProvisioningStateRunning), + createNestedDeploymentOperation("nested-b", "depb", armresources.ProvisioningStateRunning), + createLeafOperation("root-leaf", "Microsoft.Web/sites", "root-web"), + }, + "depa": { + createNestedDeploymentOperation("nested-a1", "depa1", armresources.ProvisioningStateRunning), + createLeafOperation("depa-leaf", "Microsoft.Storage/storageAccounts", "depa-storage"), + }, + "depa1": { + createLeafOperation("depa1-leaf", "Microsoft.KeyVault/vaults", "depa1-vault"), + }, + "depb": { + createLeafOperation("depb-leaf", "Microsoft.Web/sites", "depb-web"), + }, + }, + expectedNestedCalls: []string{"depa", "depa1", "depb"}, + }, + { + name: "wide-fan-out", + operationsByName: func() map[string][]*armresources.DeploymentOperation { + operationsByName := map[string][]*armresources.DeploymentOperation{ + "deployment_name": {}, + } + + for i := range 12 { + name := fmt.Sprintf("depw%d", i) + operationsByName["deployment_name"] = append( + operationsByName["deployment_name"], + createNestedDeploymentOperation( + fmt.Sprintf("nested-%s", name), + name, + armresources.ProvisioningStateRunning, + ), + ) + operationsByName[name] = []*armresources.DeploymentOperation{ + createLeafOperation( + fmt.Sprintf("leaf-%s", name), + "Microsoft.Web/sites", + fmt.Sprintf("site-%s", name), + ), + } + } + + return operationsByName + }(), + expectedNestedCalls: []string{ + "depw0", "depw1", "depw2", "depw3", "depw4", "depw5", + "depw6", "depw7", "depw8", "depw9", "depw10", "depw11", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + resourceService := azapi.NewResourceService( + mockContext.SubscriptionCredentialProvider, + mockContext.ArmClientOptions, + ) + deploymentService := mockazapi.NewStandardDeploymentsFromMockContext(mockContext) + scope := newSubscriptionScope(deploymentService, "SUBSCRIPTION_ID", "eastus2") + deployment := NewSubscriptionDeployment(scope, "DEPLOYMENT_NAME") + + callCounts := mockDeploymentOperationsByName(t, *mockContext, tt.operationsByName) + + arm := NewAzureResourceManager(resourceService, deploymentService) + walkCount := 0 + err := arm.WalkDeploymentOperations(*mockContext.Context, deployment, + func(ctx context.Context, operation *armresources.DeploymentOperation) error { + walkCount++ + return nil + }) + require.NoError(t, err) + require.NotZero(t, walkCount) + require.Equal(t, 1, callCounts["deployment_name"]) + + for _, nestedDeploymentName := range tt.expectedNestedCalls { + require.Equal(t, 1, callCounts[nestedDeploymentName]) + } + }) + } +} + +func TestWalkDeploymentOperationsContextCancelledDuringTraversal(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + resourceService := azapi.NewResourceService(mockContext.SubscriptionCredentialProvider, mockContext.ArmClientOptions) + deploymentService := mockazapi.NewStandardDeploymentsFromMockContext(mockContext) + scope := newSubscriptionScope(deploymentService, "SUBSCRIPTION_ID", "eastus2") + deployment := NewSubscriptionDeployment(scope, "DEPLOYMENT_NAME") + + startedNestedFetch := make(chan struct{}) + releaseNestedFetch := make(chan struct{}) + + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodGet && strings.Contains( + strings.ToLower(request.URL.Path), + "/providers/microsoft.resources/deployments/deployment_name/operations", + ) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + body := createDeploymentOperationResponse(t, + createNestedDeploymentOperation("nested-cancel", "dep-cancel", armresources.ProvisioningStateRunning), + ) + + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBuffer([]byte(body))), + Request: &http.Request{ + Method: http.MethodGet, + URL: request.URL, + }, + }, nil + }) + + mockContext.HttpClient.When(func(request *http.Request) bool { + return request.Method == http.MethodGet && strings.Contains( + strings.ToLower(request.URL.Path), + "/deployments/dep-cancel/operations", + ) + }).RespondFn(func(request *http.Request) (*http.Response, error) { + select { + case <-startedNestedFetch: + default: + close(startedNestedFetch) + } + + <-releaseNestedFetch + + body := createDeploymentOperationResponse(t) + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBuffer([]byte(body))), + Request: &http.Request{ + Method: http.MethodGet, + URL: request.URL, + }, + }, nil + }) + + ctx, cancel := context.WithCancel(*mockContext.Context) + defer cancel() + + arm := NewAzureResourceManager(resourceService, deploymentService) + resultCh := make(chan error, 1) + + go func() { + err := arm.WalkDeploymentOperations(ctx, deployment, + func(ctx context.Context, operation *armresources.DeploymentOperation) error { + return nil + }) + resultCh <- err + }() + + select { + case <-startedNestedFetch: + case <-time.After(2 * time.Second): + t.Fatal("nested deployment fetch did not start") + } + + cancel() + close(releaseNestedFetch) + + select { + case err := <-resultCh: + require.Error(t, err) + case <-time.After(2 * time.Second): + t.Fatal("WalkDeploymentOperations did not return after cancellation") + } +} + +func TestWalkDeploymentOperationsCallbackErrorNoDeadlock(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + resourceService := azapi.NewResourceService(mockContext.SubscriptionCredentialProvider, mockContext.ArmClientOptions) + deploymentService := mockazapi.NewStandardDeploymentsFromMockContext(mockContext) + scope := newSubscriptionScope(deploymentService, "SUBSCRIPTION_ID", "eastus2") + deployment := NewSubscriptionDeployment(scope, "DEPLOYMENT_NAME") + + // Root has two nested deployments so both workers will be busy. + // Each nested deployment returns a leaf operation, giving the callback a chance to error. + operationsByName := map[string][]*armresources.DeploymentOperation{ + "deployment_name": { + createNestedDeploymentOperation("nested-a", "dep-a", armresources.ProvisioningStateRunning), + createNestedDeploymentOperation("nested-b", "dep-b", armresources.ProvisioningStateRunning), + }, + "dep-a": { + createLeafOperation("leaf-a", "Microsoft.Web/sites", "site-a"), + }, + "dep-b": { + createLeafOperation("leaf-b", "Microsoft.Storage/storageAccounts", "storage-b"), + }, + } + + // Gate nested fetches so both workers have results ready close together. + release := make(chan struct{}) + mockDeploymentOperationsByNameWithGate(t, *mockContext, operationsByName, release) + + callbackErr := fmt.Errorf("callback failed") + arm := NewAzureResourceManager(resourceService, deploymentService) + + resultCh := make(chan error, 1) + go func() { + callCount := 0 + err := arm.WalkDeploymentOperations(*mockContext.Context, deployment, + func(ctx context.Context, operation *armresources.DeploymentOperation) error { + // Error on the first nested leaf operation received, while the other worker + // may be trying to send on the results channel. + if operation.Properties != nil && operation.Properties.TargetResource != nil && + *operation.Properties.TargetResource.ResourceType != string(azapi.AzureResourceTypeDeployment) { + callCount++ + if callCount == 1 { + return callbackErr + } + } + return nil + }) + resultCh <- err + }() + + // Release the gate so both nested fetches complete. + close(release) + + select { + case err := <-resultCh: + require.ErrorIs(t, err, callbackErr) + case <-time.After(5 * time.Second): + t.Fatal("WalkDeploymentOperations deadlocked on callback error") + } +} + +func TestWalkDeploymentOperationsFail(t *testing.T) { subCalls := 0 groupCalls := 0 @@ -273,16 +740,17 @@ func TestGetDeploymentResourceOperationsFail(t *testing.T) { }) arm := NewAzureResourceManager(resourceService, deploymentService) - operations, err := arm.GetDeploymentResourceOperations(*mockContext.Context, deployment, &qStart) - - require.Nil(t, operations) + err := arm.WalkDeploymentOperations(*mockContext.Context, deployment, + func(ctx context.Context, operation *armresources.DeploymentOperation) error { + return nil + }) require.NotNil(t, err) require.ErrorContains(t, err, "failed getting list of deployment operations from subscription") require.Equal(t, 1, subCalls) require.Equal(t, 0, groupCalls) } -func TestGetDeploymentResourceOperationsNoResourceGroup(t *testing.T) { +func TestWalkDeploymentOperationsNoResourceGroup(t *testing.T) { subCalls := 0 groupCalls := 0 @@ -329,16 +797,16 @@ func TestGetDeploymentResourceOperationsNoResourceGroup(t *testing.T) { }) arm := NewAzureResourceManager(resourceService, deploymentService) - operations, err := arm.GetDeploymentResourceOperations(*mockContext.Context, deployment, &qStart) - - require.NotNil(t, operations) - require.Nil(t, err) - require.Len(t, operations, 0) + err := arm.WalkDeploymentOperations(*mockContext.Context, deployment, + func(ctx context.Context, operation *armresources.DeploymentOperation) error { + return nil + }) + require.NoError(t, err) require.Equal(t, 1, subCalls) require.Equal(t, 0, groupCalls) } -func TestGetDeploymentResourceOperationsWithNestedDeployments(t *testing.T) { +func TestWalkDeploymentOperationsWithNestedDeployments(t *testing.T) { subCalls := 0 groupCalls := 0 @@ -402,11 +870,15 @@ func TestGetDeploymentResourceOperationsWithNestedDeployments(t *testing.T) { }) arm := NewAzureResourceManager(resourceService, deploymentService) - operations, err := arm.GetDeploymentResourceOperations(*mockContext.Context, deployment, &qStart) + operationCount := 0 + err := arm.WalkDeploymentOperations(*mockContext.Context, deployment, + func(ctx context.Context, operation *armresources.DeploymentOperation) error { + operationCount++ + return nil + }) - require.NotNil(t, operations) - require.Nil(t, err) - require.Len(t, operations, 4) + require.NoError(t, err) + require.Equal(t, 8, operationCount) require.Equal(t, 1, subCalls) require.Equal(t, 2, groupCalls) } diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go index 284ce3fb548..684e69eb8f4 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go @@ -1011,12 +1011,12 @@ type mockedScope struct { type mockResourceManager struct{} -func (m *mockResourceManager) GetDeploymentResourceOperations( +func (m *mockResourceManager) WalkDeploymentOperations( ctx context.Context, deployment infra.Deployment, - queryStart *time.Time, -) ([]*armresources.DeploymentOperation, error) { - return nil, nil + fn infra.WalkDeploymentOperationFunc, +) error { + return nil } func (m *mockResourceManager) GetResourceTypeDisplayName( diff --git a/cli/azd/pkg/infra/provisioning_progress_display.go b/cli/azd/pkg/infra/provisioning_progress_display.go index bbdb1726873..11ac7bdebb3 100644 --- a/cli/azd/pkg/infra/provisioning_progress_display.go +++ b/cli/azd/pkg/infra/provisioning_progress_display.go @@ -8,6 +8,7 @@ import ( "fmt" "log" "os" + "slices" "sort" "strconv" "strings" @@ -29,9 +30,15 @@ type ProvisioningProgressDisplay struct { displayedResources map[string]bool // Cache for display names, keyed by resource IDs resourceDisplayNames map[string]string - resourceManager ResourceManager - console input.Console - deployment Deployment + // Tracks the number of times we've observed a terminal provisioning state for a given deployment operation, + // keyed by operation ID + terminalOperationPollCounts map[string]int + // The last recorded spinner message, used to avoid unnecessary updates to the spinner + lastSpinnerMessage string + + resourceManager ResourceManager + console input.Console + deployment Deployment } func NewProvisioningProgressDisplay( @@ -40,11 +47,12 @@ func NewProvisioningProgressDisplay( deployment Deployment, ) *ProvisioningProgressDisplay { return &ProvisioningProgressDisplay{ - displayedResources: map[string]bool{}, - resourceDisplayNames: map[string]string{}, - deployment: deployment, - resourceManager: rm, - console: console, + displayedResources: map[string]bool{}, + resourceDisplayNames: map[string]string{}, + terminalOperationPollCounts: map[string]int{}, + deployment: deployment, + resourceManager: rm, + console: console, } } @@ -121,31 +129,54 @@ func (display *ProvisioningProgressDisplay) ReportProgress( ) } - operations, err := display.resourceManager.GetDeploymentResourceOperations(ctx, display.deployment, queryStart) - if err != nil { - // Status display is best-effort activity. - return err - } - newlyDeployedResources := []*armresources.DeploymentOperation{} newlyFailedResources := []*armresources.DeploymentOperation{} runningDeployments := []*armresources.DeploymentOperation{} - for i := range operations { - if operations[i].Properties.TargetResource != nil { - resourceId := *operations[i].Properties.TargetResource.ResourceName + err := display.resourceManager.WalkDeploymentOperations(ctx, display.deployment, + func(ctx context.Context, operation *armresources.DeploymentOperation) error { + if isNestedDeployment(operation) { + if isTerminalProvisioningState(operation.Properties.ProvisioningState) { + display.terminalOperationPollCounts[*operation.ID]++ + if display.terminalOperationPollCounts[*operation.ID] >= 2 { + // we poll terminal operations twice to ensure we have properly seen it, + // to avoid missing any resources that are created right at the end of the deployment operation + return SkipExpand + } + + } else { + // if the operation is observed in a non-terminal state again, clear its poll count + delete(display.terminalOperationPollCounts, *operation.ID) + } + + return nil + } + + if operation.Properties.Timestamp == nil || + operation.Properties.ProvisioningOperation == nil || + operation.Properties.TargetResource == nil || + operation.Properties.TargetResource.ID == nil { + return nil + } - if !display.displayedResources[resourceId] { - switch *operations[i].Properties.ProvisioningState { + if *operation.Properties.ProvisioningOperation == armresources.ProvisioningOperationCreate && + operation.Properties.Timestamp.After(*queryStart) && + !display.displayedResources[*operation.Properties.TargetResource.ID] { + switch *operation.Properties.ProvisioningState { case string(armresources.ProvisioningStateSucceeded): - newlyDeployedResources = append(newlyDeployedResources, operations[i]) + newlyDeployedResources = append(newlyDeployedResources, operation) case string(armresources.ProvisioningStateRunning): - runningDeployments = append(runningDeployments, operations[i]) + runningDeployments = append(runningDeployments, operation) case string(armresources.ProvisioningStateFailed): - newlyFailedResources = append(newlyFailedResources, operations[i]) + newlyFailedResources = append(newlyFailedResources, operation) } } - } + + return nil + }) + if err != nil { + // Status display is best-effort activity. + return err } sort.Slice(newlyDeployedResources, func(i int, j int) bool { @@ -159,7 +190,6 @@ func (display *ProvisioningProgressDisplay) ReportProgress( display.logNewlyCreatedResources(ctx, displayedResources, runningDeployments) return nil } - func (display *ProvisioningProgressDisplay) logNewlyCreatedResources( ctx context.Context, resources []*armresources.DeploymentOperation, @@ -201,7 +231,7 @@ func (display *ProvisioningProgressDisplay) logNewlyCreatedResources( resourceTypeName, *resource.Properties.TargetResource.ResourceName) - display.displayedResources[*resource.Properties.TargetResource.ResourceName] = true + display.displayedResources[*resource.Properties.TargetResource.ID] = true } // update progress inProgress := []string{} @@ -227,10 +257,18 @@ func (display *ProvisioningProgressDisplay) logNewlyCreatedResources( return } + // ensure stable ordering + slices.Sort(inProgress) + + message := "Creating/Updating resources" if len(inProgress) > 0 { - display.console.ShowSpinner(ctx, - fmt.Sprintf("Creating/Updating resources (%s)", strings.Join(inProgress, ", ")), input.Step) - } else { - display.console.ShowSpinner(ctx, "Creating/Updating resources", input.Step) + message = fmt.Sprintf("%s (%s)", message, strings.Join(inProgress, ", ")) } + + // only update the spinner message if it has changed, to avoid updates which can cause flickering + if message != display.lastSpinnerMessage { + display.console.ShowSpinner(ctx, message, input.Step) + } + + display.lastSpinnerMessage = message } diff --git a/cli/azd/pkg/infra/provisioning_progress_display_test.go b/cli/azd/pkg/infra/provisioning_progress_display_test.go index 4c6b5289776..7af0b74994e 100644 --- a/cli/azd/pkg/infra/provisioning_progress_display_test.go +++ b/cli/azd/pkg/infra/provisioning_progress_display_test.go @@ -7,6 +7,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -27,12 +28,18 @@ type mockResourceManager struct { operations []*armresources.DeploymentOperation } -func (mock *mockResourceManager) GetDeploymentResourceOperations( +func (mock *mockResourceManager) WalkDeploymentOperations( ctx context.Context, target Deployment, - startTime *time.Time, -) ([]*armresources.DeploymentOperation, error) { - return mock.operations, nil + fn WalkDeploymentOperationFunc, +) error { + for _, operation := range mock.operations { + if err := fn(ctx, operation); err != nil && !errors.Is(err, SkipExpand) { + return err + } + } + + return nil } func (mock *mockResourceManager) GetResourceTypeDisplayName( @@ -152,3 +159,106 @@ func TestReportProgress(t *testing.T) { require.NoError(t, err) assert.Len(t, mockContext.Console.Output(), outputLength) } + +type walkSkipAwareResourceManager struct { + nestedOperation *armresources.DeploymentOperation + childOperation *armresources.DeploymentOperation + childVisits int +} + +func (mock *walkSkipAwareResourceManager) WalkDeploymentOperations( + ctx context.Context, + target Deployment, + fn WalkDeploymentOperationFunc, +) error { + err := fn(ctx, mock.nestedOperation) + if err != nil { + if errors.Is(err, SkipExpand) { + return nil + } + + return err + } + + mock.childVisits++ + return fn(ctx, mock.childOperation) +} + +func (mock *walkSkipAwareResourceManager) GetResourceTypeDisplayName( + ctx context.Context, + subscriptionId string, + resourceId string, + resourceType azapi.AzureResourceType, +) (string, error) { + return string(resourceType), nil +} + +func (mock *walkSkipAwareResourceManager) GetResourceGroupsForEnvironment( + ctx context.Context, + subscriptionId string, + envName string, +) ([]*azapi.Resource, error) { + return []*azapi.Resource{}, nil +} + +func (mock *walkSkipAwareResourceManager) FindResourceGroupForEnvironment( + ctx context.Context, + subscriptionId string, + envName string, +) (string, error) { + return "", nil +} + +func TestReportProgressSkipsExpansionAfterTwoTerminalPolls(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + deploymentService := mockazapi.NewDeploymentsServiceFromMockContext(mockContext) + + scope := newSubscriptionScope(deploymentService, "SUBSCRIPTION_ID", "eastus2") + deployment := NewSubscriptionDeployment( + scope, + "DEPLOYMENT_NAME", + ) + mockAzDeploymentShow(t, *mockContext) + + walkRm := &walkSkipAwareResourceManager{ + nestedOperation: &armresources.DeploymentOperation{ + ID: to.Ptr("nested-operation-id"), + Properties: &armresources.DeploymentOperationProperties{ + ProvisioningOperation: to.Ptr(armresources.ProvisioningOperationCreate), + ProvisioningState: to.Ptr(string(armresources.ProvisioningStateSucceeded)), + TargetResource: &armresources.TargetResource{ + ResourceType: to.Ptr(string(azapi.AzureResourceTypeDeployment)), + ID: to.Ptr("/subscriptions/SUBSCRIPTION_ID/resourceGroups/resource-group-name/providers/" + + "Microsoft.Resources/deployments/nested"), + ResourceName: to.Ptr("nested"), + }, + Timestamp: to.Ptr(time.Now().UTC()), + }, + }, + childOperation: &armresources.DeploymentOperation{ + ID: to.Ptr("child-operation-id"), + Properties: &armresources.DeploymentOperationProperties{ + ProvisioningOperation: to.Ptr(armresources.ProvisioningOperationCreate), + ProvisioningState: to.Ptr(string(armresources.ProvisioningStateSucceeded)), + Duration: to.Ptr("PT1S"), + TargetResource: &armresources.TargetResource{ + ResourceType: to.Ptr(string(azapi.AzureResourceTypeWebSite)), + ID: to.Ptr("/subscriptions/SUBSCRIPTION_ID/resourceGroups/resource-group-name/providers/" + + "Microsoft.Web/sites/site"), + ResourceName: to.Ptr("site"), + }, + Timestamp: to.Ptr(time.Now().UTC()), + }, + }, + } + + progressDisplay := NewProvisioningProgressDisplay(walkRm, mockContext.Console, deployment) + startTime := time.Now().Add(-time.Minute) + + err := progressDisplay.ReportProgress(*mockContext.Context, &startTime) + require.NoError(t, err) + err = progressDisplay.ReportProgress(*mockContext.Context, &startTime) + require.NoError(t, err) + + require.Equal(t, 1, walkRm.childVisits) +}