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

Stop replication task before applying changes that require the task to be stopped. #24047

Merged
merged 15 commits into from
Apr 26, 2022

Conversation

fermezz
Copy link
Contributor

@fermezz fermezz commented Apr 6, 2022

Previous attempt to fix this issue relied on a new handle_task_lifecycle parameter. We do not do this in this PR because that attempt was before start_replication_task existed, so I think it's a good idea to manage the entire lifecycle through the one parameter.

I missed the branch-prefix convention. Let me know if it's important enough and I can re-create the PR with a new branch.

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

Closes #2236.
Closes #16092 (dup).

Output from acceptance testing:

Existing acceptance tests pass:

$ make testacc TESTS=TestAccDMSReplicationTask_startReplicationTask PKG=dms
Command being timed: "/usr/local/go/pkg/tool/linux_amd64/compile -o $WORK/b760/_pkg_.a -trimpath $WORK/b760=> -p github.com/hashicorp/terraform-provider-aws/internal/service/accessanalyzer -lang=go1.17 -complete -buildid hOz6ADJNtP16IxXhuG8-/hOz6ADJNtP16IxXhuG8- -goversion go1.18 -c=4 -nolocalimports -importcfg $WORK/b760/importcfg -pack internal/service/accessanalyzer/analyzer.go internal/service/accessanalyzer/generate.go internal/service/accessanalyzer/tags_gen.go"
    User time (seconds): 11.00
    System time (seconds): 7.02
    Percent of CPU this job got: 21%
    Elapsed (wall clock) time (h:mm:ss or m:ss): 1:24.02
    Average shared text size (kbytes): 0
    Average unshared data size (kbytes): 0
    Average stack size (kbytes): 0
    Average total size (kbytes): 0
    Maximum resident set size (kbytes): 790716
    Average resident set size (kbytes): 0
    Major (requiring I/O) page faults: 24661
    Minor (reclaiming a frame) page faults: 123527
    Voluntary context switches: 59091
    Involuntary context switches: 4010
    Swaps: 0
    File system inputs: 3506000
    File system outputs: 784
    Socket messages sent: 0
    Socket messages received: 0
    Signals delivered: 0
    Page size (bytes): 4096
    Exit status: 0
=== RUN   TestAccDMSReplicationTask_startReplicationTask
=== PAUSE TestAccDMSReplicationTask_startReplicationTask
=== CONT  TestAccDMSReplicationTask_startReplicationTask
--- PASS: TestAccDMSReplicationTask_startReplicationTask (1247.64s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dms        1247.783s

Result from adapted acceptance tests:

Command being timed: "/usr/local/go/pkg/tool/linux_amd64/compile -o $WORK/b760/_pkg_.a -trimpath $WORK/b760=> -p github.com/hashicorp/terraform-provider-aws/internal/service/accessanalyzer -lang=go1.17 -complete -buildid hOz6ADJNtP16IxXhuG8-/hOz6ADJNtP16IxXhuG8- -goversion go1.18 -c=4 -nolocalimports -importcfg $WORK/b760/importcfg -pack internal/service/accessanalyzer/analyzer.go internal/service/accessanalyzer/generate.go internal/service/accessanalyzer/tags_gen.go"
    User time (seconds): 11.00
    System time (seconds): 7.02
    Percent of CPU this job got: 21%
    Elapsed (wall clock) time (h:mm:ss or m:ss): 1:24.02
    Average shared text size (kbytes): 0
    Average unshared data size (kbytes): 0
    Average stack size (kbytes): 0
    Average total size (kbytes): 0
    Maximum resident set size (kbytes): 790716
    Average resident set size (kbytes): 0
    Major (requiring I/O) page faults: 24661
    Minor (reclaiming a frame) page faults: 123527
    Voluntary context switches: 59091
    Involuntary context switches: 4010
    Swaps: 0
    File system inputs: 3506000
    File system outputs: 784
    Socket messages sent: 0
    Socket messages received: 0
    Signals delivered: 0
    Page size (bytes): 4096
    Exit status: 0
=== RUN   TestAccDMSReplicationTask_startReplicationTask
=== PAUSE TestAccDMSReplicationTask_startReplicationTask
=== CONT  TestAccDMSReplicationTask_startReplicationTask
--- PASS: TestAccDMSReplicationTask_startReplicationTask (992.97s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dms        993.036s

@github-actions github-actions bot added needs-triage Waiting for first response or review from a maintainer. size/M Managed by automation to categorize the size of a PR. service/databasemigrationservice tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Apr 6, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome @fermezz 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

@fermezz fermezz force-pushed the fix-start-replication-task branch from 1bcbdc8 to 0cc6828 Compare April 6, 2022 16:16
Comment on lines 385 to 392
startReplicationTaskType := dms.StartReplicationTaskTypeValueStartReplication
if fromStatus != replicationTaskStatusReady {
startReplicationTaskType = dms.StartReplicationTaskTypeValueResumeProcessing
}

_, err = conn.StartReplicationTask(&dms.StartReplicationTaskInput{
ReplicationTaskArn: task.ReplicationTaskArn,
StartReplicationTaskType: aws.String(dms.StartReplicationTaskTypeValueStartReplication),
StartReplicationTaskType: aws.String(startReplicationTaskType),
Copy link
Contributor Author

@fermezz fermezz Apr 6, 2022

Choose a reason for hiding this comment

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

Author's note: This change is necessary because for tasks of type full-load and full-load-and-cdc you can only use start-replication when it's the first time the task is run. Afterwards, you have to use resume-processing, otherwise it will fail.

Proof of it, is the following error while running the acceptance tests without this change:

replication_task_test.go:100: Step 4/4 error: Error running apply: exit status 1

Error: error starting DMS Replication Task (tf-acc-test-748333755630122031): InvalidParameterCombinationException: Start Type : START_REPLICATION, valid only for tasks running for the first time status code: 400, request id: af7935aa-89d8-49de-b835-8d6a87addc65

with aws_dms_replication_task.test, on terraform_plugin_test.tf line 176, in resource "aws_dms_replication_task" "test":
    176: resource "aws_dms_replication_task" "test" {

--- FAIL: TestAccDMSReplicationTask_startReplicationTask (1373.44s)

This way, we only use start-replication after the task is on status ready, which only happens right after the task has been created.

@fermezz fermezz marked this pull request as ready for review April 6, 2022 23:49
Copy link

@andrew-wiggins andrew-wiggins left a comment

Choose a reason for hiding this comment

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

Thank you for jumping on this so quick, especially the update to fix StartReplicationTaskTypeValueStartReplication being the only start replication task type! LGTM!

@justinretzolk justinretzolk added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Apr 8, 2022
@johnsonaj johnsonaj self-assigned this Apr 26, 2022
@johnsonaj johnsonaj added the service/dms Issues and PRs that pertain to the dms service. label Apr 26, 2022
@johnsonaj
Copy link
Contributor

LGTM 🚀

% make testacc TESTS=TestAccReplicationTask_ PKG=dms
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/dms/... -v -count 1 -parallel 20 -run='TestAccReplicationTask_'  -timeout 180m
=== RUN   TestAccReplicationTask_basic
=== PAUSE TestAccReplicationTask_basic
=== RUN   TestAccReplicationTask_cdcStartPosition
=== PAUSE TestAccReplicationTask_cdcStartPosition
=== RUN   TestAccReplicationTask_startReplicationTask
=== PAUSE TestAccReplicationTask_startReplicationTask
=== RUN   TestAccReplicationTask_disappears
=== PAUSE TestAccReplicationTask_disappears
=== CONT  TestAccReplicationTask_basic
=== CONT  TestAccReplicationTask_startReplicationTask
=== CONT  TestAccReplicationTask_disappears
=== CONT  TestAccReplicationTask_cdcStartPosition
--- PASS: TestAccReplicationTask_basic (520.30s)
--- PASS: TestAccReplicationTask_cdcStartPosition (520.82s)
--- PASS: TestAccReplicationTask_disappears (552.30s)
--- PASS: TestAccReplicationTask_startReplicationTask (1388.83s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/dms	1391.129s

@johnsonaj johnsonaj self-requested a review April 26, 2022 21:08
@johnsonaj
Copy link
Contributor

@fermezz thanks for the contribution 🎉 👏🏾

I made a few changes for more accurate status feedback and testing

  • moved status check to startReplicationTask since it is available in the find function
  • added a _disappears test