From dcbb93ee2bd05ad15486bc1e5f4d4223ecd22eb5 Mon Sep 17 00:00:00 2001 From: Ayush Rangwala Date: Mon, 28 Aug 2023 22:19:04 +0530 Subject: [PATCH 1/5] - Implement timeouts for CRUD operations for ports - Add acceptance tests Signed-off-by: Ayush Rangwala --- equinix/data_source_metal_port.go | 2 +- equinix/port_helpers.go | 72 ++++++----- equinix/resource_metal_port.go | 27 +++-- equinix/resource_metal_port_acc_test.go | 154 +++++++++++++++++++++++- go.mod | 1 + go.sum | 1 + 6 files changed, 208 insertions(+), 49 deletions(-) diff --git a/equinix/data_source_metal_port.go b/equinix/data_source_metal_port.go index bb448c197..dc0d0e373 100644 --- a/equinix/data_source_metal_port.go +++ b/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": { diff --git a/equinix/port_helpers.go b/equinix/port_helpers.go index 037294b5d..742198d7b 100644 --- a/equinix/port_helpers.go +++ b/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 { diff --git a/equinix/resource_metal_port.go b/equinix/resource_metal_port.go index 96567b034..53a36ec87 100644 --- a/equinix/resource_metal_port.go +++ b/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,7 +117,8 @@ 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) @@ -124,12 +126,12 @@ func resourceMetalPortUpdate(d *schema.ResourceData, meta interface{}) error { 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 { @@ -137,10 +139,10 @@ func resourceMetalPortUpdate(d *schema.ResourceData, meta interface{}) error { } } - 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, } { diff --git a/equinix/resource_metal_port_acc_test.go b/equinix/resource_metal_port_acc_test.go index a7bc59e9e..9a2097c14 100644 --- a/equinix/resource_metal_port_acc_test.go +++ b/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, + }, + }, + }) +} diff --git a/go.mod b/go.mod index 0ce4605c9..ae176069d 100644 --- a/go.mod +++ b/go.mod @@ -18,6 +18,7 @@ require ( github.com/hashicorp/terraform-plugin-docs v0.14.1 github.com/hashicorp/terraform-plugin-sdk/v2 v2.26.1 github.com/packethost/packngo v0.30.0 + github.com/pkg/errors v0.9.1 github.com/stretchr/testify v1.8.4 golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 golang.org/x/oauth2 v0.11.0 diff --git a/go.sum b/go.sum index 21478e5b5..5f9dcc7fb 100644 --- a/go.sum +++ b/go.sum @@ -532,6 +532,7 @@ github.com/oklog/run v1.0.0/go.mod h1:dlhp/R75TPv97u0XWUtDeV/lRKWPKSdTuV0TZvrmrQ github.com/packethost/packngo v0.30.0 h1:JVeTwbXXETsLTDQncUbYwIFpkOp/xevXrffM2HrFECI= github.com/packethost/packngo v0.30.0/go.mod h1:BT/XcdwLVmeMtGPbovnxCpnI1s9ylSE1cs/7pq007NE= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= From fdfb0f83ac501ddf530efa6abd6a59e41017caf5 Mon Sep 17 00:00:00 2001 From: Ayush Rangwala Date: Wed, 20 Sep 2023 22:48:33 +0530 Subject: [PATCH 2/5] fix: metal ports create timeout acc tests Signed-off-by: Ayush Rangwala --- equinix/resource_metal_port_acc_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/equinix/resource_metal_port_acc_test.go b/equinix/resource_metal_port_acc_test.go index 9a2097c14..71274d4f5 100644 --- a/equinix/resource_metal_port_acc_test.go +++ b/equinix/resource_metal_port_acc_test.go @@ -438,6 +438,16 @@ func TestAccMetalPortCreate_hybridBonded_timeout(t *testing.T) { ImportStateIdFunc: testAccWaitForPortActive(deviceName, "bond0"), ImportStatePersist: true, }, + { + ResourceName: "equinix_metal_port.bond0", + ImportState: true, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("equinix_metal_port.bond0", "network_type", "layer3"), + ), + }, + { + Config: confAccMetalPort_HybridBonded_timeout(rInt, rs, "5s", ""), + }, { Config: confAccMetalPort_HybridBonded_timeout(rInt, rs, "5s", ""), Destroy: true, From 67cbf064c9edab8b50e26dab019e8fb6e7a111ed Mon Sep 17 00:00:00 2001 From: Ayush Rangwala Date: Thu, 21 Sep 2023 19:36:50 +0530 Subject: [PATCH 3/5] Update defailt timeout from 20m to 30m --- equinix/resource_metal_port.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/equinix/resource_metal_port.go b/equinix/resource_metal_port.go index 53a36ec87..375897ec8 100644 --- a/equinix/resource_metal_port.go +++ b/equinix/resource_metal_port.go @@ -22,9 +22,9 @@ var ( func resourceMetalPort() *schema.Resource { return &schema.Resource{ Timeouts: &schema.ResourceTimeout{ - Create: schema.DefaultTimeout(20 * time.Minute), - Update: schema.DefaultTimeout(20 * time.Minute), - Delete: schema.DefaultTimeout(20 * time.Minute), + Create: schema.DefaultTimeout(30 * time.Minute), + Update: schema.DefaultTimeout(30 * time.Minute), + Delete: schema.DefaultTimeout(30 * time.Minute), }, ReadWithoutTimeout: diagnosticsWrapper(resourceMetalPortRead), // Create and Update are the same func From c63460cde704dbe6123983193895e438623e66f7 Mon Sep 17 00:00:00 2001 From: Ayush Rangwala Date: Thu, 21 Sep 2023 22:22:03 +0530 Subject: [PATCH 4/5] Use 20m default timeout again --- equinix/resource_metal_port.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/equinix/resource_metal_port.go b/equinix/resource_metal_port.go index 375897ec8..53a36ec87 100644 --- a/equinix/resource_metal_port.go +++ b/equinix/resource_metal_port.go @@ -22,9 +22,9 @@ var ( func resourceMetalPort() *schema.Resource { return &schema.Resource{ Timeouts: &schema.ResourceTimeout{ - Create: schema.DefaultTimeout(30 * time.Minute), - Update: schema.DefaultTimeout(30 * time.Minute), - Delete: schema.DefaultTimeout(30 * time.Minute), + Create: schema.DefaultTimeout(20 * time.Minute), + Update: schema.DefaultTimeout(20 * time.Minute), + Delete: schema.DefaultTimeout(20 * time.Minute), }, ReadWithoutTimeout: diagnosticsWrapper(resourceMetalPortRead), // Create and Update are the same func From 383679e5aaab68b59bd3e1a2a33dbb1ab70e59bd Mon Sep 17 00:00:00 2001 From: Ayush Rangwala Date: Tue, 26 Sep 2023 23:19:22 +0530 Subject: [PATCH 5/5] update default timeout to 30 min and update doc --- docs/resources/equinix_metal_port.md | 11 +++++++++++ equinix/resource_metal_port.go | 6 +++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/docs/resources/equinix_metal_port.md b/docs/resources/equinix_metal_port.md index a2a5179de..656a5fe12 100644 --- a/docs/resources/equinix_metal_port.md +++ b/docs/resources/equinix_metal_port.md @@ -30,6 +30,17 @@ ports. attached VLANs (from `vlan_ids` parameter). * `reset_on_delete` - (Optional) Behavioral setting to reset the port to default settings (layer3 bonded mode without any vlan attached) before delete/destroy. +### Timeouts + +The `timeouts` block allows you to specify [timeouts](https://www.terraform.io/configuration/resources#operation-timeouts) for certain actions: + +These timeout includes the time to disbond, convert to L2/L3, bond and update native vLAN. + +* `create` - (Defaults to 30 mins) Used when creating the Port. +* `update` - (Defaults to 30 mins) Used when updating the Port. +* `delete` - (Defaults to 30 mins) Used when deleting the Port. + + ## Attributes Reference In addition to all arguments above, the following attributes are exported: diff --git a/equinix/resource_metal_port.go b/equinix/resource_metal_port.go index 53a36ec87..375897ec8 100644 --- a/equinix/resource_metal_port.go +++ b/equinix/resource_metal_port.go @@ -22,9 +22,9 @@ var ( func resourceMetalPort() *schema.Resource { return &schema.Resource{ Timeouts: &schema.ResourceTimeout{ - Create: schema.DefaultTimeout(20 * time.Minute), - Update: schema.DefaultTimeout(20 * time.Minute), - Delete: schema.DefaultTimeout(20 * time.Minute), + Create: schema.DefaultTimeout(30 * time.Minute), + Update: schema.DefaultTimeout(30 * time.Minute), + Delete: schema.DefaultTimeout(30 * time.Minute), }, ReadWithoutTimeout: diagnosticsWrapper(resourceMetalPortRead), // Create and Update are the same func