-
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
Merged
mnaamani
merged 7 commits into
Joystream:master
from
mnaamani:colossus-specify-temp-folder
Dec 26, 2023
Merged
temp folder refactor #5000
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
06b200b
storage-node: temp folder refactor
mnaamani cb1412a
improve initializing uploads and temp directories
mnaamani 967ee7e
storage-node: cleaner code
mnaamani e4903ce
storage-node: ensure logs folder is not same as uploads folder
mnaamani d2698f3
Merge branch 'master' into colossus-specify-temp-folder
mnaamani a287fdc
storage-node: update warning and argument description for tempFolder
mnaamani 3dd8e0a
storage-node: use tempFolder arg in docker-compose
mnaamani File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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,7 @@ 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' | ||||||||||
import { v4 as uuidv4 } from 'uuid' | ||||||||||
const fsPromises = fs.promises | ||||||||||
|
||||||||||
|
@@ -53,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).\nIf not specified a subfolder under the uploads directory will be used.', | ||||||||||
}), | ||||||||||
port: flags.integer({ | ||||||||||
char: 'o', | ||||||||||
required: true, | ||||||||||
|
@@ -158,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, | ||||||||||
|
@@ -176,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))) { | ||||||||||
|
@@ -220,16 +228,26 @@ 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( | ||||||||||
'You did not specify a path to the temporary directory. ' + | ||||||||||
'A temp folder under the uploads folder willl be used. ' + | ||||||||||
'In a future release passing an absolute path to a temporary directory with the ' + | ||||||||||
'"tempFolder" argument will be required.' | ||||||||||
) | ||||||||||
} | ||||||||||
|
||||||||||
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 X_HOST_ID = uuidv4() | ||||||||||
|
||||||||||
const pendingDataObjectsDir = path.join(flags.uploads, PendingDirName) | ||||||||||
|
@@ -259,7 +277,7 @@ Supported values: warn, error, debug, info. Default:debug`, | |||||||||
selectedBuckets, | ||||||||||
qnApi, | ||||||||||
flags.uploads, | ||||||||||
TempDirName, | ||||||||||
tempFolder, | ||||||||||
flags.syncWorkersNumber, | ||||||||||
flags.syncWorkersTimeout, | ||||||||||
flags.syncInterval, | ||||||||||
|
@@ -297,7 +315,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({ | ||||||||||
|
@@ -308,7 +325,7 @@ Supported values: warn, error, debug, info. Default:debug`, | |||||||||
workerId, | ||||||||||
maxFileSize, | ||||||||||
uploadsDir: flags.uploads, | ||||||||||
tempFileUploadingDir, | ||||||||||
tempFileUploadingDir: tempFolder, | ||||||||||
pendingDataObjectsDir, | ||||||||||
acceptPendingObjectsService, | ||||||||||
process: this.config, | ||||||||||
|
@@ -426,26 +443,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> { | ||||||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?