Add retry policy for azureStorage #1480
Conversation
merge master
…-fix-azureUpload
| fileServerClient.createDirectoryIfNotExists(azureShare, azureFoler, (error: any, result: any, response: any) => { | ||
| if (error) { | ||
| getLogger() | ||
| .error(`Create directory failed:, ${error}`); | ||
| deferred.reject(error); | ||
| deferred.resolve(false); |
| `${this.azureStorageShare}/${path.join('nni', getExperimentId(), trialJobId, 'output')}`; | ||
| while (retryCount >= 0) { | ||
| //upload local files, including scripts for running the trial and configuration (e.g., hyperparameters) for the trial, to azure storage | ||
| let resultUploadDir1: boolean = await AzureStorageClientUtility.uploadDirectory(this.azureStorageClient, |
There was a problem hiding this comment.
Define meaningful name, how about resultUploadNNIScript
There was a problem hiding this comment.
fixed, rename to resultUploadNNIScript
| `nni/${getExperimentId()}/${trialJobId}`, this.azureStorageShare, | ||
| `${trialLocalTempFolder}`); | ||
| //upload code files to azure storage | ||
| let resultUploadDir2: boolean = await AzureStorageClientUtility.uploadDirectory(this.azureStorageClient, |
There was a problem hiding this comment.
The same, resultUploadCodeFile
There was a problem hiding this comment.
fixed, rename to resultUploadCodeFile
|
|
||
| trialJobOutputUrl = `https://${this.azureStorageAccountName}.file.core.windows.net/` + | ||
| `${this.azureStorageShare}/${path.join('nni', getExperimentId(), trialJobId, 'output')}`; | ||
| while (retryCount >= 0) { |
There was a problem hiding this comment.
It's a little confusion here because even retryCount == 0, we will enter the loop.
I think change to do...while is better to understand:
retryCount = 1
do
{
} while( retyrCount-- > 0)
There was a problem hiding this comment.
fixed, changed logic.
| break; | ||
| } else { | ||
| //wait for 5 seconds to re-upload files | ||
| await delay(5000); |
There was a problem hiding this comment.
Should you differentiate the failure is caused by false resultUploadCodeFile, false resultUploadNNIScript, or both? rather than re-upload both regardless.
There was a problem hiding this comment.
fixed, re-upload two folders separately.
| this.log.info('Re-upload files to azure-storage'); | ||
| retryCount -= 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
these code is duplicated with frameworklauncher training service, consider to do a refactor to remove dup code.
There was a problem hiding this comment.
fixed, extract duplicated code to kubernetesTrainingService.
| await fileServerClient.createFileFromLocalFile(azureShare, azureDirectory, azureFileName, localFilePath, | ||
| (error: any, result: any, response: any) => { | ||
| if (error) { | ||
| getLogger() | ||
| .error(`Upload file failed:, ${error}`); | ||
| deferred.reject(error); | ||
| deferred.resolve(false); |
| @@ -211,16 +211,28 @@ class FrameworkControllerTrainingService extends KubernetesTrainingService imple | |||
| if (this.azureStorageClient === undefined) { | |||
| throw new Error('azureStorageClient is not initialized'); | |||
| } | |||
| let retryCount: number = 1; | |||
There was a problem hiding this comment.
retryCount always equals to 1?
There was a problem hiding this comment.
Add config file for this kind of configuration.
| } else { | ||
| //wait for 5 seconds to re-upload files | ||
| await delay(5000); | ||
| this.log.info('Re-upload files to azure-storage'); |
There was a problem hiding this comment.
'Upload failed, Retry: upload files to azure-storage'
| this.log.info('Re-upload files to azure-storage'); | ||
| retryCount -= 1; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| this.log.error(error); |
There was a problem hiding this comment.
there is no error anymore (becomes false or true). what if the upload is failed after all the retries?
There was a problem hiding this comment.
If trialJobOutputUrl is '', the trial will be set to FAILED
| //return a empty url when got error | ||
| return Promise.resolve(""); | ||
| } | ||
| return Promise.resolve(trialJobOutputUrl); |
There was a problem hiding this comment.
after try retryCount times, the files are still not uploaded, what does this function return?
There was a problem hiding this comment.
this function will return an empty path, and submitTrial function will set trial status FAILED.
There was a problem hiding this comment.
better to write log to tell the FAILED is induced by data upload.
There was a problem hiding this comment.
add log Retry-count is used up, upload files to azureStorage for trial ${trialJobId} failed!
src/nni_manager/config/config.yaml
Outdated
| @@ -0,0 +1 @@ | |||
| azureStorageUploadRetryCount: 1 #If upload files to azure storage failed, NNI will retry the process of uploading, this field will specify the number of attempts to re-upload files. | |||
There was a problem hiding this comment.
putting it here is strange, but let's keep it for now
There was a problem hiding this comment.
changed to put it in config
| @@ -120,7 +120,8 @@ export namespace ValidationSchemas { | |||
| azureStorage: joi.object({ | |||
| accountName: joi.string().regex(/^([0-9]|[a-z]|[A-Z]|-){3,31}$/), | |||
| azureShare: joi.string().regex(/^([0-9]|[a-z]|[A-Z]|-){3,63}$/) | |||
| }) | |||
| }), | |||
| uploadRetryCount: joi.number().min(1) | |||
There was a problem hiding this comment.
suggest to make it a configuration of azureStorage
No description provided.