Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Set timeout for v/virtual_machine reconfigure task #1645

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

rashaev
Copy link
Contributor

@rashaev rashaev commented Apr 7, 2022

Description

Sometimes reconfiguration tasks take a long time. And if they last more than 5 minutes, then the tasks fail. Because the limit is set as a constant as seen below.

// Reconfigure wraps the Reconfigure task and the subsequent waiting for
// the task to complete.
func Reconfigure(vm *object.VirtualMachine, spec types.VirtualMachineConfigSpec) error {
log.Printf("[DEBUG] Reconfiguring virtual machine %q", vm.InventoryPath)
ctx, cancel := context.WithTimeout(context.Background(), provider.DefaultAPITimeout)
defer cancel()
task, err := vm.Reconfigure(ctx, spec)
if err != nil {
return err
}
tctx, tcancel := context.WithTimeout(context.Background(), provider.DefaultAPITimeout)
defer tcancel()
return task.Wait(tctx)
}

This pull request adds a timeout setting for r/virtual_machine reconfiguration tasks.

Release Note

resource/virtual_machine: Virtual machine reconfiguration tasks will use the provider api_timeout setting. (GH-1645)

References

Closes #1646
Closes #1662
Closes #1764
Closes #1795

@hashicorp-cla
Copy link

hashicorp-cla commented Apr 7, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added provider Type: Provider size/xs Relative Sizing: Extra-Small labels Apr 7, 2022
@tenthirtyam
Copy link
Collaborator

tenthirtyam commented Apr 7, 2022

Hi @rashaev 👋,

Can you please open an enhancement issue and then link the pull request to the issue?

Ryan Johnson
Staff II Solutions Architect | VMware, Inc.

@tenthirtyam tenthirtyam added the waiting-response Status: Waiting on a Response label Apr 7, 2022
@github-actions github-actions bot removed the waiting-response Status: Waiting on a Response label Apr 7, 2022
@tenthirtyam tenthirtyam changed the title Timeout setting for tasks Reconfiguration feat: Set timeout setting for v/virtual_machine reconfigure task Apr 7, 2022
@tenthirtyam tenthirtyam added the waiting-response Status: Waiting on a Response label Apr 8, 2022
@tenthirtyam tenthirtyam added this to the v2.3.0 milestone Apr 8, 2022
@tenthirtyam tenthirtyam added the area/vm Area: Virtual Machines label Apr 8, 2022
@tenthirtyam tenthirtyam requested a review from appilon April 8, 2022 00:25
@rashaev
Copy link
Contributor Author

rashaev commented Apr 8, 2022

Hi @rashaev wave,

Can you please open an enhancement issue and then link the pull request to the issue?

Ryan Johnson Staff II Solutions Architect | VMware, Inc.

HI @tenthirtyam . Of course. I already did it.

@github-actions github-actions bot removed the waiting-response Status: Waiting on a Response label Apr 8, 2022
@tenthirtyam tenthirtyam self-requested a review June 2, 2022 12:25
Copy link
Collaborator

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring to @appilon review.

@tenthirtyam tenthirtyam added community/contribution Community: Contribution Help Wanted waiting-response Status: Waiting on a Response labels Jun 2, 2022
@tenthirtyam tenthirtyam added the needs-review Status: Pull Request Needs Review label Jun 10, 2022
@tenthirtyam tenthirtyam self-assigned this Jul 15, 2022
@tenthirtyam tenthirtyam self-requested a review July 15, 2022 21:59
@tenthirtyam tenthirtyam changed the title feat: Set timeout setting for v/virtual_machine reconfigure task feat: Set timeout for v/virtual_machine reconfigure task Jul 15, 2022
@tenthirtyam tenthirtyam removed the waiting-response Status: Waiting on a Response label Jul 15, 2022
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provider really doesn't implement the canonical setup for handling timeouts. https://www.terraform.io/plugin/sdkv2/resources/retries-and-customizable-timeouts

It appears the provider awkwardly has a private defaultAPITimeout which doesn't remain a default... in the provider configure function it gets set to the provider block attr api_timeout. Some places in the provider also seems to use a public constant provider.DefaultAPITimeout which does remain a hardcoded 5 minute timer..

With all that said, I'm comfortable approving this improvement for now, but would recommend harmonizing timeouts throughout the provider as a tech debt task.

@tenthirtyam
Copy link
Collaborator

Agreed, @appilon. I've opened #1698 for the technical debt.

Ryan Johnson
Senior Staff Solutions Architect | Product Engineering @ VMware, Inc.

Copy link
Collaborator

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

Ryan Johnson
Senior Staff Solutions Architect | Product Engineering @ VMware, Inc.

@tenthirtyam tenthirtyam merged commit 916bbbf into hashicorp:main Jul 26, 2022
tenthirtyam added a commit that referenced this pull request Jul 26, 2022
Updates `CHANGELOG.md` to include the enhancement provided in #1645.
@rashaev rashaev deleted the feat/reconfigure-timeout branch July 26, 2022 07:46
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2022
@tenthirtyam tenthirtyam removed the needs-review Status: Pull Request Needs Review label Aug 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/vm Area: Virtual Machines community/contribution Community: Contribution Help Wanted provider Type: Provider size/xs Relative Sizing: Extra-Small
Projects
None yet
4 participants