Update TF install to shared binary approach #10147

lakshmimsft · 2025-08-01T20:53:09Z

Description

This pull request updates Terraform install to a global shared binary approach, which alleviates race conditions and state lock errors caused by concurrent operations. Additionally, the changes include state lock timeout handling and error checking around Terraform binary availability.

Type of change

This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional).

Fixes: #10179

Contributor checklist

Please verify that the PR meets the following requirements, where applicable:

An overview of proposed schema changes is included in a linked GitHub issue.
- Yes
- Not applicable
A design document PR is created in the design-notes repository, if new APIs are being introduced.
- Yes
- Not applicable
The design document has been reviewed and approved by Radius maintainers/approvers.
- Yes
- Not applicable
A PR for the samples repository is created, if existing samples are affected by the changes in this PR.
- Yes
- Not applicable
A PR for the documentation repository is created, if the changes in this PR affect the documentation or any user facing updates are made.
- Yes
- Not applicable
A PR for the recipes repository is created, if existing recipes are affected by the changes in this PR.
- Yes
- Not applicable

github-actions · 2025-08-01T21:05:57Z

Unit Tests

4 094 tests ±0 4 091 ✅ ±0 7m 23s ⏱️ +3s
307 suites ±0 3 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 7624827. ± Comparison against base commit d5d62cb.

This pull request removes 4 and adds 4 tests. Note that renamed tests count towards both.

github.com/radius-project/radius/pkg/recipes/terraform ‑ TestInstall_NoPreMountedBinary_Download
github.com/radius-project/radius/pkg/recipes/terraform ‑ TestInstall_PreMountedBinary
github.com/radius-project/radius/pkg/recipes/terraform ‑ TestInstall_PreMountedBinaryInvalid_FallbackToDownload
github.com/radius-project/radius/pkg/recipes/terraform ‑ TestInstall_PreMountedBinaryNotExecutable

github.com/radius-project/radius/pkg/recipes/terraform ‑ TestInstall_GlobalBinaryConcurrency
github.com/radius-project/radius/pkg/recipes/terraform ‑ TestInstall_GlobalBinaryReuse
github.com/radius-project/radius/pkg/recipes/terraform ‑ TestInstall_MultipleConcurrentCallsUseSameBinary
github.com/radius-project/radius/pkg/recipes/terraform ‑ TestInstall_SuccessfulDownload

♻️ This comment has been updated with latest results.

codecov · 2025-08-01T21:06:01Z

Codecov Report

❌ Patch coverage is 42.22222% with 78 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.83%. Comparing base (d5d62cb) to head (7624827).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/recipes/terraform/install.go	43.95%	43 Missing and 8 partials ⚠️
pkg/recipes/terraform/execute.go	0.00%	27 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10147      +/-   ##
==========================================
+ Coverage   49.81%   49.83%   +0.01%     
==========================================
  Files         640      640              
  Lines       49690    49733      +43     
==========================================
+ Hits        24753    24782      +29     
- Misses      23050    23066      +16     
+ Partials     1887     1885       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: lakshmimsft <[email protected]>

radius-functional-tests · 2025-08-05T17:31:03Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	radius-project/radius
Commit ref	`7624827`
Unique ID	funcbd55d410da
Image tag	pr-funcbd55d410da

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.29.0
Dapr:
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcbd55d410da
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcbd55d410da
dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-funcbd55d410da
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcbd55d410da
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcbd55d410da
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

ytimocin · 2025-08-05T19:23:32Z

pkg/recipes/terraform/config/backends/kubernetes.go


-	return fmt.Sprintf("%x", hash), nil
+	return suffix, nil


thanks for the cleanup

ytimocin · 2025-08-05T19:28:21Z

pkg/recipes/terraform/types.go

@@ -34,6 +34,9 @@ import (
 const (
 	executionSubDir                = "deploy"
 	workingDirFileMode fs.FileMode = 0700
+
+	// DefaultStateLockTimeout is the default timeout for acquiring Terraform state locks
+	DefaultStateLockTimeout = "10m"


Is the app going to wait 10 minutes to acquire the lock? I think our worker will have already timed out by then because it uses 2 minutes in terms of timeout.

No, it tries to acquire the lock immediately. It will wait for 10min if the lock is not available. https://developer.hashicorp.com/terraform/cli/commands/apply#lock-timeout-duration

ytimocin · 2025-08-05T19:38:27Z

pkg/recipes/terraform/install.go

+
+// ensureGlobalTerraformBinary ensures a global shared Terraform binary is available.
+// Uses mutex-based locking to prevent race conditions during concurrent access.
+func ensureGlobalTerraformBinary(ctx context.Context, installer *install.Installer, logger logr.Logger) (string, error) {


I believe that this function can be split into smaller pieces in terms of modularity. I see a few opportunities:

If os.Stat(globalBinary) && os.Stat(globalMarker) is true then we can just check to see if globalTerraformReady is true or false. Based on that we can call another function like verifyBinaryWorks which can be used in other places too.

Some logger.Infos could be logger.Warn if Warn exists for logger.

Download and Install Terraform could be split to another function.

We do tfexec.NewTerraform and tf.Version in a few places. We can split that to its own function.

My main concern with this function is that it does a lot of things and testing it would be difficult. We should simplify it as much as we can.

I will address this in separate PR.

ytimocin · 2025-08-05T19:39:52Z

pkg/recipes/terraform/execute.go

 // initAndApply runs Terraform init and apply in the provided working directory.
-func initAndApply(ctx context.Context, tf *tfexec.Terraform) (*tfjson.State, error) {
+func initAndApply(ctx context.Context, tf *tfexec.Terraform, stateLockTimeout string) (*tfjson.State, error) {


Instead of passing stateLockTimeout, we can pass in ...tfexec.ApplyOption (or an array of the same) because, in future, we can have more options and they are just going to float the signature of this function.

I think I might leave this as is for now. We can refactor when we need to.

sylvainsf

Looks great!

vishwahiremat · 2025-08-05T18:05:59Z

pkg/recipes/driver/terraform.go

+		ResourceRecipe:   &opts.Recipe,
+		EnvRecipe:        &opts.Definition,
+		Secrets:          opts.Secrets,
+		StateLockTimeout: terraform.DefaultStateLockTimeout,


what happens if the deployment takes longer than StateLockTimeout ?

this is time taken to wait for lock to be available, so it can then run the deployment. https://developer.hashicorp.com/terraform/cli/commands/apply#lock-timeout-duration

vishwahiremat · 2025-08-05T18:09:22Z

pkg/recipes/terraform/execute.go

+	if execPath := tf.ExecPath(); execPath != "" {
+		if _, err := os.Stat(execPath); err != nil {
+			logger.Info(fmt.Sprintf("ERROR: Terraform binary missing at %s during state fetch: %s", execPath, err.Error()))
+			return nil, fmt.Errorf("terraform binary disappeared at %s: %w", execPath, err)


i am not sure if we wanna add exec path in the error output.

I will address this in separate PR

lakshmimsft had a problem deploying to functional-tests August 1, 2025 20:53 — with GitHub Actions Failure

lakshmimsft temporarily deployed to publish-bicep August 1, 2025 20:53 — with GitHub Actions Inactive

lakshmimsft force-pushed the lakshmimsft/testtf branch from 80bf545 to 155c718 Compare August 1, 2025 22:19

lakshmimsft had a problem deploying to functional-tests August 1, 2025 22:19 — with GitHub Actions Failure

lakshmimsft temporarily deployed to publish-bicep August 1, 2025 22:19 — with GitHub Actions Inactive

lakshmimsft had a problem deploying to functional-tests August 1, 2025 23:29 — with GitHub Actions Failure

lakshmimsft temporarily deployed to publish-bicep August 1, 2025 23:29 — with GitHub Actions Inactive

lakshmimsft force-pushed the lakshmimsft/testtf branch from bdbc5a9 to 58ec7fc Compare August 2, 2025 19:25

lakshmimsft had a problem deploying to functional-tests August 2, 2025 19:25 — with GitHub Actions Failure

lakshmimsft temporarily deployed to publish-bicep August 2, 2025 19:25 — with GitHub Actions Inactive

lakshmimsft force-pushed the lakshmimsft/testtf branch from 58ec7fc to 6b33054 Compare August 2, 2025 20:09

lakshmimsft had a problem deploying to functional-tests August 2, 2025 20:09 — with GitHub Actions Failure

lakshmimsft temporarily deployed to publish-bicep August 2, 2025 20:09 — with GitHub Actions Inactive

lakshmimsft force-pushed the lakshmimsft/testtf branch from 6b33054 to a55a451 Compare August 2, 2025 21:17

lakshmimsft had a problem deploying to functional-tests August 2, 2025 21:17 — with GitHub Actions Failure

lakshmimsft temporarily deployed to publish-bicep August 2, 2025 21:17 — with GitHub Actions Inactive

lakshmimsft force-pushed the lakshmimsft/testtf branch from a55a451 to b417b00 Compare August 3, 2025 04:25

lakshmimsft had a problem deploying to functional-tests August 3, 2025 04:25 — with GitHub Actions Failure

lakshmimsft temporarily deployed to publish-bicep August 3, 2025 04:25 — with GitHub Actions Inactive

lakshmimsft added 2 commits August 2, 2025 22:42

try statelocktimeouts

b138f8c

Signed-off-by: lakshmimsft <[email protected]>

updating install to global binary

af097dd

Signed-off-by: lakshmimsft <[email protected]>

lakshmimsft force-pushed the lakshmimsft/testtf branch from b417b00 to af097dd Compare August 3, 2025 05:42

lakshmimsft had a problem deploying to functional-tests August 3, 2025 05:43 — with GitHub Actions Failure

lakshmimsft temporarily deployed to publish-bicep August 3, 2025 05:43 — with GitHub Actions Inactive

lakshmimsft temporarily deployed to publish-bicep August 4, 2025 05:02 — with GitHub Actions Inactive

lakshmimsft had a problem deploying to functional-tests August 4, 2025 05:02 — with GitHub Actions Failure

lakshmimsft force-pushed the lakshmimsft/testtf branch from 86aec85 to 3b5ee35 Compare August 4, 2025 05:34

lakshmimsft had a problem deploying to functional-tests August 4, 2025 05:34 — with GitHub Actions Failure

lakshmimsft force-pushed the lakshmimsft/testtf branch from 35c4520 to 7c9d898 Compare August 5, 2025 07:19

lakshmimsft had a problem deploying to functional-tests August 5, 2025 07:20 — with GitHub Actions Failure

lakshmimsft temporarily deployed to publish-bicep August 5, 2025 07:20 — with GitHub Actions Inactive

lakshmimsft force-pushed the lakshmimsft/testtf branch 2 times, most recently from f947c9d to bc6353d Compare August 5, 2025 17:08

lakshmimsft had a problem deploying to functional-tests August 5, 2025 17:08 — with GitHub Actions Failure

lakshmimsft temporarily deployed to publish-bicep August 5, 2025 17:08 — with GitHub Actions Inactive

lakshmimsft force-pushed the lakshmimsft/testtf branch from bc6353d to 4a02b42 Compare August 5, 2025 17:24

lakshmimsft had a problem deploying to functional-tests August 5, 2025 17:24 — with GitHub Actions Failure

lakshmimsft had a problem deploying to publish-bicep August 5, 2025 17:24 — with GitHub Actions Error

clean up

7624827

Signed-off-by: lakshmimsft <[email protected]>

lakshmimsft force-pushed the lakshmimsft/testtf branch from 4a02b42 to 7624827 Compare August 5, 2025 17:25

lakshmimsft temporarily deployed to functional-tests August 5, 2025 17:25 — with GitHub Actions Inactive

lakshmimsft temporarily deployed to publish-bicep August 5, 2025 17:25 — with GitHub Actions Inactive

lakshmimsft changed the title ~~[WIP] Do Not Review TF Updates~~ Update TF install to shared binary approach Aug 5, 2025

lakshmimsft marked this pull request as ready for review August 5, 2025 17:33

lakshmimsft requested review from a team as code owners August 5, 2025 17:33

lakshmimsft requested review from sylvainsf and vishwahiremat August 5, 2025 17:33

ytimocin reviewed Aug 5, 2025

View reviewed changes

sylvainsf approved these changes Aug 5, 2025

View reviewed changes

vishwahiremat reviewed Aug 5, 2025

View reviewed changes

lakshmimsft merged commit ad7811f into main Aug 5, 2025
36 checks passed

lakshmimsft deleted the lakshmimsft/testtf branch August 5, 2025 20:03

lakshmimsft mentioned this pull request Aug 7, 2025

Refactor functions in tf install #10182

Open

8 tasks

This was referenced Aug 11, 2025

Scheduled long running test failed - Run ID: 16760111680 #10180

Closed

Scheduled functional test (noncloud) failed - Run ID: 16737625426 #10173

Closed

Scheduled functional test (noncloud) failed - Run ID: 16740779880 #10174

Closed

Update TF install to shared binary approach #10147

Update TF install to shared binary approach #10147

Uh oh!

Conversation

lakshmimsft commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Contributor checklist

Uh oh!

github-actions bot commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Tests

Uh oh!

codecov bot commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

radius-functional-tests bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Radius functional test overview

Test Status

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lakshmimsft Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylvainsf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lakshmimsft Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lakshmimsft commented Aug 1, 2025 •

edited

Loading

github-actions bot commented Aug 1, 2025 •

edited

Loading

codecov bot commented Aug 1, 2025 •

edited

Loading

radius-functional-tests bot commented Aug 5, 2025 •

edited

Loading

lakshmimsft Aug 5, 2025 •

edited

Loading

lakshmimsft Aug 5, 2025 •

edited

Loading