Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit6651c16

Browse files
authored
fix: avoid terraform state concurrent access, remove global mutex (#5273)
1 parent85a6d14 commit6651c16

File tree

5 files changed

+63
-54
lines changed

5 files changed

+63
-54
lines changed

‎cli/server.go‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,14 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co
112112
notifyCtx,notifyStop:=signal.NotifyContext(ctx,InterruptSignals...)
113113
defernotifyStop()
114114

115+
// Ensure we have a unique cache directory for this process.
116+
cacheDir:=filepath.Join(cfg.CacheDirectory.Value,uuid.NewString())
117+
err=os.MkdirAll(cacheDir,0o700)
118+
iferr!=nil {
119+
returnxerrors.Errorf("create cache directory: %w",err)
120+
}
121+
deferos.RemoveAll(cacheDir)
122+
115123
// Clean up idle connections at the end, e.g.
116124
// embedded-postgres can leave an idle connection
117125
// which is caught by goleaks.
@@ -355,7 +363,7 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co
355363
Database:databasefake.New(),
356364
DERPMap:derpMap,
357365
Pubsub:database.NewPubsubInMemory(),
358-
CacheDir:cfg.CacheDirectory.Value,
366+
CacheDir:cacheDir,
359367
GoogleTokenValidator:googleTokenValidator,
360368
GitAuthConfigs:gitAuthConfigs,
361369
RealIPConfig:realIPConfig,
@@ -632,7 +640,8 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co
632640
}()
633641
provisionerdMetrics:=provisionerd.NewMetrics(options.PrometheusRegistry)
634642
fori:=0;i<cfg.Provisioner.Daemons.Value;i++ {
635-
daemon,err:=newProvisionerDaemon(ctx,coderAPI,provisionerdMetrics,logger,cfg,errCh,false)
643+
daemonCacheDir:=filepath.Join(cacheDir,fmt.Sprintf("provisioner-%d",i))
644+
daemon,err:=newProvisionerDaemon(ctx,coderAPI,provisionerdMetrics,logger,cfg,daemonCacheDir,errCh,false)
636645
iferr!=nil {
637646
returnxerrors.Errorf("create provisioner daemon: %w",err)
638647
}
@@ -902,6 +911,7 @@ func newProvisionerDaemon(
902911
metrics provisionerd.Metrics,
903912
logger slog.Logger,
904913
cfg*codersdk.DeploymentConfig,
914+
cacheDirstring,
905915
errChchanerror,
906916
devbool,
907917
) (srv*provisionerd.Server,errerror) {
@@ -912,9 +922,9 @@ func newProvisionerDaemon(
912922
}
913923
}()
914924

915-
err=os.MkdirAll(cfg.CacheDirectory.Value,0o700)
925+
err=os.MkdirAll(cacheDir,0o700)
916926
iferr!=nil {
917-
returnnil,xerrors.Errorf("mkdir %q: %w",cfg.CacheDirectory.Value,err)
927+
returnnil,xerrors.Errorf("mkdir %q: %w",cacheDir,err)
918928
}
919929

920930
terraformClient,terraformServer:=provisionersdk.MemTransportPipe()
@@ -930,7 +940,7 @@ func newProvisionerDaemon(
930940
ServeOptions:&provisionersdk.ServeOptions{
931941
Listener:terraformServer,
932942
},
933-
CachePath:cfg.CacheDirectory.Value,
943+
CachePath:cacheDir,
934944
Logger:logger,
935945
})
936946
iferr!=nil&&!xerrors.Is(err,context.Canceled) {

‎provisioner/terraform/executor.go‎

Lines changed: 34 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -23,27 +23,15 @@ import (
2323
"github.com/coder/coder/provisionersdk/proto"
2424
)
2525

26-
// initMut is a global mutex that protects the Terraform cache directory from
27-
// concurrent usage by path. Only `terraform init` commands are guarded by this
28-
// mutex.
29-
//
30-
// When cache path is set, we must protect against multiple calls to
31-
// `terraform init`.
32-
//
33-
// From the Terraform documentation:
34-
//
35-
//Note: The plugin cache directory is not guaranteed to be concurrency
36-
//safe. The provider installer's behavior in environments with multiple
37-
//terraform init calls is undefined.
38-
varinitMut=&sync.Mutex{}
39-
4026
typeexecutorstruct {
27+
mut*sync.Mutex
4128
binaryPathstring
42-
cachePathstring
43-
workdirstring
29+
// cachePath and workdir must not be used by multiple processes at once.
30+
cachePathstring
31+
workdirstring
4432
}
4533

46-
func (eexecutor)basicEnv() []string {
34+
func (e*executor)basicEnv() []string {
4735
// Required for "terraform init" to find "git" to
4836
// clone Terraform modules.
4937
env:=safeEnviron()
@@ -55,7 +43,8 @@ func (e executor) basicEnv() []string {
5543
returnenv
5644
}
5745

58-
func (eexecutor)execWriteOutput(ctx,killCtx context.Context,args,env []string,stdOutWriter,stdErrWriter io.WriteCloser) (errerror) {
46+
// execWriteOutput must only be called while the lock is held.
47+
func (e*executor)execWriteOutput(ctx,killCtx context.Context,args,env []string,stdOutWriter,stdErrWriter io.WriteCloser) (errerror) {
5948
deferfunc() {
6049
closeErr:=stdOutWriter.Close()
6150
iferr==nil&&closeErr!=nil {
@@ -98,7 +87,8 @@ func (e executor) execWriteOutput(ctx, killCtx context.Context, args, env []stri
9887
returncmd.Wait()
9988
}
10089

101-
func (eexecutor)execParseJSON(ctx,killCtx context.Context,args,env []string,vinterface{})error {
90+
// execParseJSON must only be called while the lock is held.
91+
func (e*executor)execParseJSON(ctx,killCtx context.Context,args,env []string,vinterface{})error {
10292
ifctx.Err()!=nil {
10393
returnctx.Err()
10494
}
@@ -133,7 +123,7 @@ func (e executor) execParseJSON(ctx, killCtx context.Context, args, env []string
133123
returnnil
134124
}
135125

136-
func (eexecutor)checkMinVersion(ctx context.Context)error {
126+
func (e*executor)checkMinVersion(ctx context.Context)error {
137127
v,err:=e.version(ctx)
138128
iferr!=nil {
139129
returnerr
@@ -147,7 +137,8 @@ func (e executor) checkMinVersion(ctx context.Context) error {
147137
returnnil
148138
}
149139

150-
func (eexecutor)version(ctx context.Context) (*version.Version,error) {
140+
// version doesn't need the lock because it doesn't read or write to any state.
141+
func (e*executor)version(ctx context.Context) (*version.Version,error) {
151142
returnversionFromBinaryPath(ctx,e.binaryPath)
152143
}
153144

@@ -177,7 +168,10 @@ func versionFromBinaryPath(ctx context.Context, binaryPath string) (*version.Ver
177168
returnversion.NewVersion(vj.Version)
178169
}
179170

180-
func (eexecutor)init(ctx,killCtx context.Context,logrlogSink)error {
171+
func (e*executor)init(ctx,killCtx context.Context,logrlogSink)error {
172+
e.mut.Lock()
173+
defere.mut.Unlock()
174+
181175
outWriter,doneOut:=logWriter(logr,proto.LogLevel_DEBUG)
182176
errWriter,doneErr:=logWriter(logr,proto.LogLevel_ERROR)
183177
deferfunc() {
@@ -193,23 +187,14 @@ func (e executor) init(ctx, killCtx context.Context, logr logSink) error {
193187
"-input=false",
194188
}
195189

196-
// When cache path is set, we must protect against multiple calls
197-
// to `terraform init`.
198-
//
199-
// From the Terraform documentation:
200-
// Note: The plugin cache directory is not guaranteed to be
201-
// concurrency safe. The provider installer's behavior in
202-
// environments with multiple terraform init calls is undefined.
203-
ife.cachePath!="" {
204-
initMut.Lock()
205-
deferinitMut.Unlock()
206-
}
207-
208190
returne.execWriteOutput(ctx,killCtx,args,e.basicEnv(),outWriter,errWriter)
209191
}
210192

211193
// revive:disable-next-line:flag-parameter
212-
func (eexecutor)plan(ctx,killCtx context.Context,env,vars []string,logrlogSink,destroybool) (*proto.Provision_Response,error) {
194+
func (e*executor)plan(ctx,killCtx context.Context,env,vars []string,logrlogSink,destroybool) (*proto.Provision_Response,error) {
195+
e.mut.Lock()
196+
defere.mut.Unlock()
197+
213198
planfilePath:=filepath.Join(e.workdir,"terraform.tfplan")
214199
args:= []string{
215200
"plan",
@@ -257,7 +242,8 @@ func (e executor) plan(ctx, killCtx context.Context, env, vars []string, logr lo
257242
},nil
258243
}
259244

260-
func (eexecutor)planResources(ctx,killCtx context.Context,planfilePathstring) ([]*proto.Resource,error) {
245+
// planResources must only be called while the lock is held.
246+
func (e*executor)planResources(ctx,killCtx context.Context,planfilePathstring) ([]*proto.Resource,error) {
261247
plan,err:=e.showPlan(ctx,killCtx,planfilePath)
262248
iferr!=nil {
263249
returnnil,xerrors.Errorf("show terraform plan file: %w",err)
@@ -270,14 +256,16 @@ func (e executor) planResources(ctx, killCtx context.Context, planfilePath strin
270256
returnConvertResources(plan.PlannedValues.RootModule,rawGraph)
271257
}
272258

273-
func (eexecutor)showPlan(ctx,killCtx context.Context,planfilePathstring) (*tfjson.Plan,error) {
259+
// showPlan must only be called while the lock is held.
260+
func (e*executor)showPlan(ctx,killCtx context.Context,planfilePathstring) (*tfjson.Plan,error) {
274261
args:= []string{"show","-json","-no-color",planfilePath}
275262
p:=new(tfjson.Plan)
276263
err:=e.execParseJSON(ctx,killCtx,args,e.basicEnv(),p)
277264
returnp,err
278265
}
279266

280-
func (eexecutor)graph(ctx,killCtx context.Context) (string,error) {
267+
// graph must only be called while the lock is held.
268+
func (e*executor)graph(ctx,killCtx context.Context) (string,error) {
281269
ifctx.Err()!=nil {
282270
return"",ctx.Err()
283271
}
@@ -302,9 +290,12 @@ func (e executor) graph(ctx, killCtx context.Context) (string, error) {
302290
}
303291

304292
// revive:disable-next-line:flag-parameter
305-
func (eexecutor)apply(
293+
func (e*executor)apply(
306294
ctx,killCtx context.Context,plan []byte,env []string,logrlogSink,
307295
) (*proto.Provision_Response,error) {
296+
e.mut.Lock()
297+
defere.mut.Unlock()
298+
308299
planFile,err:=ioutil.TempFile("","coder-terrafrom-plan")
309300
iferr!=nil {
310301
returnnil,xerrors.Errorf("create plan file: %w",err)
@@ -356,7 +347,8 @@ func (e executor) apply(
356347
},nil
357348
}
358349

359-
func (eexecutor)stateResources(ctx,killCtx context.Context) ([]*proto.Resource,error) {
350+
// stateResources must only be called while the lock is held.
351+
func (e*executor)stateResources(ctx,killCtx context.Context) ([]*proto.Resource,error) {
360352
state,err:=e.state(ctx,killCtx)
361353
iferr!=nil {
362354
returnnil,err
@@ -375,7 +367,8 @@ func (e executor) stateResources(ctx, killCtx context.Context) ([]*proto.Resourc
375367
returnresources,nil
376368
}
377369

378-
func (eexecutor)state(ctx,killCtx context.Context) (*tfjson.State,error) {
370+
// state must only be called while the lock is held.
371+
func (e*executor)state(ctx,killCtx context.Context) (*tfjson.State,error) {
379372
args:= []string{"show","-json","-no-color"}
380373
state:=&tfjson.State{}
381374
err:=e.execParseJSON(ctx,killCtx,args,e.basicEnv(),state)

‎provisioner/terraform/provision_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ func readProvisionLog(t *testing.T, response proto.DRPCProvisioner_ProvisionClie
7777

7878
iflog:=msg.GetLog();log!=nil {
7979
t.Log(log.Level.String(),log.Output)
80-
logBuf.WriteString(log.Output)
80+
_,_=logBuf.WriteString(log.Output)
8181
}
8282
ifc=msg.GetComplete();c!=nil {
8383
require.Empty(t,c.Error)
@@ -190,8 +190,6 @@ func TestProvision_Cancel(t *testing.T) {
190190
funcTestProvision(t*testing.T) {
191191
t.Parallel()
192192

193-
ctx,api:=setupProvisioner(t,nil)
194-
195193
testCases:= []struct {
196194
Namestring
197195
Filesmap[string]string
@@ -329,6 +327,8 @@ func TestProvision(t *testing.T) {
329327
t.Run(testCase.Name,func(t*testing.T) {
330328
t.Parallel()
331329

330+
ctx,api:=setupProvisioner(t,nil)
331+
332332
directory:=t.TempDir()
333333
forpath,content:=rangetestCase.Files {
334334
err:=os.WriteFile(filepath.Join(directory,path), []byte(content),0o600)

‎provisioner/terraform/serve.go‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package terraform
33
import (
44
"context"
55
"path/filepath"
6+
"sync"
67
"time"
78

89
"github.com/cli/safeexec"
@@ -22,8 +23,9 @@ type ServeOptions struct {
2223
// BinaryPath specifies the "terraform" binary to use.
2324
// If omitted, the $PATH will attempt to find it.
2425
BinaryPathstring
25-
CachePathstring
26-
Logger slog.Logger
26+
// CachePath must not be used by multiple processes at once.
27+
CachePathstring
28+
Logger slog.Logger
2729

2830
// ExitTimeout defines how long we will wait for a running Terraform
2931
// command to exit (cleanly) if the provision was stopped. This only
@@ -91,6 +93,7 @@ func Serve(ctx context.Context, options *ServeOptions) error {
9193
options.ExitTimeout=defaultExitTimeout
9294
}
9395
returnprovisionersdk.Serve(ctx,&server{
96+
execMut:&sync.Mutex{},
9497
binaryPath:options.BinaryPath,
9598
cachePath:options.CachePath,
9699
logger:options.Logger,
@@ -99,14 +102,16 @@ func Serve(ctx context.Context, options *ServeOptions) error {
99102
}
100103

101104
typeserverstruct {
105+
execMut*sync.Mutex
102106
binaryPathstring
103107
cachePathstring
104108
logger slog.Logger
105109
exitTimeout time.Duration
106110
}
107111

108-
func (s*server)executor(workdirstring)executor {
109-
returnexecutor{
112+
func (s*server)executor(workdirstring)*executor {
113+
return&executor{
114+
mut:s.execMut,
110115
binaryPath:s.binaryPath,
111116
cachePath:s.cachePath,
112117
workdir:workdir,

‎provisionerd/provisionerd.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ type Options struct {
5757
JobPollJitter time.Duration
5858
JobPollDebounce time.Duration
5959
ProvisionersProvisioners
60-
WorkDirectorystring
60+
// WorkDirectory must not be used by multiple processes at once.
61+
WorkDirectorystring
6162
}
6263

6364
// New creates and starts a provisioner daemon.

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp