33 Fix reliability issues (#42)

* Add request run state prechecking

* Add response checking

* Add waits and retries for the different response codes. See issue https://github.com/terraform-providers/terraform-provider-skytap/issues/34

* Fail early on 422 if resource not busy otherwise retry.

* Cut Published service Update method and related structures

* Add requirement for vm to be stopped prior to interface delete

* Add environment busy check in place of vm ready check

* Add comment to clarify intention of running the environment after create

skytap/convert.go

@@ -47,3 +47,8 @@ func vmRunStateToPtr(vmRunState VMRunstate) *VMRunstate {
 func nicTypeToPtr(nicType NICType) *NICType {
 	return &nicType
 }
+
+// environmentRunStateToPtr returns a pointer to the passed EnvironmentRunstate.
+func environmentRunStateToPtr(environmentRunstate EnvironmentRunstate) *EnvironmentRunstate {
+	return &environmentRunstate
+}

skytap/environment.go

@@ -3,6 +3,8 @@ package skytap
 import (
 	"context"
 	"fmt"
+	"log"
+	"strings"
 )
 
 // Default URL paths
@@ -133,6 +135,7 @@ const (
 	EnvironmentRunstateStopped   EnvironmentRunstate = "stopped"
 	EnvironmentRunstateSuspended EnvironmentRunstate = "suspended"
 	EnvironmentRunstateRunning   EnvironmentRunstate = "running"
+	EnvironmentRunstateHalted    EnvironmentRunstate = "halted"
 	EnvironmentRunstateBusy      EnvironmentRunstate = "busy"
 )
 
@@ -183,7 +186,7 @@ func (s *EnvironmentsServiceClient) List(ctx context.Context) (*EnvironmentListR
 	}
 
 	var environmentsListResponse EnvironmentListResult
-	_, err = s.client.do(ctx, req, &environmentsListResponse.Value)
+	_, err = s.client.do(ctx, req, &environmentsListResponse.Value, nil, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -201,7 +204,7 @@ func (s *EnvironmentsServiceClient) Get(ctx context.Context, id string) (*Enviro
 	}
 
 	var environment Environment
-	_, err = s.client.do(ctx, req, &environment)
+	_, err = s.client.do(ctx, req, &environment, nil, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -210,31 +213,39 @@ func (s *EnvironmentsServiceClient) Get(ctx context.Context, id string) (*Enviro
 }
 
 // Create an environment
-func (s *EnvironmentsServiceClient) Create(ctx context.Context, request *CreateEnvironmentRequest) (*Environment, error) {
-	req, err := s.client.newRequest(ctx, "POST", environmentLegacyBasePath, request)
+func (s *EnvironmentsServiceClient) Create(ctx context.Context, opts *CreateEnvironmentRequest) (*Environment, error) {
+	req, err := s.client.newRequest(ctx, "POST", environmentLegacyBasePath, opts)
 	if err != nil {
 		return nil, err
 	}
 
 	var createdEnvironment Environment
-	_, err = s.client.do(ctx, req, &createdEnvironment)
+	_, err = s.client.do(ctx, req, &createdEnvironment, nil, opts)
 	if err != nil {
 		return nil, err
 	}
 
-	runstate := EnvironmentRunstateRunning
+	env, err := s.Get(ctx, *createdEnvironment.ID)
+	if err != nil {
+		return nil, err
+	}
+
+	var runstate *EnvironmentRunstate
+	if *env.VMCount > 0 {
+		runstate = environmentRunStateToPtr(EnvironmentRunstateRunning)
+	}
 
 	updateOpts := &UpdateEnvironmentRequest{
-		Name:            request.Name,
-		Description:     request.Description,
-		Owner:           request.Owner,
-		OutboundTraffic: request.OutboundTraffic,
-		Routable:        request.Routable,
-		SuspendOnIdle:   request.SuspendOnIdle,
-		SuspendAtTime:   request.SuspendAtTime,
-		ShutdownOnIdle:  request.ShutdownOnIdle,
-		ShutdownAtTime:  request.ShutdownAtTime,
-		Runstate:        &runstate,
+		Name:            opts.Name,
+		Description:     opts.Description,
+		Owner:           opts.Owner,
+		OutboundTraffic: opts.OutboundTraffic,
+		Routable:        opts.Routable,
+		SuspendOnIdle:   opts.SuspendOnIdle,
+		SuspendAtTime:   opts.SuspendAtTime,
+		ShutdownOnIdle:  opts.ShutdownOnIdle,
+		ShutdownAtTime:  opts.ShutdownAtTime,
+		Runstate:        runstate, // we are expecting the environment to start its VMs after creation
 	}
 
 	// update environment after creation to establish the resource information.
@@ -256,7 +267,7 @@ func (s *EnvironmentsServiceClient) Update(ctx context.Context, id string, updat
 	}
 
 	var environment Environment
-	_, err = s.client.do(ctx, req, &environment)
+	_, err = s.client.do(ctx, req, &environment, envRunStateNotBusy(id), updateEnvironment)
 	if err != nil {
 		return nil, err
 	}
@@ -272,10 +283,150 @@ func (s *EnvironmentsServiceClient) Delete(ctx context.Context, id string) error
 	if err != nil {
 		return err
 	}
-	_, err = s.client.do(ctx, req, nil)
+	_, err = s.client.do(ctx, req, nil, envRunStateNotBusy(id), nil)
 	if err != nil {
 		return err
 	}
 
 	return nil
 }
+
+func (payload *CreateEnvironmentRequest) compareResponse(ctx context.Context, c *Client, v interface{}, state *environmentVMRunState) (string, bool) {
+	if envOriginal, ok := v.(*Environment); ok {
+		env, err := c.Environments.Get(ctx, *envOriginal.ID)
+		if err != nil {
+			return requestNotAsExpected, false
+		}
+		logEnvironmentStatus(env)
+		log.Printf("[DEBUG] SDK environment runstate after create (%s)\n", *env.Runstate)
+		if *env.Runstate != EnvironmentRunstateBusy {
+			return "", true
+		}
+		return "environment not ready", false
+	}
+	log.Printf("[ERROR] SDK environment comparison not possible on (%v)\n", v)
+	return requestNotAsExpected, false
+}
+
+func (payload *UpdateEnvironmentRequest) compareResponse(ctx context.Context, c *Client, v interface{}, state *environmentVMRunState) (string, bool) {
+	if envOriginal, ok := v.(*Environment); ok {
+		env, err := c.Environments.Get(ctx, *envOriginal.ID)
+		if err != nil {
+			return requestNotAsExpected, false
+		}
+		logEnvironmentStatus(env)
+		actual := payload.buildComparison(env)
+		if payload.string() == actual.string() {
+			return "", true
+		}
+		return "environment not ready", false
+	}
+	log.Printf("[ERROR] SDK environment comparison not possible on (%v)\n", v)
+	return requestNotAsExpected, false
+}
+
+func (payload *UpdateEnvironmentRequest) buildComparison(env *Environment) UpdateEnvironmentRequest {
+	actual := UpdateEnvironmentRequest{}
+	if payload.Name != nil {
+		actual.Name = env.Name
+	}
+	if payload.Description != nil {
+		actual.Description = env.Description
+	}
+	if payload.Owner != nil {
+		actual.Owner = env.OwnerName
+	}
+	if payload.OutboundTraffic != nil {
+		actual.OutboundTraffic = env.OutboundTraffic
+	}
+	if payload.Routable != nil {
+		actual.Routable = env.Routable
+	}
+	if payload.SuspendOnIdle != nil {
+		actual.SuspendOnIdle = env.SuspendOnIdle
+	}
+	if payload.SuspendAtTime != nil {
+		actual.SuspendAtTime = env.SuspendAtTime
+	}
+	if payload.ShutdownOnIdle != nil {
+		actual.ShutdownOnIdle = env.ShutdownOnIdle
+	}
+	if payload.ShutdownAtTime != nil {
+		actual.ShutdownAtTime = env.ShutdownAtTime
+	}
+	if payload.Runstate != nil {
+		actual.Runstate = env.Runstate
+	}
+	return actual
+}
+
+func (payload *UpdateEnvironmentRequest) string() string {
+	name := ""
+	description := ""
+	owner := ""
+	outboundTraffic := ""
+	routable := ""
+	suspendOnIdle := ""
+	suspendAtTime := ""
+	shutdownOnIdle := ""
+	shutdownAtTime := ""
+	runstate := ""
+
+	if payload.Name != nil {
+		name = *payload.Name
+	}
+	if payload.Description != nil {
+		description = *payload.Description
+	}
+	if payload.Owner != nil {
+		owner = *payload.Owner
+	}
+	if payload.OutboundTraffic != nil {
+		outboundTraffic = fmt.Sprintf("%t", *payload.OutboundTraffic)
+	}
+	if payload.Routable != nil {
+		routable = fmt.Sprintf("%t", *payload.Routable)
+	}
+	if payload.SuspendOnIdle != nil {
+		suspendOnIdle = fmt.Sprintf("%d", *payload.SuspendOnIdle)
+	}
+	if payload.SuspendAtTime != nil {
+		suspendAtTime = *payload.SuspendAtTime
+	}
+	if payload.ShutdownOnIdle != nil {
+		shutdownOnIdle = fmt.Sprintf("%d", *payload.ShutdownOnIdle)
+	}
+	if payload.ShutdownAtTime != nil {
+		shutdownAtTime = *payload.ShutdownAtTime
+	}
+	if payload.Runstate != nil {
+		runstate = string(*payload.Runstate)
+	}
+	var sb strings.Builder
+	sb.WriteString(name)
+	sb.WriteString(description)
+	sb.WriteString(owner)
+	sb.WriteString(outboundTraffic)
+	sb.WriteString(routable)
+	sb.WriteString(suspendOnIdle)
+	sb.WriteString(suspendAtTime)
+	sb.WriteString(shutdownOnIdle)
+	sb.WriteString(shutdownAtTime)
+	sb.WriteString(runstate)
+	log.Printf("[DEBUG] SDK environment payload (%s)\n", sb.String())
+	return sb.String()
+}
+
+func logEnvironmentStatus(env *Environment) {
+	if env.RateLimited != nil && *env.RateLimited {
+		log.Printf("[INFO] SDK environment rate limiting detected\n")
+	}
+	if len(env.Errors) > 0 {
+		log.Printf("[INFO] SDK environment errors detected: (%s)\n",
+			strings.Join(env.Errors, ", "))
+	}
+	if len(env.ErrorDetails) > 0 {
+		log.Printf("[INFO] SDK environment errors detected: (%s)\n",
+			strings.Join(env.ErrorDetails, ", "))
+	}
+}