Fix ibm pi instance update wait for status

[jaywcarman]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #4258

[hkantare]

Can you please add test case results and scenario based test results

[jaywcarman]

@hkantare

The TestAccIBMPIInstancesDataSource_basic and TestAccIBMPIInstancesDataSource_basic acceptance tests passed for me when I ran them locally with this patch.

Setup:

export IC_API_KEY="$(ic_apikeys jwcarman)"
export IAAS_CLASSIC_API_KEY="$(ic_apikeys jwcarman)"
export IAAS_CLASSIC_USERNAME="jwcarman"
export IC_REGION="mon"
export IC_ZONE="mon01"
export PI_CLOUDINSTANCE_ID="8fa27c40-827c-4568-8813-79b398e9cd27"
export PI_NETWORK_NAME="terraform-test-power"
export PI_IMAGE="8de97d73-95c0-46e9-a509-0b10be36eded"

---

export TESTARGS="-run TestAccIBMPIInstancesDataSource_basic":

=== RUN   TestAccIBMPIInstancesDataSource_basic
--- PASS: TestAccIBMPIInstancesDataSource_basic (16.21s)
PASS
ok      github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/power   16.298s

---

export TESTARGS="-run TestAccIBMPIInstanceBasic":

=== RUN   TestAccIBMPIInstanceBasic
--- PASS: TestAccIBMPIInstanceBasic (387.52s)
PASS
ok      github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/power   387.616s

[rmoralesjr]

@yussufsh The existing code has been like this for 3 years. I wonder if the back end services changed behavior to cause the VM to go to VERIFY_RESIZE and then SHUTDOWN when resizing down? The service broker code does not look changed recently so it may be further down the stack. I'll run some tests with another client to see if the root cause is on the server side & not in Terraform and post results here.

[rmoralesjr]

OK I see in the logs posted in 4258 that the VM was SHUTDOWN prior to the update operation and so after the update the VM went back to SHUTDOWN state after the resizing states. I reproduced this behavior using the CLI. @yussufsh @dhirendersingh19 I think one option is to check the VM state prior to the update operation and look for that state after the update operation use that as a target state. Or Jay's change to use the isWaitforPIInstanceUpdate function I think would work as well except it doesn't cover error states like the original call to isWaitForPIInstanceAvailable function.

[jaywcarman]

Or Jay's change to use the isWaitforPIInstanceUpdate function I think would work as well except it doesn't cover error states like the original call to isWaitForPIInstanceAvailable function.

Since the Pending state is limited to []string{"RESIZE", "VERIFY_RESIZE"} (code ref) I think an error would be returned if the lpar returns any error state.

See https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/resource/state.go#L44-L45:

// If the Refresh function returns a state other than the Target state or one
// listed in Pending, return immediately with an error.

I don't understand why error states are included as isWaitForPIInstanceAvailable Target states. Why wouldn't we want an error returned on errors states?

[rmoralesjr]

I think the original coder listed the ERROR state as a target since that is a state from which terraform does not have to wait for completion anymore and they did not want to stop the rest of the terraform config from processing. But if we are covered for the ERROR state by the state.go processing then I am OK with the current change. Yussuf and Dhirender do you have concerns?

[rmoralesjr]

@jaywcarman I am going to revert this change since it is causing regressions in the scenarios where the PVM instance is active state. I should have asked you why you shutdown your PVM instance to do the mem/proc change outside terraform (in CLI or GUI)? The PVM instance only needs to be shutdown in when mem or proc is greater than maxMemLpar and maxCPULpar . . .

	_if mem > maxMemLpar || procs > maxCPULpar {

		err = performChangeAndReboot(ctx, client, instanceID, cloudInstanceID, mem, procs)
		if err != nil {
			return diag.FromErr(err)
		}_

[rmoralesjr]

The problem is the isWaitforPIInstanceUpdate doesn't actually detect the target state of Active since the refresh function it uses never passes that state (only SHUTOFF, RESIZE and "") even though the "ACTIVE" string is listed as a Target state in list . . .

func isPIInstanceShutAfterResourceChange(client *st.IBMPIInstanceClient, id string) resource.StateRefreshFunc {
	return func() (interface{}, string, error) {

		pvm, err := client.Get(id)
		if err != nil {
			return nil, "", err
		}

		if *pvm.Status == "SHUTOFF" && pvm.Health.Status == helpers.PIInstanceHealthOk {
			log.Printf("The lpar is now off after the resource change...")
			return pvm, "SHUTOFF", nil
		}

		return pvm, "RESIZE", nil
	}
}

In the case where mem and proc is changed and no shutdown is needed, the update operation will timeout since shutdown state is never seen. I don't think the manual step (via cli or GUI )of shutting down the VM is needed since the terraform code in the comment above will perform the reboot if needed.