-
Notifications
You must be signed in to change notification settings - Fork 115
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
temp folder refactor #5000
temp folder refactor #5000
Changes from 4 commits
06b200b
cb1412a
967ee7e
e4903ce
d2698f3
a287fdc
3dd8e0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1,14 +1,11 @@ | ||||||||||
import { flags } from '@oclif/command' | ||||||||||
import { ApiPromise } from '@polkadot/api' | ||||||||||
import { KeyringPair } from '@polkadot/keyring/types' | ||||||||||
import { PalletStorageStorageBucketRecord } from '@polkadot/types/lookup' | ||||||||||
import fs from 'fs' | ||||||||||
import sleep from 'sleep-promise' | ||||||||||
import _ from 'lodash' | ||||||||||
import path from 'path' | ||||||||||
import rimraf from 'rimraf' | ||||||||||
import sleep from 'sleep-promise' | ||||||||||
import { promisify } from 'util' | ||||||||||
import ApiCommandBase from '../command-base/ApiCommandBase' | ||||||||||
import fs from 'fs' | ||||||||||
import { PalletStorageStorageBucketRecord } from '@polkadot/types/lookup' | ||||||||||
import { KeyringPair } from '@polkadot/keyring/types' | ||||||||||
import { customFlags } from '../command-base/CustomFlags' | ||||||||||
import { loadDataObjectIdCache } from '../services/caching/localDataObjects' | ||||||||||
import logger, { DatePatternByFrequency, Frequency, initNewLogger } from '../services/logger' | ||||||||||
|
@@ -23,6 +20,8 @@ import { getStorageBucketIdsByWorkerId } from '../services/sync/storageObligatio | |||||||||
import { PendingDirName, performSync, TempDirName } from '../services/sync/synchronizer' | ||||||||||
import { createApp } from '../services/webApi/app' | ||||||||||
import ExitCodes from './../command-base/ExitCodes' | ||||||||||
import ApiCommandBase from '../command-base/ApiCommandBase' | ||||||||||
|
||||||||||
const fsPromises = fs.promises | ||||||||||
|
||||||||||
/** | ||||||||||
|
@@ -52,6 +51,10 @@ export default class Server extends ApiCommandBase { | |||||||||
required: true, | ||||||||||
description: 'Data uploading directory (absolute path).', | ||||||||||
}), | ||||||||||
tempFolder: flags.string({ | ||||||||||
description: | ||||||||||
'Directory to store tempory files during sync and upload (absolute path).\n,Temporary directory (absolute path). If not specified a subfolder under the uploads directory will be used.', | ||||||||||
}), | ||||||||||
port: flags.integer({ | ||||||||||
char: 'o', | ||||||||||
required: true, | ||||||||||
|
@@ -157,11 +160,19 @@ Supported values: warn, error, debug, info. Default:debug`, | |||||||||
async run(): Promise<void> { | ||||||||||
const { flags } = this.parse(Server) | ||||||||||
|
||||||||||
const logSource = `StorageProvider_${flags.worker}` | ||||||||||
const api = await this.getApi() | ||||||||||
|
||||||||||
if (flags.dev) { | ||||||||||
await this.ensureDevelopmentChain() | ||||||||||
} | ||||||||||
|
||||||||||
if (flags.logFilePath && path.relative(flags.logFilePath, flags.uploads) === '') { | ||||||||||
this.error('Paths for logs and uploads must be unique.') | ||||||||||
} | ||||||||||
|
||||||||||
if (!_.isEmpty(flags.elasticSearchEndpoint) || !_.isEmpty(flags.logFilePath)) { | ||||||||||
initNewLogger({ | ||||||||||
elasticSearchlogSource: logSource, | ||||||||||
elasticSearchlogSource: `StorageProvider_${flags.worker}`, | ||||||||||
elasticSearchEndpoint: flags.elasticSearchEndpoint, | ||||||||||
elasticSearchIndexPrefix: flags.elasticSearchIndexPrefix, | ||||||||||
elasticSearchUser: flags.elasticSearchUser, | ||||||||||
|
@@ -175,8 +186,6 @@ Supported values: warn, error, debug, info. Default:debug`, | |||||||||
|
||||||||||
logger.info(`Query node endpoint set: ${flags.queryNodeEndpoint}`) | ||||||||||
|
||||||||||
const api = await this.getApi() | ||||||||||
|
||||||||||
const workerId = flags.worker | ||||||||||
|
||||||||||
if (!(await verifyWorkerId(api, workerId))) { | ||||||||||
|
@@ -219,16 +228,25 @@ Supported values: warn, error, debug, info. Default:debug`, | |||||||||
const enableUploadingAuth = false | ||||||||||
const operatorRoleKey = undefined | ||||||||||
|
||||||||||
await recreateTempDirectory(flags.uploads, TempDirName) | ||||||||||
|
||||||||||
if (fs.existsSync(flags.uploads)) { | ||||||||||
await loadDataObjectIdCache(flags.uploads, TempDirName, PendingDirName) | ||||||||||
if (!flags.tempFolder) { | ||||||||||
logger.warn( | ||||||||||
'It is recommended to specify a unique file path for temporary files.' + | ||||||||||
'For now a temp folder under the uploads folder will be used.' + | ||||||||||
'In future this will be warning will become and error!' | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can you please rephrase that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will rephrase the whole warning to:
WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in a287fdc |
||||||||||
) | ||||||||||
} | ||||||||||
|
||||||||||
if (flags.dev) { | ||||||||||
await this.ensureDevelopmentChain() | ||||||||||
const tempFolder = flags.tempFolder || path.join(flags.uploads, TempDirName) | ||||||||||
|
||||||||||
if (path.relative(tempFolder, flags.uploads) === '') { | ||||||||||
this.error('Paths for temporary and uploads folders must be unique.') | ||||||||||
Comment on lines
+242
to
+243
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Please see the other comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. commented here #5000 (comment) |
||||||||||
} | ||||||||||
|
||||||||||
await createDirectory(flags.uploads) | ||||||||||
await loadDataObjectIdCache(flags.uploads) | ||||||||||
|
||||||||||
await createDirectory(tempFolder) | ||||||||||
|
||||||||||
const pendingDataObjectsDir = path.join(flags.uploads, PendingDirName) | ||||||||||
|
||||||||||
const acceptPendingObjectsService = await AcceptPendingObjectsService.create( | ||||||||||
|
@@ -256,7 +274,7 @@ Supported values: warn, error, debug, info. Default:debug`, | |||||||||
selectedBuckets, | ||||||||||
qnApi, | ||||||||||
flags.uploads, | ||||||||||
TempDirName, | ||||||||||
tempFolder, | ||||||||||
flags.syncWorkersNumber, | ||||||||||
flags.syncWorkersTimeout, | ||||||||||
flags.syncInterval, | ||||||||||
|
@@ -292,7 +310,6 @@ Supported values: warn, error, debug, info. Default:debug`, | |||||||||
try { | ||||||||||
const port = flags.port | ||||||||||
const maxFileSize = await api.consts.storage.maxDataObjectSize.toNumber() | ||||||||||
const tempFileUploadingDir = path.join(flags.uploads, TempDirName) | ||||||||||
logger.debug(`Max file size runtime parameter: ${maxFileSize}`) | ||||||||||
|
||||||||||
const app = await createApp({ | ||||||||||
|
@@ -303,7 +320,7 @@ Supported values: warn, error, debug, info. Default:debug`, | |||||||||
workerId, | ||||||||||
maxFileSize, | ||||||||||
uploadsDir: flags.uploads, | ||||||||||
tempFileUploadingDir, | ||||||||||
tempFileUploadingDir: tempFolder, | ||||||||||
pendingDataObjectsDir, | ||||||||||
acceptPendingObjectsService, | ||||||||||
process: this.config, | ||||||||||
|
@@ -418,26 +435,14 @@ async function runCleanupWithInterval( | |||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* Removes and recreates the temporary directory from the uploading directory. | ||||||||||
* All files in the temp directory are deleted. | ||||||||||
* Creates a directory recursivly. Like `mkdir -p` | ||||||||||
* | ||||||||||
* @param uploadsDirectory - data uploading directory | ||||||||||
* @param tempDirName - temporary directory name within the uploading directory | ||||||||||
* @param tempDirName - full path to temporary directory | ||||||||||
* @returns void promise. | ||||||||||
*/ | ||||||||||
async function recreateTempDirectory(uploadsDirectory: string, tempDirName: string): Promise<void> { | ||||||||||
try { | ||||||||||
const tempFileUploadingDir = path.join(uploadsDirectory, tempDirName) | ||||||||||
|
||||||||||
logger.info(`Removing temp directory ...`) | ||||||||||
const rimrafAsync = promisify(rimraf) | ||||||||||
await rimrafAsync(tempFileUploadingDir) | ||||||||||
|
||||||||||
logger.info(`Creating temp directory ...`) | ||||||||||
await fsPromises.mkdir(tempFileUploadingDir) | ||||||||||
} catch (err) { | ||||||||||
logger.error(`Temp directory IO error: ${err}`) | ||||||||||
} | ||||||||||
async function createDirectory(dirName: string): Promise<void> { | ||||||||||
logger.info(`Creating directory ${dirName}`) | ||||||||||
await fsPromises.mkdir(dirName, { recursive: true }) | ||||||||||
} | ||||||||||
|
||||||||||
async function verifyWorkerId(api: ApiPromise, workerId: number): Promise<boolean> { | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be
path.relative(flags.logFilePath, flags.uploads) === '..'
, I tested thepath.relative
function aspath.relative('/uploads/temp', '/uploads')
and I got..
as return valueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is was only trying to protect against the two paths being the same location. I was not concerned with temp or logs being subfolders of the main uploads directory. That is why I was comparing with
''
null string.Originally I was doing
if (lags.logFilePath === flags.uploads) { }
but I decided to usepath.relative()
to take into account trailing/
path separator..Given that I added proper filtering of filenames when they are loaded from uploads directory, and if we assume the file names of logs and temp files will not clash with the object id name, perhaps this constraint is not really necessary at all?