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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

new: Add support for Unified Migrations #1150

Merged
merged 10 commits into from
Dec 6, 2023
Merged

new: Add support for Unified Migrations #1150

merged 10 commits into from
Dec 6, 2023

Conversation

lgarber-akamai
Copy link
Contributor

@lgarber-akamai lgarber-akamai commented Nov 21, 2023 •
edited

馃摑 Description

This change adds support for the following new linode_instance migration_type field introduced in Unified Migrations project. Additionally this change adds support for cross-region migrations on updates to the region field rather than just instance recreation.

鉁旓笍 How to Test

E2E Testing:

Cross-Region Migration

NOTE: This test currently needs to be run against prod due to availability limitations.

export RUN_LONG_TEST=1

make PKG_NAME=linode/instance TESTARGS="-run TestAccResourceInstance_migration" testacc

Resize Migration

export LINODE_URL=...
export LINODE_TOKEN=...

make PKG_NAME=linode/instance TESTARGS="-run TestAccResourceInstance_typeChangeDiskImplicit" testacc

Manual Testing

TODO

@lgarber-akamai lgarber-akamai changed the base branch from dev to proj/unified-migrations November 21, 2023 18:46
@lgarber-akamai lgarber-akamai changed the title new: Add support for unified migrations new: Add support for Unified Migrations Nov 21, 2023
@lgarber-akamai lgarber-akamai marked this pull request as ready for review November 27, 2023 16:33
@lgarber-akamai lgarber-akamai requested a review from a team as a code owner November 27, 2023 16:33
@lgarber-akamai lgarber-akamai requested review from ykim-1 and vshanthe and removed request for a team November 27, 2023 16:33
ykim-1
ykim-1 approved these changes Dec 1, 2023
View reviewed changes
Copy link
Contributor

@ykim-1 ykim-1 left a comment

Choose a reason for hiding this comment

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

I get a 5xx error occasionally but other than that it looks good

test Outdated Show resolved Hide resolved
@zliang-akamai
Copy link
Member

zliang-akamai commented Dec 4, 2023 •
edited

Timeout occasionally happened during my tests. Is it possible to increase the time out value? Maybe change LinodeInstanceUpdateTimeout to be like a few hours with warning logs/notification of the long run to users?

I also thought about change the timeout in runtime when the region change is detected but couldn't think about a way to implement it because the update function can't access the timeout setting. Did I miss anything?

@lgarber-akamai
Copy link
Contributor Author

lgarber-akamai commented Dec 5, 2023 •
edited

Timeout occasionally happened during my tests. Is it possible to increase the time out value? Maybe change LinodeInstanceUpdateTimeout to be like a few hours with warning logs/notification of the long run to users?

I also thought about change the timeout in runtime when the region change is detected but couldn't think about a way to implement it because the update function can't access the timeout setting. Did I miss anything?

@zliang-akamai Thanks for testing this out! I'm not sure if Terraform respects the test context deadline or the configurable timeout (or both), but I'll do some testing and push up a commit to address this. From a user perspective, I think it makes sense for us to bump the default timeout or at least inform the user to configure a longer timeout period.

zliang-akamai
zliang-akamai reviewed Dec 5, 2023
View reviewed changes
docs/resources/instance.md Outdated
@@ -271,7 +273,7 @@ The following arguments are available in an `ipv4` configuration block of an `in
The `timeouts` block allows you to specify [timeouts](https://www.terraform.io/docs/configuration/resources.html#timeouts) for certain actions:
Copy link
Member

Choose a reason for hiding this comment

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

Timeout doc of Terraform has been moved to here: https://developer.hashicorp.com/terraform/language/resources/syntax#operation-timeouts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I'll address that real quick

zliang-akamai
zliang-akamai approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@zliang-akamai zliang-akamai left a comment

Choose a reason for hiding this comment

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

Tested locally and looks good to me!

@lgarber-akamai lgarber-akamai merged commit 8241fe4 into linode:proj/unified-migrations Dec 6, 2023
4 checks passed
zliang-akamai added a commit that referenced this pull request Jan 8, 2024
@zliang-akamai @lgarber-akamai
Project: Unified Migration (#1201)
931ebd6 
* new: Add support for Unified Migrations (#1150)

* WIP

* Add region migrations

* minor changes

* long running test

* Update test logic

* Support filtering in GetRegionsWithCaps

* Add migration_type to ImportStateVerifyIgnore

* point at feature branch

* Update timeout and docs

* Update timeouts link

* Bump linodego to v1.27.0

* go mod tidy

---------

Co-authored-by: Lena Garber <114949949+lgarber-akamai@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zliang-akamai zliang-akamai zliang-akamai approved these changes

@ykim-1 ykim-1 ykim-1 approved these changes

@vshanthe vshanthe Awaiting requested review from vshanthe vshanthe was automatically assigned from linode/dx

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lgarber-akamai @zliang-akamai @ykim-1