Merge pull request #346 from mkumatag/fix/issue-300-cleanup-timeout

Fix: Add configurable timeout for VM deletion cleanup (#300)

Author: ppc64le-cloud-bot

builder/powervs/builder.go

@@ -5,6 +5,7 @@ package powervs
 
 import (
 	"context"
+	"time"
 
 	"github.com/hashicorp/hcl/v2/hcldec"
 	"github.com/hashicorp/packer-plugin-sdk/common"
@@ -94,6 +95,12 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack
 
 	var steps []multistep.Step
 
+	// Parse cleanup timeout
+	cleanupTimeout, _ := time.ParseDuration(b.config.CleanupTimeout)
+	if cleanupTimeout == 0 {
+		cleanupTimeout = 10 * time.Minute // Default
+	}
+
 	steps = append(steps,
 		&StepImageBaseImage{
 			Source: b.config.Source,
@@ -103,9 +110,10 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack
 			DHCPNetwork: b.config.DHCPNetwork,
 		},
 		&StepCreateInstance{
-			InstanceName: b.config.InstanceName,
-			KeyPairName:  b.config.KeyPairName,
-			UserData:     b.config.UserData,
+			InstanceName:   b.config.InstanceName,
+			KeyPairName:    b.config.KeyPairName,
+			UserData:       b.config.UserData,
+			CleanupTimeout: cleanupTimeout,
 		},
 		&communicator.StepConnect{
 			Config:    &b.config.RunConfig.Comm,

builder/powervs/builder.hcl2spec.go

@@ -4,6 +4,9 @@
 package common
 
 import (
+	"fmt"
+	"time"
+
 	"github.com/hashicorp/packer-plugin-sdk/communicator"
 	"github.com/hashicorp/packer-plugin-sdk/template/interpolate"
 )
@@ -48,12 +51,29 @@ type RunConfig struct {
 	Source       Source   `mapstructure:"source" required:"true"`
 	Capture      Capture  `mapstructure:"capture" required:"true"`
 
+	// CleanupTimeout specifies the maximum time to wait for instance deletion during cleanup.
+	// If the instance is not deleted within this time, cleanup will fail gracefully with a warning.
+	// Format: duration string (e.g., "10m", "15m30s")
+	// Default: 10 minutes
+	CleanupTimeout string `mapstructure:"cleanup_timeout" required:"false"`
+
 	// Communicator settings
 	Comm communicator.Config `mapstructure:",squash"`
 }
 
 func (c *RunConfig) Prepare(ctx *interpolate.Context) []error {
 	// Validation
 	errs := c.Comm.Prepare(ctx)
+
+	// Set default cleanup timeout if not specified
+	if c.CleanupTimeout == "" {
+		c.CleanupTimeout = "10m"
+	}
+
+	// Validate cleanup timeout format
+	if _, err := time.ParseDuration(c.CleanupTimeout); err != nil {
+		errs = append(errs, fmt.Errorf("invalid cleanup_timeout format: %s (use format like '10m', '15m30s')", c.CleanupTimeout))
+	}
+
 	return errs
 }

builder/powervs/common/run_config.go

@@ -14,10 +14,18 @@ import (
 	packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
 )
 
+const (
+	// DefaultCleanupTimeout is the default maximum time to wait for instance deletion
+	DefaultCleanupTimeout = 10 * time.Minute
+	// CleanupPollInterval is how often to check instance deletion status
+	CleanupPollInterval = 10 * time.Second
+)
+
 type StepCreateInstance struct {
-	InstanceName string
-	KeyPairName  string
-	UserData     string
+	InstanceName   string
+	KeyPairName    string
+	UserData       string
+	CleanupTimeout time.Duration
 
 	doCleanup bool
 }
@@ -114,20 +122,75 @@ func (s *StepCreateInstance) Cleanup(state multistep.StateBag) {
 	ui.Say("Deleting the Instance")
 	instanceClient := state.Get("instanceClient").(*instance.IBMPIInstanceClient)
 	i := state.Get("instance").(*models.PVMInstance)
+
+	// Initiate deletion
 	err := instanceClient.Delete(*i.PvmInstanceID)
 	if err != nil {
 		ui.Error(fmt.Sprintf(
-			"Error cleaning up instance. Please delete the instance manually: %s", *i.ServerName))
+			"Error initiating instance deletion. Please delete the instance manually: %s (ID: %s)",
+			*i.ServerName, *i.PvmInstanceID))
+		ui.Error(fmt.Sprintf("Deletion error: %v", err))
+		return
+	}
+
+	// Wait for deletion with timeout
+	timeout := s.CleanupTimeout
+	if timeout == 0 {
+		timeout = DefaultCleanupTimeout
 	}
-	for {
+
+	begin := time.Now()
+	errorStateCount := 0
+	maxErrorStateRetries := 5 // Give some retries even in ERROR state
+
+	//nolint:staticcheck // SA1015 this disable staticcheck for the next line
+	err = pollUntil(time.Tick(CleanupPollInterval), time.After(timeout), func() (bool, error) {
 		in, err := instanceClient.Get(*i.PvmInstanceID)
-		if err == nil {
-			ui.Message(fmt.Sprintf("VM still exists, state: %s", *in.Status))
-			time.Sleep(10 * time.Second)
-			continue
-		} else {
-			ui.Message("instance deleted successfully")
-			break
+
+		// Instance not found means it was successfully deleted
+		if err != nil {
+			ui.Message("Instance deleted successfully")
+			return true, nil
+		}
+
+		// Instance still exists, check its state
+		currentState := *in.Status
+		elapsed := time.Since(begin).Round(time.Second)
+		ui.Message(fmt.Sprintf("VM still exists, state: %s (elapsed: %s)", currentState, elapsed))
+
+		// If instance is in ERROR state, warn but continue for a few retries
+		// Sometimes instances in ERROR state can still be deleted
+		if currentState == "ERROR" {
+			errorStateCount++
+			if errorStateCount == 1 {
+				ui.Message("Warning: Instance entered ERROR state during deletion")
+				ui.Message("Continuing to wait for deletion (may require manual cleanup)")
+			}
+			if errorStateCount >= maxErrorStateRetries {
+				ui.Error(fmt.Sprintf(
+					"Instance has been in ERROR state for %d checks. Deletion may have failed.",
+					errorStateCount))
+				ui.Error(fmt.Sprintf(
+					"Please verify instance status manually: %s (ID: %s)",
+					*i.ServerName, *i.PvmInstanceID))
+				// Don't return error, let timeout handle it
+			}
+		}
+
+		return false, nil
+	})
+
+	if err != nil {
+		// Timeout occurred
+		ui.Error(fmt.Sprintf(
+			"Timed out waiting for instance deletion after %s", timeout))
+		ui.Error(fmt.Sprintf(
+			"Instance may need manual cleanup: %s (ID: %s)",
+			*i.ServerName, *i.PvmInstanceID))
+
+		// Try to get final state
+		if finalInstance, getErr := instanceClient.Get(*i.PvmInstanceID); getErr == nil {
+			ui.Error(fmt.Sprintf("Final instance state: %s", *finalInstance.Status))
 		}
 	}
 }

builder/powervs/step_create_instance.go

@@ -2,6 +2,13 @@
 
 - `subnet_ids` ([]string) - Subnet I Ds
 
+- `user_data` (string) - User Data
+
 - `dhcp_network` (bool) - DHCP Network
 
+- `cleanup_timeout` (string) - CleanupTimeout specifies the maximum time to wait for instance deletion during cleanup.
+  If the instance is not deleted within this time, cleanup will fail gracefully with a warning.
+  Format: duration string (e.g., "10m", "15m30s")
+  Default: 10 minutes
+
 <!-- End of code generated from the comments of the RunConfig struct in builder/powervs/common/run_config.go; -->