From 8d13a4beb314b0afa9e335e18c711bf39120f9b3 Mon Sep 17 00:00:00 2001 From: Vikyath Harekal Date: Fri, 2 May 2025 09:52:00 +0000 Subject: [PATCH] feat: [CDE-753]: Fix backend handling of gitspace deletion requests (#3745) * feat: [CDE-753]: Fix lint * feat: [CDE-753]: Fix backend handling of gitspace deletion requests --- .../infrastructure/trigger_infra_event.go | 19 ++-- .../orchestrator/orchestrator_trigger.go | 86 ++++++++++++------- app/services/gitspace/action_delete.go | 2 +- infraprovider/docker_provider.go | 1 + infraprovider/infra_provider.go | 1 + 5 files changed, 67 insertions(+), 42 deletions(-) diff --git a/app/gitspace/infrastructure/trigger_infra_event.go b/app/gitspace/infrastructure/trigger_infra_event.go index a72beaa9b..dc38c6ced 100644 --- a/app/gitspace/infrastructure/trigger_infra_event.go +++ b/app/gitspace/infrastructure/trigger_infra_event.go @@ -100,7 +100,7 @@ func (i InfraProvisioner) TriggerInfraEventWithOpts( return err } - _, configMetadata, err := i.getAllParamsFromDB(ctx, gitspaceConfig.InfraProviderResource, infraProvider) + allParams, configMetadata, err := i.getAllParamsFromDB(ctx, gitspaceConfig.InfraProviderResource, infraProvider) if err != nil { return fmt.Errorf("could not get all params from DB while provisioning: %w", err) } @@ -126,6 +126,8 @@ func (i InfraProvisioner) TriggerInfraEventWithOpts( gitspaceConfig, opts.RequiredGitspacePorts, stoppedInfra, + configMetadata, + allParams, ) } return i.provisionExistingInfrastructure( @@ -145,9 +147,10 @@ func (i InfraProvisioner) TriggerInfraEventWithOpts( *infra, opts.CanDeleteUserData, configMetadata, + allParams, ) } - return infraProvider.Deprovision(ctx, *infra, opts.CanDeleteUserData, configMetadata) + return infraProvider.Deprovision(ctx, *infra, opts.CanDeleteUserData, configMetadata, allParams) case enum.InfraEventCleanup: return infraProvider.CleanupInstanceResources(ctx, *infra) @@ -167,6 +170,8 @@ func (i InfraProvisioner) provisionNewInfrastructure( gitspaceConfig types.GitspaceConfig, requiredGitspacePorts []types.GitspacePort, stoppedInfra types.Infrastructure, + configMetadata map[string]any, + allParams []types.InfraProviderParameter, ) error { // Logic for new provisioning... infraProvisionedLatest, _ := i.infraProvisionedStore.FindLatestByGitspaceInstanceID( @@ -185,12 +190,8 @@ func (i InfraProvisioner) provisionNewInfrastructure( } infraProviderResource := gitspaceConfig.InfraProviderResource - allParams, configMetadata, err := i.getAllParamsFromDB(ctx, infraProviderResource, infraProvider) - if err != nil { - return fmt.Errorf("could not get all params from DB while provisioning: %w", err) - } - err = infraProvider.ValidateParams(allParams) + err := infraProvider.ValidateParams(allParams) if err != nil { return fmt.Errorf("invalid provisioning params %v: %w", infraProviderResource.Metadata, err) } @@ -210,6 +211,7 @@ func (i InfraProvisioner) provisionNewInfrastructure( InputParameters: allParams, ConfigMetadata: configMetadata, InstanceInfo: stoppedInfra.InstanceInfo, + Status: enum.InfraStatusPending, } responseMetadata, err := json.Marshal(infrastructure) if err != nil { @@ -315,6 +317,7 @@ func (i InfraProvisioner) deprovisionNewInfrastructure( infra types.Infrastructure, canDeleteUserData bool, configMetadata map[string]any, + params []types.InfraProviderParameter, ) error { infraProvisionedLatest, err := i.infraProvisionedStore.FindLatestByGitspaceInstanceID( ctx, gitspaceConfig.GitspaceInstance.ID) @@ -328,7 +331,7 @@ func (i InfraProvisioner) deprovisionNewInfrastructure( return nil } - err = infraProvider.Deprovision(ctx, infra, canDeleteUserData, configMetadata) + err = infraProvider.Deprovision(ctx, infra, canDeleteUserData, configMetadata, params) if err != nil { return fmt.Errorf("unable to trigger deprovision infra %+v: %w", infra, err) } diff --git a/app/gitspace/orchestrator/orchestrator_trigger.go b/app/gitspace/orchestrator/orchestrator_trigger.go index 9639f8479..f8cc7e072 100644 --- a/app/gitspace/orchestrator/orchestrator_trigger.go +++ b/app/gitspace/orchestrator/orchestrator_trigger.go @@ -344,17 +344,18 @@ func (o Orchestrator) TriggerCleanupInstanceResources(ctx context.Context, gitsp return nil } -// TriggerDeleteGitspace removes the Gitspace container and triggers infra deprovisioning to deprovision +// TriggerStopAndDeleteGitspace removes the Gitspace container and triggers infra deprovisioning to deprovision // the infra resources. // canDeleteUserData = false -> trigger deprovision of all resources except storage associated to user data. // canDeleteUserData = true -> trigger deprovision of all resources. -func (o Orchestrator) TriggerDeleteGitspace( +func (o Orchestrator) TriggerStopAndDeleteGitspace( ctx context.Context, gitspaceConfig types.GitspaceConfig, canDeleteUserData bool, ) error { infra, err := o.getProvisionedInfra(ctx, gitspaceConfig, []enum.InfraStatus{ + enum.InfraStatusPending, enum.InfraStatusProvisioned, enum.InfraStatusStopped, enum.InfraStatusDestroyed, @@ -368,6 +369,8 @@ func (o Orchestrator) TriggerDeleteGitspace( } if err = o.stopAndRemoveGitspaceContainer(ctx, gitspaceConfig, *infra, canDeleteUserData); err != nil { log.Warn().Msgf("error stopping and removing gitspace container: %v", err) + // If stop fails, delete the gitspace anyway + return o.triggerDeleteGitspace(ctx, gitspaceConfig, infra, canDeleteUserData) } // TODO: Add a job for cleanup of infra if stop fails @@ -375,22 +378,56 @@ func (o Orchestrator) TriggerDeleteGitspace( "Checking and force deleting the infra if required for gitspace instance %s", gitspaceConfig.GitspaceInstance.Identifier, ) - ticker := time.NewTicker(60 * time.Second) - timeout := time.After(15 * time.Minute) + if err = o.waitForGitspaceCleanupOrTimeout(ctx, gitspaceConfig, 15*time.Minute, 60*time.Second); err != nil { + return o.triggerDeleteGitspace(ctx, gitspaceConfig, infra, canDeleteUserData) + } + return nil +} + +func (o Orchestrator) triggerDeleteGitspace( + ctx context.Context, + gitspaceConfig types.GitspaceConfig, + infra *types.Infrastructure, + canDeleteUserData bool, +) error { + opts := infrastructure.InfraEventOpts{CanDeleteUserData: true} + err := o.infraProvisioner.TriggerInfraEventWithOpts( + ctx, + enum.InfraEventDeprovision, + gitspaceConfig, + infra, + opts, + ) + if err != nil { + if canDeleteUserData { + o.emitGitspaceEvent(ctx, gitspaceConfig, enum.GitspaceEventTypeInfraDeprovisioningFailed) + } else { + o.emitGitspaceEvent(ctx, gitspaceConfig, enum.GitspaceEventTypeInfraResetFailed) + } + return fmt.Errorf( + "cannot trigger deprovision infrastructure with gitspace identifier %s: %w", + gitspaceConfig.GitspaceInstance.Identifier, + err, + ) + } + return nil +} + +func (o Orchestrator) waitForGitspaceCleanupOrTimeout( + ctx context.Context, + gitspaceConfig types.GitspaceConfig, + timeoutDuration time.Duration, + tickInterval time.Duration, +) error { + ticker := time.NewTicker(tickInterval) defer ticker.Stop() - ch := make(chan error) + + timeout := time.After(timeoutDuration) for { select { - case msg := <-ch: - if msg == nil { - return msg - } case <-ticker.C: - instance, err := o.gitspaceInstanceStore.Find( - ctx, - gitspaceConfig.GitspaceInstance.ID, - ) + instance, err := o.gitspaceInstanceStore.Find(ctx, gitspaceConfig.GitspaceInstance.ID) if err != nil { return fmt.Errorf( "failed to find gitspace instance %s: %w", @@ -403,27 +440,10 @@ func (o Orchestrator) TriggerDeleteGitspace( return nil } case <-timeout: - opts := infrastructure.InfraEventOpts{CanDeleteUserData: true} - err := o.infraProvisioner.TriggerInfraEventWithOpts( - ctx, - enum.InfraEventDeprovision, - gitspaceConfig, - infra, - opts, + return fmt.Errorf( + "timeout waiting for gitspace cleanup for instance %s", + gitspaceConfig.GitspaceInstance.Identifier, ) - if err != nil { - if canDeleteUserData { - o.emitGitspaceEvent(ctx, gitspaceConfig, enum.GitspaceEventTypeInfraDeprovisioningFailed) - } else { - o.emitGitspaceEvent(ctx, gitspaceConfig, enum.GitspaceEventTypeInfraResetFailed) - } - return fmt.Errorf( - "cannot trigger deprovision infrastructure with gitspace identifier %s: %w", - gitspaceConfig.GitspaceInstance.Identifier, - err, - ) - } - return nil } } } diff --git a/app/services/gitspace/action_delete.go b/app/services/gitspace/action_delete.go index 689edb9aa..68a14bdbf 100644 --- a/app/services/gitspace/action_delete.go +++ b/app/services/gitspace/action_delete.go @@ -89,7 +89,7 @@ func (c *Service) RemoveGitspace(ctx context.Context, config types.GitspaceConfi ) } - if err := c.orchestrator.TriggerDeleteGitspace(ctx, config, canDeleteUserData); err != nil { + if err := c.orchestrator.TriggerStopAndDeleteGitspace(ctx, config, canDeleteUserData); err != nil { log.Ctx(ctx).Err(err).Msgf("error during triggering delete for gitspace instance %s", config.GitspaceInstance.Identifier) config.GitspaceInstance.State = enum.GitspaceInstanceStateError diff --git a/infraprovider/docker_provider.go b/infraprovider/docker_provider.go index 1ded15ee6..d7570681e 100644 --- a/infraprovider/docker_provider.go +++ b/infraprovider/docker_provider.go @@ -208,6 +208,7 @@ func (d DockerProvider) Deprovision( infra types.Infrastructure, canDeleteUserData bool, _ map[string]any, + _ []types.InfraProviderParameter, ) error { if canDeleteUserData { err := d.deleteVolume(ctx, infra) diff --git a/infraprovider/infra_provider.go b/infraprovider/infra_provider.go index bf0733604..e1f2ff738 100644 --- a/infraprovider/infra_provider.go +++ b/infraprovider/infra_provider.go @@ -67,6 +67,7 @@ type InfraProvider interface { infra types.Infrastructure, canDeleteUserData bool, configMetadata map[string]any, + params []types.InfraProviderParameter, ) error // AvailableParams provides a schema to define the infrastructure.