From 5b0c62f3d975469a8878d4e79479a25b67ee6c6e Mon Sep 17 00:00:00 2001 From: Angus Salkeld Date: Thu, 8 Apr 2021 21:00:34 +1000 Subject: [PATCH] return the last error with any error we get (#50) * Fix linting warnings * in workflow.run() append the last_error to any error * Improve the error handling * things caught by handling errors --- ironic/data_source_ironic_introspection.go | 48 +++++++-- ironic/provider.go | 15 +-- ironic/resource_ironic_allocation_v1.go | 82 +++++++++------ ironic/resource_ironic_deployment.go | 49 +++++---- ironic/resource_ironic_node_v1.go | 116 ++++++++++++++++----- ironic/resource_ironic_port_v1.go | 39 +++++-- ironic/workflow.go | 91 +++++++++------- 7 files changed, 296 insertions(+), 144 deletions(-) diff --git a/ironic/data_source_ironic_introspection.go b/ironic/data_source_ironic_introspection.go index c95a400de..5ff9c9637 100644 --- a/ironic/data_source_ironic_introspection.go +++ b/ironic/data_source_ironic_introspection.go @@ -2,9 +2,10 @@ package ironic import ( "fmt" + "time" + "github.com/gophercloud/gophercloud/openstack/baremetalintrospection/v1/introspection" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" - "time" ) // Schema resource for an introspection data source, that has some selected details about the node exposed. @@ -79,11 +80,26 @@ func dataSourceIronicIntrospectionRead(d *schema.ResourceData, meta interface{}) return fmt.Errorf("could not get introspection status: %s", err.Error()) } - d.Set("finished", status.Finished) - d.Set("finished_at", status.FinishedAt) - d.Set("started_at", status.StartedAt) - d.Set("error", status.Error) - d.Set("state", status.State) + err = d.Set("finished", status.Finished) + if err != nil { + return err + } + err = d.Set("finished_at", status.FinishedAt.Format("2006-01-02T15:04:05")) + if err != nil { + return err + } + err = d.Set("started_at", status.StartedAt.Format("2006-01-02T15:04:05")) + if err != nil { + return err + } + err = d.Set("error", status.Error) + if err != nil { + return err + } + err = d.Set("state", status.State) + if err != nil { + return err + } if status.Finished { data, err := introspection.GetIntrospectionData(client, uuid).Extract() @@ -100,14 +116,26 @@ func dataSourceIronicIntrospectionRead(d *schema.ResourceData, meta interface{}) "ip": v.IP, }) } - d.Set("interfaces", interfaces) + err = d.Set("interfaces", interfaces) + if err != nil { + return err + } // CPU data - d.Set("cpu_arch", data.CPUArch) - d.Set("cpu_count", data.CPUs) + err = d.Set("cpu_arch", data.CPUArch) + if err != nil { + return err + } + err = d.Set("cpu_count", data.CPUs) + if err != nil { + return err + } // Memory info - d.Set("memory_mb", data.MemoryMB) + err = d.Set("memory_mb", data.MemoryMB) + if err != nil { + return err + } } d.SetId(time.Now().UTC().String()) diff --git a/ironic/provider.go b/ironic/provider.go index 49071651a..0050f15e3 100644 --- a/ironic/provider.go +++ b/ironic/provider.go @@ -3,6 +3,12 @@ package ironic import ( "context" "fmt" + "log" + "net/http" + "strings" + "sync" + "time" + "github.com/gophercloud/gophercloud" "github.com/gophercloud/gophercloud/openstack/baremetal/httpbasic" "github.com/gophercloud/gophercloud/openstack/baremetal/noauth" @@ -13,11 +19,6 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/helper/validation" "github.com/hashicorp/terraform-plugin-sdk/terraform" - "log" - "net/http" - "strings" - "sync" - "time" ) // Clients stores the client connection information for Ironic and Inspector @@ -354,7 +355,7 @@ func waitForConductor(ctx context.Context, client *gophercloud.ServiceClient) { log.Printf("[DEBUG] Waiting for conductor API to become available...") driverCount := 0 - drivers.ListDrivers(client, drivers.ListDriversOpts{ + err := drivers.ListDrivers(client, drivers.ListDriversOpts{ Detail: false, }).EachPage(func(page pagination.Page) (bool, error) { actual, err := drivers.ExtractDrivers(page) @@ -365,7 +366,7 @@ func waitForConductor(ctx context.Context, client *gophercloud.ServiceClient) { return true, nil }) // If we have any drivers, conductor is up. - if driverCount > 0 { + if err == nil && driverCount > 0 { return } diff --git a/ironic/resource_ironic_allocation_v1.go b/ironic/resource_ironic_allocation_v1.go index 70de42b88..8477a0f68 100644 --- a/ironic/resource_ironic_allocation_v1.go +++ b/ironic/resource_ironic_allocation_v1.go @@ -81,34 +81,33 @@ func resourceAllocationV1Create(d *schema.ResourceData, meta interface{}) error d.SetId(result.UUID) // Wait for state to change from allocating - var state string timeout := 1 * time.Minute checkInterval := 2 * time.Second - getState := func() string { - resourceAllocationV1Read(d, meta) - return d.Get("state").(string) - } - - for state = getState(); state == "allocating"; state = getState() { + for { + err = resourceAllocationV1Read(d, meta) + if err != nil { + return err + } + state := d.Get("state").(string) log.Printf("[DEBUG] Requested allocation %s; current state is '%s'\n", d.Id(), state) - - time.Sleep(checkInterval) - checkInterval += 2 - timeout -= checkInterval - if timeout < 0 { - return fmt.Errorf("timed out waiting for allocation") + switch state { + case "allocating": + time.Sleep(checkInterval) + checkInterval += 2 + timeout -= checkInterval + if timeout < 0 { + return fmt.Errorf("timed out waiting for allocation") + } + case "error": + err := d.Get("last_error").(string) + _ = resourceAllocationV1Delete(d, meta) + d.SetId("") + return fmt.Errorf("error creating resource: %s", err) + default: + return nil } } - - if state == "error" { - err := d.Get("last_error").(string) - resourceAllocationV1Delete(d, meta) - d.SetId("") - return fmt.Errorf("error creating resource: %s", err) - } - - return nil } // Read the allocation's data from Ironic @@ -123,16 +122,35 @@ func resourceAllocationV1Read(d *schema.ResourceData, meta interface{}) error { return err } - d.Set("name", result.Name) - d.Set("resource_class", result.ResourceClass) - d.Set("candidate_nodes", result.CandidateNodes) - d.Set("traits", result.Traits) - d.Set("extra", result.Extra) - d.Set("node_uuid", result.NodeUUID) - d.Set("state", result.State) - d.Set("last_error", result.LastError) - - return nil + err = d.Set("name", result.Name) + if err != nil { + return err + } + err = d.Set("resource_class", result.ResourceClass) + if err != nil { + return err + } + err = d.Set("candidate_nodes", result.CandidateNodes) + if err != nil { + return err + } + err = d.Set("traits", result.Traits) + if err != nil { + return err + } + err = d.Set("extra", result.Extra) + if err != nil { + return err + } + err = d.Set("node_uuid", result.NodeUUID) + if err != nil { + return err + } + err = d.Set("state", result.State) + if err != nil { + return err + } + return d.Set("last_error", result.LastError) } // Delete an allocation from Ironic if it exists diff --git a/ironic/resource_ironic_deployment.go b/ironic/resource_ironic_deployment.go index 84f57e662..622f8f3bd 100644 --- a/ironic/resource_ironic_deployment.go +++ b/ironic/resource_ironic_deployment.go @@ -5,14 +5,15 @@ import ( "crypto/x509" "encoding/base64" "fmt" + "io/ioutil" + "log" + "net/http" + "github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes" utils "github.com/gophercloud/utils/openstack/baremetal/v1/nodes" retryablehttp "github.com/hashicorp/go-retryablehttp" "github.com/hashicorp/go-version" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" - "io/ioutil" - "log" - "net/http" ) // Schema resource definition for an Ironic deployment. @@ -88,7 +89,7 @@ func resourceDeploymentCreate(d *schema.ResourceData, meta interface{}) error { } // Reload the resource before returning - defer resourceDeploymentRead(d, meta) + defer func() { _ = resourceDeploymentRead(d, meta) }() // Set instance info instanceInfo := d.Get("instance_info").(map[string]interface{}) @@ -163,10 +164,8 @@ func fetchFullIgnition(userDataURL string, userDataCaCert string, userDataHeader log.Printf("could not get user_data_url: %s", err) return "", err } - if userDataHeaders != nil { - for k, v := range userDataHeaders { - req.Header.Add(k, v.(string)) - } + for k, v := range userDataHeaders { + req.Header.Add(k, v.(string)) } resp, err := client.Do(req) if err != nil { @@ -187,9 +186,15 @@ func fetchFullIgnition(userDataURL string, userDataCaCert string, userDataHeader // buildConfigDrive handles building a config drive appropriate for the Ironic version we are using. Newer versions // support sending the user data directly, otherwise we need to build an ISO image -func buildConfigDrive(apiVersion, userData string, networkData, metaData map[string]interface{}) (configDrive interface{}, err error) { +func buildConfigDrive(apiVersion, userData string, networkData, metaData map[string]interface{}) (interface{}, error) { actual, err := version.NewVersion(apiVersion) + if err != nil { + return nil, err + } minimum, err := version.NewVersion("1.56") + if err != nil { + return nil, err + } if minimum.GreaterThan(actual) { // Create config drive ISO directly with gophercloud/utils @@ -202,17 +207,14 @@ func buildConfigDrive(apiVersion, userData string, networkData, metaData map[str if err != nil { return nil, err } - configDrive = &configDriveISO - } else { - // Let Ironic handle creating the config drive - configDrive = &nodes.ConfigDrive{ - UserData: userData, - NetworkData: networkData, - MetaData: metaData, - } + return &configDriveISO, nil } - - return + // Let Ironic handle creating the config drive + return &nodes.ConfigDrive{ + UserData: userData, + NetworkData: networkData, + MetaData: metaData, + }, nil } // Read the deployment's data from Ironic @@ -229,10 +231,11 @@ func resourceDeploymentRead(d *schema.ResourceData, meta interface{}) error { return fmt.Errorf("could not find node %s: %s", id, err) } - d.Set("provision_state", result.ProvisionState) - d.Set("last_error", result.LastError) - - return nil + err = d.Set("provision_state", result.ProvisionState) + if err != nil { + return err + } + return d.Set("last_error", result.LastError) } // Delete an deployment from Ironic - this cleans the node and returns it's state to 'available' diff --git a/ironic/resource_ironic_node_v1.go b/ironic/resource_ironic_node_v1.go index cd3cdf28b..503e79a9a 100644 --- a/ironic/resource_ironic_node_v1.go +++ b/ironic/resource_ironic_node_v1.go @@ -222,7 +222,7 @@ func resourceNodeV1Create(d *schema.ResourceData, meta interface{}) error { } _, err := ports.Create(client, portCreateOpts).Extract() if err != nil { - resourcePortV1Read(d, meta) + _ = resourcePortV1Read(d, meta) return err } } @@ -282,32 +282,96 @@ func resourceNodeV1Read(d *schema.ResourceData, meta interface{}) error { // TODO: Ironic's Create is different than the Node object itself, GET returns things like the // RaidConfig, we need to add those and handle them in CREATE - d.Set("boot_interface", node.BootInterface) - d.Set("conductor_group", node.ConductorGroup) - d.Set("console_interface", node.ConsoleInterface) - d.Set("deploy_interface", node.DeployInterface) - d.Set("driver", node.Driver) - d.Set("driver_info", node.DriverInfo) - d.Set("extra", node.Extra) - d.Set("inspect_interface", node.InspectInterface) - d.Set("instance_uuid", node.InstanceUUID) - d.Set("management_interface", node.ManagementInterface) - d.Set("name", node.Name) - d.Set("network_interface", node.NetworkInterface) - d.Set("owner", node.Owner) - d.Set("power_interface", node.PowerInterface) - d.Set("power_state", node.PowerState) - d.Set("root_device", node.Properties["root_device"]) + err = d.Set("boot_interface", node.BootInterface) + if err != nil { + return err + } + err = d.Set("conductor_group", node.ConductorGroup) + if err != nil { + return err + } + err = d.Set("console_interface", node.ConsoleInterface) + if err != nil { + return err + } + err = d.Set("deploy_interface", node.DeployInterface) + if err != nil { + return err + } + err = d.Set("driver", node.Driver) + if err != nil { + return err + } + err = d.Set("driver_info", node.DriverInfo) + if err != nil { + return err + } + err = d.Set("extra", node.Extra) + if err != nil { + return err + } + err = d.Set("inspect_interface", node.InspectInterface) + if err != nil { + return err + } + err = d.Set("instance_uuid", node.InstanceUUID) + if err != nil { + return err + } + err = d.Set("management_interface", node.ManagementInterface) + if err != nil { + return err + } + err = d.Set("name", node.Name) + if err != nil { + return err + } + err = d.Set("network_interface", node.NetworkInterface) + if err != nil { + return err + } + err = d.Set("owner", node.Owner) + if err != nil { + return err + } + err = d.Set("power_interface", node.PowerInterface) + if err != nil { + return err + } + err = d.Set("power_state", node.PowerState) + if err != nil { + return err + } + err = d.Set("root_device", node.Properties["root_device"]) + if err != nil { + return err + } delete(node.Properties, "root_device") - d.Set("properties", node.Properties) - d.Set("raid_interface", node.RAIDInterface) - d.Set("rescue_interface", node.RescueInterface) - d.Set("resource_class", node.ResourceClass) - d.Set("storage_interface", node.StorageInterface) - d.Set("vendor_interface", node.VendorInterface) - d.Set("provision_state", node.ProvisionState) - - return nil + err = d.Set("properties", node.Properties) + if err != nil { + return err + } + err = d.Set("raid_interface", node.RAIDInterface) + if err != nil { + return err + } + err = d.Set("rescue_interface", node.RescueInterface) + if err != nil { + return err + } + err = d.Set("resource_class", node.ResourceClass) + if err != nil { + return err + } + err = d.Set("storage_interface", node.StorageInterface) + if err != nil { + return err + } + err = d.Set("vendor_interface", node.VendorInterface) + if err != nil { + return err + } + return d.Set("provision_state", node.ProvisionState) } // Update a node's state based on the terraform config - TODO: handle everything diff --git a/ironic/resource_ironic_port_v1.go b/ironic/resource_ironic_port_v1.go index fee5a2a8a..c2b17011b 100644 --- a/ironic/resource_ironic_port_v1.go +++ b/ironic/resource_ironic_port_v1.go @@ -76,16 +76,35 @@ func resourcePortV1Read(d *schema.ResourceData, meta interface{}) error { return err } - d.Set("address", port.Address) - d.Set("node_uuid", port.NodeUUID) - d.Set("port_group_id", port.PortGroupUUID) - d.Set("local_link_collection", port.LocalLinkConnection) - d.Set("pxe_enabled", port.PXEEnabled) - d.Set("physical_network", port.PhysicalNetwork) - d.Set("extra", port.Extra) - d.Set("is_smart_nic", port.IsSmartNIC) - - return nil + err = d.Set("address", port.Address) + if err != nil { + return err + } + err = d.Set("node_uuid", port.NodeUUID) + if err != nil { + return err + } + err = d.Set("port_group_id", port.PortGroupUUID) + if err != nil { + return err + } + err = d.Set("local_link_collection", port.LocalLinkConnection) + if err != nil { + return err + } + err = d.Set("pxe_enabled", port.PXEEnabled) + if err != nil { + return err + } + err = d.Set("physical_network", port.PhysicalNetwork) + if err != nil { + return err + } + err = d.Set("extra", port.Extra) + if err != nil { + return err + } + return d.Set("is_smart_nic", port.IsSmartNIC) } func resourcePortV1Update(d *schema.ResourceData, meta interface{}) error { diff --git a/ironic/workflow.go b/ironic/workflow.go index b0e8eac59..a22d79300 100644 --- a/ironic/workflow.go +++ b/ironic/workflow.go @@ -2,10 +2,11 @@ package ironic import ( "fmt" - "github.com/gophercloud/gophercloud" - "github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes" "log" "time" + + "github.com/gophercloud/gophercloud" + "github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes" ) // provisionStateWorkflow is used to track state through the process of updating's it's provision state @@ -22,7 +23,6 @@ type provisionStateWorkflow struct { // ChangeProvisionStateToTarget drives Ironic's state machine through the process to reach our desired end state. This requires multiple // possibly long-running steps. If required, we'll build a config drive ISO for deployment. func ChangeProvisionStateToTarget(client *gophercloud.ServiceClient, uuid string, target nodes.TargetProvisionState, configDrive interface{}) error { - // Run the provisionStateWorkflow - this could take a while wf := provisionStateWorkflow{ target: target, @@ -32,8 +32,7 @@ func ChangeProvisionStateToTarget(client *gophercloud.ServiceClient, uuid string configDrive: configDrive, } - err := wf.run() - return err + return wf.run() } // Keep driving the state machine forward @@ -44,18 +43,20 @@ func (workflow *provisionStateWorkflow) run() error { log.Printf("[DEBUG] Node is in state '%s'", workflow.node.ProvisionState) done, err := workflow.next() - if done || err != nil { - return err + if err != nil { + _ = workflow.reloadNode() // to get the lastError + return fmt.Errorf("%w , last error was '%s'", err, workflow.node.LastError) + } + if done { + return nil } time.Sleep(workflow.wait) } - - return nil } // Do the next thing to get us to our target state -func (workflow *provisionStateWorkflow) next() (done bool, err error) { +func (workflow *provisionStateWorkflow) next() (bool, error) { // Refresh the node on each run if err := workflow.reloadNode(); err != nil { return true, err @@ -82,11 +83,11 @@ func (workflow *provisionStateWorkflow) next() (done bool, err error) { } // Change a node to "manageable" stable -func (workflow *provisionStateWorkflow) toManageable() (done bool, err error) { +func (workflow *provisionStateWorkflow) toManageable() (bool, error) { switch state := workflow.node.ProvisionState; state { case "manageable": // We're done! - return true, err + return true, nil case "enroll", "adopt failed", "clean failed", @@ -100,25 +101,33 @@ func (workflow *provisionStateWorkflow) toManageable() (done bool, err error) { default: return true, fmt.Errorf("cannot go from state '%s' to state 'manageable'", state) } - - return false, nil } // Clean a node -func (workflow *provisionStateWorkflow) toClean() (done bool, err error) { +func (workflow *provisionStateWorkflow) toClean() (bool, error) { // Node must be manageable first - workflow.reloadNode() + err := workflow.reloadNode() + if err != nil { + return true, err + } if workflow.node.ProvisionState != string(nodes.Manageable) { - if err := ChangeProvisionStateToTarget(workflow.client, workflow.uuid, nodes.TargetManage, nil); err != nil { + err = ChangeProvisionStateToTarget(workflow.client, workflow.uuid, nodes.TargetManage, nil) + if err != nil { return true, err } } // Set target to clean - workflow.changeProvisionState(nodes.TargetClean) + _, err = workflow.changeProvisionState(nodes.TargetClean) + if err != nil { + return true, err + } for { - workflow.reloadNode() + err = workflow.reloadNode() + if err != nil { + return true, err + } state := workflow.node.ProvisionState switch state { @@ -129,17 +138,18 @@ func (workflow *provisionStateWorkflow) toClean() (done bool, err error) { // Not done, no error - Ironic is working continue default: - return true, fmt.Errorf("could not clean node, node is currently '%s', last error was '%s'", state, workflow.node.LastError) + return true, fmt.Errorf("could not clean node, node is currently '%s'", state) } } - - return true, nil } // Inspect a node -func (workflow *provisionStateWorkflow) toInspect() (done bool, err error) { +func (workflow *provisionStateWorkflow) toInspect() (bool, error) { // Node must be manageable first - workflow.reloadNode() + err := workflow.reloadNode() + if err != nil { + return true, err + } if workflow.node.ProvisionState != string(nodes.Manageable) { if err := ChangeProvisionStateToTarget(workflow.client, workflow.uuid, nodes.TargetManage, nil); err != nil { return true, err @@ -147,10 +157,16 @@ func (workflow *provisionStateWorkflow) toInspect() (done bool, err error) { } // Set target to inspect - workflow.changeProvisionState(nodes.TargetInspect) + _, err = workflow.changeProvisionState(nodes.TargetInspect) + if err != nil { + return true, err + } for { - workflow.reloadNode() + err = workflow.reloadNode() + if err != nil { + return true, err + } state := workflow.node.ProvisionState switch state { @@ -161,15 +177,13 @@ func (workflow *provisionStateWorkflow) toInspect() (done bool, err error) { // Not done, no error - Ironic is working continue default: - return true, fmt.Errorf("could not inspect node, node is currently '%s', last error was '%s'", state, workflow.node.LastError) + return true, fmt.Errorf("could not inspect node, node is currently '%s'", state) } } - - return true, nil } // Change a node to "available" state -func (workflow *provisionStateWorkflow) toAvailable() (done bool, err error) { +func (workflow *provisionStateWorkflow) toAvailable() (bool, error) { switch state := workflow.node.ProvisionState; state { case "available": // We're done! @@ -192,13 +206,10 @@ func (workflow *provisionStateWorkflow) toAvailable() (done bool, err error) { } return false, nil } - - return false, nil } // Change a node to "active" state func (workflow *provisionStateWorkflow) toActive() (bool, error) { - switch state := workflow.node.ProvisionState; state { case "active": // We're done! @@ -256,8 +267,6 @@ func (workflow *provisionStateWorkflow) toDeleted() (bool, error) { default: return true, fmt.Errorf("cannot delete node in state '%s'", state) } - - return false, nil } // Builds the ProvisionStateOpts to send to Ironic -- including config drive. @@ -270,18 +279,28 @@ func (workflow *provisionStateWorkflow) buildProvisionStateOpts(target nodes.Tar if target == "active" { opts.ConfigDrive = workflow.configDrive } + if target == "clean" { + opts.CleanSteps = []nodes.CleanStep{} + // TODO if we want to actually clean, then we need clean_steps + // currently bmo does quite a lot of work to get raid cleaning working. + // https://github.com/metal3-io/baremetal-operator/blob/master/pkg/provisioner/ironic/ironic.go#L1249-L1292 + } return &opts, nil } // Call Ironic's API and issue the change provision state request. -func (workflow *provisionStateWorkflow) changeProvisionState(target nodes.TargetProvisionState) (done bool, err error) { +func (workflow *provisionStateWorkflow) changeProvisionState(target nodes.TargetProvisionState) (bool, error) { opts, err := workflow.buildProvisionStateOpts(target) if err != nil { log.Printf("[ERROR] Unable to construct provisioning state options: %s", err.Error()) return true, err } + if target == "clean" && len(opts.CleanSteps) == 0 { + return true, nil + } + interval := 5 * time.Second for retries := 0; retries < 5; retries++ { err = nodes.ChangeProvisionState(workflow.client, workflow.uuid, *opts).ExtractErr()