Skip to content

Tweak transcode handoff on content node to be useful as an upload health check #3351

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.

Sign up for GitHub

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

Merged
dmanjunath merged 11 commits into from
Jul 1, 2022
Merged

Tweak transcode handoff on content node to be useful as an upload health check #3351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5767e29
Try out this new health check pattern
dmanjunath Jun 27, 2022
4c6b0e1
Import fix
dmanjunath Jun 27, 2022
ca24b84
WIP
dmanjunath Jun 29, 2022
f373726
Merge branch 'master' of github.com:AudiusProject/audius-protocol int…
dmanjunath Jun 29, 2022
bafc49e
Should be working content node side
dmanjunath Jun 29, 2022
a2d48f9
lint
dmanjunath Jun 29, 2022
3d66e34
Gate to tmp_track_artifacts
dmanjunath Jun 29, 2022
1f3fece
Merge branch 'master' of github.com:AudiusProject/audius-protocol int…
dmanjunath Jun 29, 2022
22f1681
lint
dmanjunath Jun 29, 2022
3df2716
Final middleware cleanup
dmanjunath Jul 1, 2022
61670d7
Lint
dmanjunath Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions creator-node/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

80 changes: 3 additions & 77 deletions creator-node/src/components/healthCheck/healthCheckController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,7 @@ const { sequelize } = require('../../models')
const { getMonitors } = require('../../monitors/monitors')
const TranscodingQueue = require('../../TranscodingQueue')

const { recoverWallet } = require('../../apiSigning')
const {
handleTrackContentUpload,
removeTrackFolder
} = require('../../fileManager')
const { ensureValidSPMiddleware } = require('../../middlewares')

const config = require('../../config')

Expand All @@ -42,53 +38,6 @@ const FIND_SYNC_REQUESTS_JOB_MAX_LAST_SUCCESSFUL_RUN_DELAY_MS = config.get(
const FIND_REPLICA_SET_UPDATES_JOB_MAX_LAST_SUCCESSFUL_RUN_DELAY_MS =
config.get('findReplicaSetUpdatesJobLastSuccessfulRunDelayMs')

// Helper Functions
/**
* Verifies that the request is made by the delegate Owner
*/
const healthCheckVerifySignature = (req, res, next) => {

@dmanjunath dmanjunath Jul 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a test middleware which we had added but no longer necessary with ensureValidSPMiddleware

@vicky-g vicky-g Jul 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason why we remove these health check helper fns?

@vicky-g vicky-g Jul 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it bc the ensureSPmiddleware does the same thing and we're using the ensureSPmiddleware in favor of this removed block?

@dmanjunath dmanjunath Jul 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a hand rolled middleware specifically for these health check routes. the problem was the script needed the delegatePrivateKey of the node in order to run to run these health checks against the node. the change to ensureSPMiddleware allows any registered content node to run the checks

const { timestamp, randomBytes, signature } = req.query
if (!timestamp || !randomBytes || !signature) {
return sendResponse(
req,
res,
errorResponseBadRequest('Missing required query parameters')
)
}

const recoveryObject = { randomBytesToSign: randomBytes, timestamp }
const recoveredPublicWallet = recoverWallet(
recoveryObject,
signature
).toLowerCase()
const recoveredTimestampDate = new Date(timestamp)
const currentTimestampDate = new Date()
const requestAge = currentTimestampDate - recoveredTimestampDate
if (requestAge >= MAX_HEALTH_CHECK_TIMESTAMP_AGE_MS) {
return sendResponse(
req,
res,
errorResponseBadRequest(
`Submitted timestamp=${recoveredTimestampDate}, current timestamp=${currentTimestampDate}. Maximum age =${MAX_HEALTH_CHECK_TIMESTAMP_AGE_MS}`
)
)
}
const delegateOwnerWallet = config.get('delegateOwnerWallet').toLowerCase()
if (recoveredPublicWallet !== delegateOwnerWallet) {
return sendResponse(
req,
res,
errorResponseBadRequest(
"Requester's public key does does not match Creator Node's delegate owner wallet."
)
)
}

next()
}

// Controllers

/**
* Controller for `health_check` route, calls
* `healthCheckComponentService`.
Expand Down Expand Up @@ -245,45 +194,22 @@ const healthCheckVerboseController = async (req) => {
})
}

/**
* Controller for `health_check/fileupload` route *
* Perform a file upload health check limited to configured delegateOwnerWallet.
* This prunes the disc artifacts created by the process after.
*/
const healthCheckFileUploadController = async (req) => {
const err =
req.fileFilterError ||
req.fileSizeError ||
(await removeTrackFolder(req, req.fileDir))
if (err) {
return errorResponseServerError(err)
}
return successResponse({ success: true })
}

// Routes

router.get('/health_check', handleResponse(healthCheckController))
router.get('/health_check/sync', handleResponse(syncHealthCheckController))
router.get(
'/health_check/duration',
healthCheckVerifySignature,
ensureValidSPMiddleware,
handleResponse(healthCheckDurationController)
)
router.get(
'/health_check/duration/heartbeat',
healthCheckVerifySignature,
ensureValidSPMiddleware,
handleResponseWithHeartbeat(healthCheckDurationController)
)
router.get(
'/health_check/verbose',
handleResponse(healthCheckVerboseController)
)
router.post(
'/health_check/fileupload',
healthCheckVerifySignature,
handleTrackContentUpload,
handleResponseWithHeartbeat(healthCheckFileUploadController)
)

module.exports = router
23 changes: 22 additions & 1 deletion creator-node/src/routes/tracks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,27 @@ module.exports = function (app) {
})
)

/**
* Delete all temporary transcode artifacts from track transcode handoff flow.
* This is called on the node that was handed off the transcode to clear the state from disk
*/
app.post(
'/clear_transcode_and_segment_artifacts',

@dmanjunath dmanjunath Jul 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can only be called by a valid SP for a folder inside tmp_track_artifacts, which need to be cleared anyway. On primaries this folder is deleted right after the transcoded files are copied into their permanent locations.

Also this is a safe operation because removeTrackFolder calls lstat before doing any fs operations, so if the dir doesn't exist it throws an error

ensureValidSPMiddleware,
handleResponse(async (req, res) => {
const fileDir = req.body.fileDir
req.logger.info('Clearing filesystem fileDir', fileDir)
if (!fileDir.includes('tmp_track_artifacts')) {
return errorResponseBadRequest(
'Cannot remove track folder outside temporary track artifacts'
)
}
await removeTrackFolder({ logContext: req.logContext }, fileDir)

@vicky-g vicky-g Jul 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably wrap this in a try/catch. if this part fails, then we wouldnt be returning any errorResponse, and only an err is thrown

@dmanjunath dmanjunath Jul 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's what handleResponse does, it wraps this entire function in a try catch

@vicky-g vicky-g Jul 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but I think itd be nicer if we have a specific err thrown VS the general internal server error. not a huge deal but would give more context on the err

example:

  app.get(
    '/test_err',
    handleResponse(async (req, res) => {
      throw new Error('test throwing in handleResponse')
    })
  )

server:

// 20220701161107
// http://35.192.87.132:4000/test_err

{
  "error": "Internal server error"
}


return successResponse()
})
)

/**
* Given that the requester is a valid SP, the current Content Node has enough storage,
* upload the track to the current node and add a transcode and segmenting job to the queue.
Expand Down Expand Up @@ -127,7 +148,7 @@ module.exports = function (app) {
* Given that the request is coming from a valid SP, serve the corresponding file
* from the transcode handoff
*
* This route is used on an available SP when the primary requests the transcoded files after
* This route is called from the primary to request the transcoded files after
* sending the first request for the transcode handoff. This route does not run on the primary.
*/
app.get(
Expand Down