Colossus 3.6.0 by mnaamani · Pull Request #4796 · Joystream/joystream

mnaamani · 2023-06-26T17:52:18Z

Added support for multiple transactor accounts when running the server, so we can take advantage of ability for operators to assign different transactor accounts per bucket.
Added support specifying which buckets to serve when running a server.
Bump version to v3.6.0

In combination these two changes now allows workers who operate multiple buckets to be able to run multiple nodes, for example with each node serving a single bucket.

Also finally added behavior to allow server to startup even if transactor account is not provided to one or more buckets. The server will still serve downloads and sync those buckets. This is very useful for testing. I can run a node and specify a valid worker id and test the nodes behavior, open telemetry traces etc.

yarn storage-node server -d ~/tmp/ -o 3333 --apiUrl wss://rpc.joystream.org \
    -s -r 1 -w 8 --buckets 0,1,2,3,4,5,6,7

2023-07-16 03:07:35:735 info: Initializing runtime connection to: wss://rpc.joystream.org
2023-07-16 03:07:38:738 info: Waiting for chain to be synced before proceeding.
2023-07-16 03:07:38:738 info: Query node endpoint set: http://localhost:8081/graphql
2023-07-16 03:07:38:738 debug: Query - storageBucketsConnection
 ›   Warning: Missing transactor key for bucket 1
2023-07-16 03:07:39:739 warn: Only subset of buckets will process uploads!
2023-07-16 03:07:39:739 info: Buckets synced and served: 1
2023-07-16 03:07:39:739 info: Buckets accepting uploads: 
2023-07-16 03:07:39:739 info: Removing temp directory ...
2023-07-16 03:07:39:739 info: Creating temp directory ...
2023-07-16 03:07:39:739 debug: Local ID cache loaded.
2023-07-16 03:07:39:739 info: Synchronization enabled.
2023-07-16 03:07:39:739 debug: Max file size runtime parameter: 64424509440
2023-07-16 03:07:39:739 info: Listening on http://localhost:3333
2023-07-16 03:07:39:739 info: Sync paused for 1 minute(s).

Even an invalid worker id can be specified, in which-case the server will startup, but with very limited functionality. This is useful for preparing a new node, testing endpoint accessibility, and general testing. We should add a separate command for this purpose.

…s are provided for operation

vercel · 2023-06-26T17:52:21Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated (UTC)
pioneer-testnet	⬜️ Ignored (Inspect)		Jul 20, 2023 10:21pm

…all buckets. Uploading to bucket without transactor key will fail with a 503 status code.

zeeshanakram3

Looks Good. I think just some cosmetic changes are required.

I tested this seems to be working as expected. The overall code refactorization and customization were long overdue, so great job.

zeeshanakram3 · 2023-07-18T04:45:37Z

storage-node/src/services/sync/storageObligations.ts

 export async function getStorageObligationsFromRuntime(
  queryNodeUrl: string,
-  workerId: number
+  workerId: number,


workerId param is not used anymore

zeeshanakram3 · 2023-07-18T05:15:01Z

storage-node/src/commands/server.ts

+
+    if (!(await verifyWorkerId(api, workerId))) {
+      logger.warn(`workerId ${workerId} does not exist in the storage working group`)
+      // this.exit(ExitCodes.InvalidWorkerId)


Kindly remove this commented line, since we have decided to bootstrap storage-node even if an invalid workerId is specified

On second thoughts, decided to revert allowing invalid workerId: https://github.com/Joystream/joystream/pull/4796/files#r1270006202

On second thoughts, decided to revert allowing invalid workerId: https://github.com/Joystream/joystream/pull/4796/files#r1270006202

Sounds good

zeeshanakram3 · 2023-07-18T06:39:23Z

storage-node/src/commands/server.ts

      )
    )
  )
+    .filter(([bucketId]) => bucketsToServe.length === 0 || bucketsToServe.includes(parseInt(bucketId)))


What should be the behavior if the workerId input is invalid & list of buckets that needed to be served is valid, should the storage-node serve those buckets? The current implementation does not allow for that.

I think workerId & buckets flags can be mutually exclusive as we can derive the worker info if the buckets flag is specified i.e. if buckets is provided then we can be assured that worker ID is the worker that is assigned to serve these buckets in the runtime. WDYT?

That's an excellent point I didn't consider that because allowing invalid worker id was a last minute change i didn't without thinking it through, also I just realized that a storage bucket can only have one operator (unlike distribution buckets).

if buckets is provided then we can be assured that worker ID is the worker that is assigned to serve these buckets in the runtime. WDYT?

But in this case you can pass buckets for different workerId.. should we allow that? (It would only make sense if both 'workers' are the same person or are cooperating (sharing transactor-key and maybe role key))

I'm tempted to completely drop the workerId flag and only have the buckets flag so you always have to be explicit as to what bucket to serve.

I don't want to make this overly complicated and prone to making mistakes, just to allow a more testable node. I think its better that I revert this change so you must provide a valid worker id, and we can separately make have another command just for testing purpouses which would allow specifying any bucket.

zeeshanakram3 · 2023-07-18T08:40:18Z

storage-node/src/command-base/ApiCommandBase.ts

+  async loadKeys(flags: {
+    dev: boolean
+    keyFile?: string
+    password?: string | string[]
+    accountUri?: string | string[]
+    keyStore?: string
+  }): Promise<void> {
+    const keyring = this.getKeyring()
+    const { dev, password, keyFile, accountUri, keyStore } = flags


Just to comfirm, what is the desired behavior here? Should these flags (keyFile, accountUri, keyStore) be allowed to be simultaneously used or they should be exclusive? Currently, they can be simultaneously used, if desired otherwise, these should be marked as exclusive

No they are not exclusive, all can be used at the same time.

zeeshanakram3 · 2023-07-18T09:24:54Z

storage-node/src/services/webApi/app.ts

  const dataObjectId = new BN(req.query.dataObjectId?.toString() || '')
  const bagId = req.query.bagId?.toString() || ''

+  if (!res.locals.uploadBuckets.includes(req.query.storageBucketId?.toString() || '')) {


I guess storageBucketId defined above can be used here instead of duplicating the expression req.query.storageBucketId?.toString() || ''?

zeeshanakram3 · 2023-07-18T10:44:49Z

storage-node/src/commands/dev/sync.ts

  async run(): Promise<void> {
    const { flags } = this.parse(DevSync)
-
+    const bucketId = '' + flags.bucketId


Suggested change

const bucketId = '' + flags.bucketId

const bucketId = flags.bucketId.toString()

I think better to use .toString() method?

zeeshanakram3

LGTM.

mnaamani added 2 commits June 26, 2023 11:34

colossus: support multiple transactor accounts and verify correct key…

d31c0e5

…s are provided for operation

colossus: bucketIds can be specified when running server

6a8c989

mnaamani added 3 commits June 26, 2023 21:27

colossus: if keypair cannot be unlocked, exit instead of just warning.

f6a182b

integration-tests: fix renamed command arg for storage-node

2f124aa

colossus: better handling of failure to unlock key files

9d23bf2

mnaamani requested a review from zeeshanakram3 June 27, 2023 08:37

mnaamani added 2 commits July 7, 2023 00:51

Merge branch 'master' into colossus-3.5.0

006ba24

Merge branch 'master' into colossus-3.5.0

a89e379

mnaamani changed the title ~~Colossus 3.5.0~~ Colossus 3.6.0 Jul 14, 2023

mnaamani added 4 commits July 15, 2023 01:26

colossus: update changelog

7d80430

colossus: allow server to run even if no transactor key provided for …

b87ee64

…all buckets. Uploading to bucket without transactor key will fail with a 503 status code.

colossus: allow startup with any worker id

e1d2834

Merge branch 'master' into colossus-3.5.0

0a5fb35

zeeshanakram3 reviewed Jul 18, 2023

View reviewed changes

colossus: code cleanup and revert allowing invalid workerId on startup

fd67725

mnaamani requested a review from zeeshanakram3 July 20, 2023 22:29

mnaamani added 2 commits July 22, 2023 08:03

Merge branch 'master' into colossus-3.5.0

91a6224

Merge branch 'master' into colossus-3.5.0

17eeb79

zeeshanakram3 approved these changes Jul 24, 2023

View reviewed changes

zeeshanakram3 mentioned this pull request Jul 26, 2023

[Colossus] CLI command for updating multiple bags #4812

Merged

mnaamani merged commit 8458489 into Joystream:master Jul 26, 2023

	const bucketId = '' + flags.bucketId
	const bucketId = flags.bucketId.toString()

Conversation

mnaamani commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeeshanakram3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaamani Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeeshanakram3 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mnaamani commented Jun 26, 2023 •

edited

Loading

vercel bot commented Jun 26, 2023 •

edited

Loading

mnaamani Jul 20, 2023 •

edited

Loading