- Implement timeouts for CRUD operations for ports

- Add acceptance tests

Signed-off-by: Ayush Rangwala <[email protected]>

equinix/data_source_metal_port.go

@@ -6,7 +6,7 @@ import (
 
 func dataSourceMetalPort() *schema.Resource {
 	return &schema.Resource{
-		Read: resourceMetalPortRead,
+		ReadWithoutTimeout: diagnosticsWrapper(resourceMetalPortRead),
 
 		Schema: map[string]*schema.Schema{
 			"port_id": {

equinix/port_helpers.go

@@ -1,16 +1,17 @@
 package equinix
 
 import (
+	"context"
 	"fmt"
 	"strings"
 	"time"
 
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/packethost/packngo"
+	"github.com/pkg/errors"
 )
 
-type portVlanAction func(*packngo.PortAssignRequest) (*packngo.Port, *packngo.Response, error)
-
 type ClientPortResource struct {
 	Client   *packngo.Client
 	Port     *packngo.Port
@@ -126,20 +127,7 @@ func specifiedVlanIds(d *schema.ResourceData) []string {
 	return []string{}
 }
 
-func processVlansOnPort(port *packngo.Port, vlanIds []string, f portVlanAction) (*packngo.Port, error) {
-	par := packngo.PortAssignRequest{PortID: port.ID}
-	for _, vId := range vlanIds {
-		par.VirtualNetworkID = vId
-		var err error
-		port, _, err = f(&par)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return port, nil
-}
-
-func batchVlans(removeOnly bool) func(*ClientPortResource) error {
+func batchVlans(ctx context.Context, start time.Time, removeOnly bool) func(*ClientPortResource) error {
 	return func(cpr *ClientPortResource) error {
 		var vlansToAssign []string
 		var currentNative string
@@ -171,35 +159,53 @@ func batchVlans(removeOnly bool) func(*ClientPortResource) error {
 				Native: &native,
 			})
 		}
-		return createAndWaitForBatch(cpr.Client, cpr.Port.ID, vacr)
+		return createAndWaitForBatch(ctx, start, cpr, vacr)
 	}
 }
 
-func createAndWaitForBatch(c *packngo.Client, portID string, vacr *packngo.VLANAssignmentBatchCreateRequest) error {
+func createAndWaitForBatch(ctx context.Context, start time.Time, cpr *ClientPortResource, vacr *packngo.VLANAssignmentBatchCreateRequest) error {
 	if len(vacr.VLANAssignments) == 0 {
 		return nil
 	}
+
+	portID := cpr.Port.ID
+	c := cpr.Client
+
 	b, _, err := c.VLANAssignments.CreateBatch(portID, vacr, nil)
 	if err != nil {
 		return fmt.Errorf("vlan assignment batch could not be created: %w", err)
 	}
 
-	// 15 minutes = 180 * 5sec-retry
-	for i := 0; i < 180; i++ {
-		<-time.After(5 * time.Second)
-		b, _, err := c.VLANAssignments.GetBatch(portID, b.ID, nil)
-		if err != nil {
-			return fmt.Errorf("vlan assignment batch %s could not be polled: %w", b.ID, err)
-		}
-		if b.State == packngo.VLANAssignmentBatchCompleted {
-			return nil
-		}
-		if b.State == packngo.VLANAssignmentBatchFailed {
-			return fmt.Errorf("vlan assignment batch %s provisioning failed: %s", b.ID, strings.Join(b.ErrorMessages, "; "))
-		}
+	deadline, _ := ctx.Deadline()
+	// originally set timeout in ctx by TF
+	ctxTimeout := deadline.Sub(start)
+
+	stateChangeConf := &retry.StateChangeConf{
+		Delay:      5 * time.Second,
+		Pending:    []string{string(packngo.VLANAssignmentBatchQueued), string(packngo.VLANAssignmentBatchInProgress)},
+		Target:     []string{string(packngo.VLANAssignmentBatchCompleted)},
+		MinTimeout: 5 * time.Second,
+		Timeout:    ctxTimeout - time.Since(start) - 30*time.Second,
+		Refresh: func() (result interface{}, state string, err error) {
+			b, _, err := c.VLANAssignments.GetBatch(portID, b.ID, nil)
+			switch b.State {
+			case packngo.VLANAssignmentBatchFailed:
+				return b, string(packngo.VLANAssignmentBatchFailed),
+					fmt.Errorf("vlan assignment batch %s provisioning failed: %s", b.ID, strings.Join(b.ErrorMessages, "; "))
+			case packngo.VLANAssignmentBatchCompleted:
+				return b, string(packngo.VLANAssignmentBatchCompleted), nil
+			default:
+				if err != nil {
+					return b, "", fmt.Errorf("vlan assignment batch %s could not be polled: %w", b.ID, err)
+				}
+				return b, string(b.State), err
+			}
+		},
 	}
-
-	return fmt.Errorf("vlan assignment batch %s is not complete after timeout", b.ID)
+	if _, err = stateChangeConf.WaitForStateContext(ctx); err != nil {
+		return errors.Wrapf(err, "vlan assignment batch %s is not complete after timeout", b.ID)
+	}
+	return nil
 }
 
 func updateNativeVlan(cpr *ClientPortResource) error {

equinix/resource_metal_port.go

@@ -1,6 +1,7 @@
 package equinix
 
 import (
+	"context"
 	"log"
 	"time"
 
@@ -25,13 +26,13 @@ func resourceMetalPort() *schema.Resource {
 			Update: schema.DefaultTimeout(20 * time.Minute),
 			Delete: schema.DefaultTimeout(20 * time.Minute),
 		},
-		Read: resourceMetalPortRead,
+		ReadWithoutTimeout: diagnosticsWrapper(resourceMetalPortRead),
 		// Create and Update are the same func
-		Create: resourceMetalPortUpdate,
-		Update: resourceMetalPortUpdate,
-		Delete: resourceMetalPortDelete,
+		CreateContext: diagnosticsWrapper(resourceMetalPortUpdate),
+		UpdateContext: diagnosticsWrapper(resourceMetalPortUpdate),
+		DeleteContext: diagnosticsWrapper(resourceMetalPortDelete),
 		Importer: &schema.ResourceImporter{
-			State: schema.ImportStatePassthrough,
+			StateContext: schema.ImportStatePassthroughContext,
 		},
 
 		Schema: map[string]*schema.Schema{
@@ -116,31 +117,32 @@ func resourceMetalPort() *schema.Resource {
 	}
 }
 
-func resourceMetalPortUpdate(d *schema.ResourceData, meta interface{}) error {
+func resourceMetalPortUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) error {
+	start := time.Now()
 	cpr, _, err := getClientPortResource(d, meta)
 	if err != nil {
 		return friendlyError(err)
 	}
 
 	for _, f := range [](func(*ClientPortResource) error){
 		portSanityChecks,
-		batchVlans(true),
+		batchVlans(ctx, start, true),
 		makeDisbond,
 		convertToL2,
 		makeBond,
 		convertToL3,
-		batchVlans(false),
+		batchVlans(ctx, start, false),
 		updateNativeVlan,
 	} {
 		if err := f(cpr); err != nil {
 			return friendlyError(err)
 		}
 	}
 
-	return resourceMetalPortRead(d, meta)
+	return resourceMetalPortRead(ctx, d, meta)
 }
 
-func resourceMetalPortRead(d *schema.ResourceData, meta interface{}) error {
+func resourceMetalPortRead(ctx context.Context, d *schema.ResourceData, meta interface{}) error {
 	meta.(*Config).addModuleToMetalUserAgent(d)
 	client := meta.(*Config).metal
 
@@ -195,9 +197,10 @@ func resourceMetalPortRead(d *schema.ResourceData, meta interface{}) error {
 	return setMap(d, m)
 }
 
-func resourceMetalPortDelete(d *schema.ResourceData, meta interface{}) error {
+func resourceMetalPortDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) error {
 	resetRaw, resetOk := d.GetOk("reset_on_delete")
 	if resetOk && resetRaw.(bool) {
+		start := time.Now()
 		cpr, resp, err := getClientPortResource(d, meta)
 		if ignoreResponseErrors(httpForbidden, httpNotFound)(resp, err) != nil {
 			return err
@@ -219,7 +222,7 @@ func resourceMetalPortDelete(d *schema.ResourceData, meta interface{}) error {
 			return err
 		}
 		for _, f := range [](func(*ClientPortResource) error){
-			batchVlans(true),
+			batchVlans(ctx, start, true),
 			makeBond,
 			convertToL3,
 		} {

equinix/resource_metal_port_acc_test.go

@@ -7,7 +7,13 @@ import (
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
+	"github.com/packethost/packngo"
+)
+
+var (
+	matchErrPortReadyTimeout = regexp.MustCompile(".* timeout while waiting for state to become 'completed'.*")
 )
 
 func confAccMetalPort_base(name string) string {
@@ -194,12 +200,44 @@ resource "equinix_metal_vlan" "test2" {
 `, confAccMetalPort_base(name))
 }
 
+func confAccMetalPort_HybridBonded_timeout(rInt int, name, createTimeout, updateTimeout string) string {
+	if createTimeout == "" {
+		createTimeout = "20m"
+	}
+	if updateTimeout == "" {
+		updateTimeout = "20m"
+	}
+
+	return fmt.Sprintf(`
+%s
+
+resource "equinix_metal_port" "bond0" {
+  port_id  = local.bond0_id
+  layer2   = false
+  bonded   = true
+  reset_on_delete = true
+  vlan_ids = [equinix_metal_vlan.test.id]
+  timeouts {
+    create = "%s"
+	update = "%s"
+  }
+  depends_on = [equinix_metal_vlan.test]
+}
+
+resource "equinix_metal_vlan" "test" {
+  description = "tfacc-vlan test-%d"
+  metro       = equinix_metal_device.test.metro
+  project_id  = equinix_metal_project.test.id
+}
+`, confAccMetalPort_base(name), createTimeout, updateTimeout, rInt)
+}
+
 func TestAccMetalPort_hybridBondedVxlan(t *testing.T) {
 	rs := acctest.RandString(10)
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:          func() { testAccPreCheck(t) },
 		ExternalProviders: testExternalProviders,
-		Providers:         testAccProviders,
+		ProviderFactories: testAccProviderFactories,
 		CheckDestroy:      testAccMetalPortDestroyed,
 		Steps: []resource.TestStep{
 			{
@@ -228,7 +266,7 @@ func TestAccMetalPort_L2IndividualNativeVlan(t *testing.T) {
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:          func() { testAccPreCheck(t) },
 		ExternalProviders: testExternalProviders,
-		Providers:         testAccProviders,
+		ProviderFactories: testAccProviderFactories,
 		CheckDestroy:      testAccMetalPortDestroyed,
 		Steps: []resource.TestStep{
 			{
@@ -261,7 +299,7 @@ func testAccMetalPortTemplate(t *testing.T, conf func(string) string, expectedTy
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:          func() { testAccPreCheck(t) },
 		ExternalProviders: testExternalProviders,
-		Providers:         testAccProviders,
+		ProviderFactories: testAccProviderFactories,
 		CheckDestroy:      testAccMetalPortDestroyed,
 		Steps: []resource.TestStep{
 			{
@@ -340,3 +378,113 @@ func testAccMetalPortDestroyed(s *terraform.State) error {
 	}
 	return nil
 }
+
+func testAccWaitForPortActive(deviceName, portName string) resource.ImportStateIdFunc {
+	return func(state *terraform.State) (string, error) {
+		rs, ok := state.RootModule().Resources[deviceName]
+		if !ok {
+			return "", fmt.Errorf("Device Not found in the state: %s", deviceName)
+		}
+		if rs.Primary.ID == "" {
+			return "", fmt.Errorf("No Record ID is set")
+		}
+
+		meta := testAccProvider.Meta()
+		rd := new(schema.ResourceData)
+		meta.(*Config).addModuleToMetalUserAgent(rd)
+		client := meta.(*Config).metal
+		device, _, err := client.Devices.Get(rs.Primary.ID, &packngo.GetOptions{Includes: []string{"ports"}})
+		if err != nil {
+			return "", fmt.Errorf("error while fetching device with Id [%s], error: %w", rs.Primary.ID, err)
+		}
+		if device == nil {
+			return "", fmt.Errorf("Not able to find devices with Id [%s]", rs.Primary.ID)
+		}
+		if len(device.NetworkPorts) == 0 {
+			return "", fmt.Errorf("Found no ports for the device with Id [%s]", rs.Primary.ID)
+		}
+
+		for _, port := range device.NetworkPorts {
+			if port.Name == portName {
+				return port.ID, nil
+			}
+		}
+
+		return "", fmt.Errorf("No port with name [%s] found", portName)
+	}
+}
+
+func TestAccMetalPortCreate_hybridBonded_timeout(t *testing.T) {
+	rs := acctest.RandString(10)
+	rInt := acctest.RandInt()
+	deviceName := "equinix_metal_device.test"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:          func() { testAccPreCheck(t) },
+		ExternalProviders: testExternalProviders,
+		ProviderFactories: testAccProviderFactories,
+		CheckDestroy:      testAccMetalPortDestroyed,
+		Steps: []resource.TestStep{
+			{
+				Config:      confAccMetalPort_HybridBonded_timeout(rInt, rs, "5s", ""),
+				ExpectError: matchErrPortReadyTimeout,
+			},
+			{
+				/**
+				Step 1 errors out, state doesnt have port, need to import that in the state before deleting
+				*/
+				ResourceName:       "equinix_metal_port.bond0",
+				ImportState:        true,
+				ImportStateIdFunc:  testAccWaitForPortActive(deviceName, "bond0"),
+				ImportStatePersist: true,
+			},
+			{
+				Config:  confAccMetalPort_HybridBonded_timeout(rInt, rs, "5s", ""),
+				Destroy: true,
+			},
+		},
+	})
+}
+
+func TestAccMetalPortUpdate_hybridBonded_timeout(t *testing.T) {
+	rs := acctest.RandString(10)
+	rInt := acctest.RandInt()
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:          func() { testAccPreCheck(t) },
+		ExternalProviders: testExternalProviders,
+		ProviderFactories: testAccProviderFactories,
+		CheckDestroy:      testAccMetalPortDestroyed,
+		Steps: []resource.TestStep{
+			{
+				Config: confAccMetalPort_HybridBonded_timeout(rInt, rs, "", "5s"),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("equinix_metal_port.bond0", "name", "bond0"),
+					resource.TestCheckResourceAttr("equinix_metal_port.bond0", "type", "NetworkBondPort"),
+					resource.TestCheckResourceAttrSet("equinix_metal_port.bond0", "bonded"),
+					resource.TestCheckResourceAttrSet("equinix_metal_port.bond0", "disbond_supported"),
+					resource.TestCheckResourceAttrSet("equinix_metal_port.bond0", "port_id"),
+					resource.TestCheckResourceAttr("equinix_metal_port.bond0", "network_type", "hybrid-bonded"),
+				),
+			},
+			{
+				Config:      confAccMetalPort_HybridBonded_timeout(rInt+1, rs, "", "5s"),
+				ExpectError: matchErrPortReadyTimeout,
+			},
+			{
+				ResourceName: "equinix_metal_port.bond0",
+				ImportState:  true,
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("equinix_metal_port.bond0", "network_type", "layer3"),
+				),
+			},
+			{
+				Config: confAccMetalPort_HybridBonded_timeout(rInt+1, rs, "", ""),
+			},
+			{
+				Config:  confAccMetalPort_HybridBonded_timeout(rInt+1, rs, "", ""),
+				Destroy: true,
+			},
+		},
+	})
+}