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

Make the terraform harness only use the rwmutex if the plugin cache is enabled #240

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

toastwaffle
Copy link
Contributor

@toastwaffle toastwaffle commented Feb 1, 2024

Description of your changes

As discussed in #239, this makes the terraform harness only take the lock if the plugin cache is enabled. If the plugin cache is not enabled, there is nothing to protect, and thus no need to take the lock.

This change also removes locking for all terraform plan operations, on the assumption that conflicts are unlikely, and can easily be retried on a subsequent reconcile if they do happen.

I have:

  • Run make reviewable to ensure this PR is ready for review. It returned a bunch of type errors which seemed unrelated to this change, but I ran make test which did pass and make build which did succeed.

How has this code been tested

Unit tests

Copy link
Collaborator

@bobh66 bobh66 left a comment

Choose a reason for hiding this comment

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

This LGTM, one minor comment. I'd like to run it through some tests locally before approving it just to be safe.

internal/terraform/terraform.go Outdated Show resolved Hide resolved
@toastwaffle
Copy link
Contributor Author

I have now tested this locally with 4 workspaces with the plugin cache disabled doing long-running applies (GKE cluster creation) in parallel (where previously they'd have gotten stuck), and 4 other workspaces with the plugin cache enabled successfully reconciling some DNS records

@bobh66 bobh66 merged commit 5cb9998 into upbound:main Feb 6, 2024
9 checks passed
@toastwaffle toastwaffle deleted the cache-lock branch February 6, 2024 16:04
@toastwaffle
Copy link
Contributor Author

@bobh66 does this qualify for an off-schedule release, or does this need to wait until the 29th?

@bobh66
Copy link
Collaborator

bobh66 commented Feb 6, 2024

Good question - seems like a good candidate for a backport - I'll see what the release team thinks

Copy link

github-actions bot commented Feb 6, 2024

Successfully created backport PR #242 for release-0.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants