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

refactor: use retry.StateChangeConf for encryption-at-rest resource. #1477

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

marcosuma
Copy link
Collaborator

Description

Please include a summary of the fix/feature/change, including any relevant motivation and context.

Link to any related issue(s): https://jira.mongodb.org/browse/INTMDB-1039

In this change we are removing the for-loop used as retry mechanism for the encryption_at_rest resource creation and using the retry strategy provided by the terraform plugin sdk.

Test

Note: I think we should unit test this the method used in the retry logic itself. But for now I've tested it manually with the following

main.tf:

resource "mongodbatlas_encryption_at_rest" "test" {
  project_id = "64fecfd3da30680cf9043172"

  aws_kms_config {
    enabled                 = true
    customer_master_key_id  = "..."
    region                  = "US_EAST_1"
    role_id                 = "64fecfd591da66678694d4b6"

  }
}

and succeeded:

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

mongodbatlas_encryption_at_rest.test: Creating...
mongodbatlas_encryption_at_rest.test: Creation complete after 5s [id=64fecfd3da30680cf9043172]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

I've also tried to "fake" it and return "RETRY" instead of "COMPLETED"

<<<< return encryptionResp, "COMPLETED", nil

>>>> return encryptionResp, "RETRY", nil

and got (unrelated) error which proves it indeed successfuly created the resource but kept retrying.

Type of change:

  • Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
  • New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have signed the MongoDB CLA
  • I have read the contribution guidelines
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Further comments

@marcosuma marcosuma requested a review from a team as a code owner September 19, 2023 09:37
@marcosuma
Copy link
Collaborator Author

I am also going to make sure I apply these proposed changes by Andrea.

Copy link
Collaborator

@maastha maastha left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@andreaangiolillo andreaangiolillo left a comment

Choose a reason for hiding this comment

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

LGTM

Refresh: resourceMongoDBAtlasEncryptionAtRestCreateRefreshFunc(ctx, projectID, conn, encryptionAtRestReq),
Timeout: 1 * time.Minute,
MinTimeout: 1 * time.Second,
Delay: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it makes sense to add a delay here to wait before retrying the operation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Delay here means:

Delay          time.Duration    // Wait this time before starting checks

for reference:

// StateChangeConf is the configuration struct used for `WaitForState`.
type StateChangeConf struct {
	Delay          time.Duration    // Wait this time before starting checks
	Pending        []string         // States that are "allowed" and will continue trying
	Refresh        StateRefreshFunc // Refreshes the current state
	Target         []string         // Target state
	Timeout        time.Duration    // The amount of time to wait before timeout
	MinTimeout     time.Duration    // Smallest time to wait before refreshes
	PollInterval   time.Duration    // Override MinTimeout/backoff and only poll this often
	NotFoundChecks int              // Number of times to allow not found (nil result from Refresh)

	// This is to work around inconsistent APIs
	ContinuousTargetOccurence int // Number of times the Target state has to occur continuously
}

@marcosuma marcosuma merged commit 1fabeeb into master Sep 26, 2023
21 checks passed
@marcosuma marcosuma deleted the INTMDB-1039-improve-retry-strategy branch September 26, 2023 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants