From c202012cf27b3f8007f38c4f68ff748a5cb21d04 Mon Sep 17 00:00:00 2001 From: Anuj Singh Date: Fri, 23 Oct 2020 15:22:57 -0700 Subject: [PATCH] fsxwindowsfileserver: fixing task concurrency issues with drive assignment --- agent/api/task/task_windows.go | 6 -- agent/api/task/task_windows_test.go | 2 +- .../fsxwindowsfileserver_windows.go | 70 +++++++++++++------ .../fsxwindowsfileserver_windows_test.go | 2 +- 4 files changed, 49 insertions(+), 31 deletions(-) diff --git a/agent/api/task/task_windows.go b/agent/api/task/task_windows.go index 705b3e5d33b..efd19de3629 100644 --- a/agent/api/task/task_windows.go +++ b/agent/api/task/task_windows.go @@ -225,18 +225,12 @@ func (task *Task) addFSxWindowsFileServerResource( vol *TaskVolume, fsxWindowsFileServerVol *fsxwindowsfileserver.FSxWindowsFileServerVolumeConfig, ) error { - hostPath, err := utils.FindUnusedDriveLetter() - if err != nil { - return err - } - fsxwindowsfileserverResource, err := fsxwindowsfileserver.NewFSxWindowsFileServerResource( task.Arn, cfg.AWSRegion, vol.Name, FSxWindowsFileServerVolumeType, fsxWindowsFileServerVol, - hostPath, task.ExecutionCredentialsID, credentialsManager, resourceFields.SSMClientCreator, diff --git a/agent/api/task/task_windows_test.go b/agent/api/task/task_windows_test.go index 1d68db506ee..9f5e6e13af5 100644 --- a/agent/api/task/task_windows_test.go +++ b/agent/api/task/task_windows_test.go @@ -716,7 +716,7 @@ func TestPostUnmarshalTaskWithFSxWindowsFileServerVolumes(t *testing.T) { "credentialsParameter": "arn", "domain": "test" }, - "fsxWindowsFileServerHostPath": "Z:\\" + "fsxWindowsFileServerHostPath": "" }, "createdAt": "0001-01-01T00:00:00Z", "desiredStatus": "NONE", diff --git a/agent/taskresource/fsxwindowsfileserver/fsxwindowsfileserver_windows.go b/agent/taskresource/fsxwindowsfileserver/fsxwindowsfileserver_windows.go index 8948193f051..4ad0959f9f5 100644 --- a/agent/taskresource/fsxwindowsfileserver/fsxwindowsfileserver_windows.go +++ b/agent/taskresource/fsxwindowsfileserver/fsxwindowsfileserver_windows.go @@ -107,7 +107,6 @@ func NewFSxWindowsFileServerResource( name string, volumeType string, volumeConfig *FSxWindowsFileServerVolumeConfig, - hostPath string, executionCredentialsID string, credentialsManager credentials.Manager, ssmClientCreator ssmfactory.SSMClientCreator, @@ -121,7 +120,6 @@ func NewFSxWindowsFileServerResource( FileSystemID: volumeConfig.FileSystemID, RootDirectory: volumeConfig.RootDirectory, AuthConfig: volumeConfig.AuthConfig, - HostPath: hostPath, }, taskARN: taskARN, region: region, @@ -357,28 +355,26 @@ func (fv *FSxWindowsFileServerResource) SetRootDirectory(rootDirectory string) { fv.VolumeConfig.RootDirectory = rootDirectory } -var DriveLetterAvailable = utils.IsAvailableDriveLetter +// GetHostPath safely returns the volume config's host path +func (fv *FSxWindowsFileServerResource) GetHostPath() string { + fv.lock.RLock() + defer fv.lock.RUnlock() + + return fv.VolumeConfig.HostPath +} + +// SetHostPath safely updates volume config's host path +func (fv *FSxWindowsFileServerResource) SetHostPath(hostPath string) { + fv.lock.Lock() + defer fv.lock.Unlock() + + fv.VolumeConfig.HostPath = hostPath +} // Create is used to create all the fsxwindowsfileserver resources for a given task func (fv *FSxWindowsFileServerResource) Create() error { var err error - volumeConfig := fv.GetVolumeConfig() - localPath := volumeConfig.HostPath - - // formatting to keep powershell happy - localPathArg := fmt.Sprintf("-LocalPath \"%s\"", strings.Trim(localPath, `\`)) - - // remove mount if localPath is not available - // handling case where agent crashes after mount and before resource status is updated - if !(DriveLetterAvailable(localPath)) { - err = fv.removeHostMount(localPathArg) - if err != nil { - seelog.Errorf("Failed to remove existing fsxwindowsfileserver resource mount on the container instance: %v", err) - fv.setTerminalReason(err.Error()) - return err - } - } var iamCredentials credentials.IAMRoleCredentials executionCredentials, ok := fv.credentialsManager.GetTaskCredentials(fv.GetExecutionCredentialsID()) @@ -418,7 +414,7 @@ func (fv *FSxWindowsFileServerResource) Create() error { password := creds.Password - err = fv.performHostMount(remotePath, localPathArg, username, password) + err = fv.performHostMount(remotePath, username, password) if err != nil { fv.setTerminalReason(err.Error()) return err @@ -519,9 +515,37 @@ func (fv *FSxWindowsFileServerResource) retrieveFileSystemDNSName(fileSystemId s return nil } +var mountLock sync.Mutex +var DriveLetterAvailable = utils.IsAvailableDriveLetter var execCommand = exec.Command -func (fv *FSxWindowsFileServerResource) performHostMount(remotePath string, localPathArg string, username string, password string) error { +func (fv *FSxWindowsFileServerResource) performHostMount(remotePath string, username string, password string) error { + // mountLock is a package-level mutex lock. + // It is used to avoid a scenario where concurrent tasks get the same drive letter and perform host mount at the same time. + // A drive letter is free until host mount is performed, a lock prevents multiple tasks getting the same drive letter. + mountLock.Lock() + defer mountLock.Unlock() + + hostPath, err := utils.FindUnusedDriveLetter() + if err != nil { + return err + } + fv.SetHostPath(hostPath) + + // formatting to keep powershell happy + localPathArg := fmt.Sprintf("-LocalPath \"%s\"", strings.Trim(hostPath, `\`)) + + // remove mount if localPath is not available + // handling case where agent crashes after mount and before resource status is updated + if !(DriveLetterAvailable(hostPath)) { + err = fv.removeHostMount(localPathArg) + if err != nil { + seelog.Errorf("Failed to remove existing fsxwindowsfileserver resource mount on the container instance: %v", err) + fv.setTerminalReason(err.Error()) + return err + } + } + // formatting to keep powershell happy creds := fmt.Sprintf("-Credential $(New-Object System.Management.Automation.PSCredential(\"%s\", $(ConvertTo-SecureString \"%s\" -AsPlainText -Force)))", username, password) remotePathArg := fmt.Sprintf("-RemotePath \"%s\"", remotePath) @@ -537,7 +561,7 @@ func (fv *FSxWindowsFileServerResource) performHostMount(remotePath string, loca "-RequirePrivacy $true", "-ErrorAction Stop") - _, err := cmd.CombinedOutput() + _, err = cmd.CombinedOutput() if err != nil { seelog.Errorf("Failed to map fsxwindowsfileserver resource on the container instance: %v", err) fv.setTerminalReason(err.Error()) @@ -576,7 +600,7 @@ func (fv *FSxWindowsFileServerResource) removeHostMount(localPathArg string) err } func (fv *FSxWindowsFileServerResource) Cleanup() error { - localPath := fv.VolumeConfig.HostPath + localPath := fv.GetHostPath() // formatting to keep powershell happy localPathArg := fmt.Sprintf("-LocalPath \"%s\"", strings.Trim(localPath, `\`)) err := fv.removeHostMount(localPathArg) diff --git a/agent/taskresource/fsxwindowsfileserver/fsxwindowsfileserver_windows_test.go b/agent/taskresource/fsxwindowsfileserver/fsxwindowsfileserver_windows_test.go index 16c9fdd8659..2c9eee70ebd 100644 --- a/agent/taskresource/fsxwindowsfileserver/fsxwindowsfileserver_windows_test.go +++ b/agent/taskresource/fsxwindowsfileserver/fsxwindowsfileserver_windows_test.go @@ -411,7 +411,7 @@ func TestPerformHostMount(t *testing.T) { execCommand = fakeExecCommand defer func() { execCommand = exec.Command }() - err := fv.performHostMount(`\\amznfsxfp8sdlcw.test.corp.com\share`, hostPath, `test\user`, `pass`) + err := fv.performHostMount(`\\amznfsxfp8sdlcw.test.corp.com\share`, `test\user`, `pass`) assert.NoError(t, err) }