[Origo] Adds support to mark storage-node/distrubtion-node under maintenance #4793

zeeshanakram3 · 2023-06-21T06:07:13Z

Addresses #4765

Adds Storage/Distribution QN mappings
Adds operator:set-metadata & leader:set-node-operational-status CLI commands
~~WIP: Updating Storage/Distribution related integration tests~~ Improving Storage/Distribution related integration tests #4815

vercel · 2023-06-21T06:07:16Z

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 18, 2023 7:00am

…sus_operational_status_feat

mnaamani · 2023-07-16T23:50:56Z

You need to add implicitly new dependency on inquirer-datepicker? (and maybe others) to storage node: this is why this check is failing building docker image for storage node https://github.com/Joystream/joystream/actions/runs/5561050023/jobs/10158465896?pr=4793#step:8:373

zeeshanakram3 · 2023-07-26T09:41:26Z

Integration tests will be added in #4815

mnaamani · 2023-07-31T17:27:41Z

@Zeeshan perhaps we can try to bump version of protobufjs in this PR?

see GHSA-h755-8qp9-cq85

…sus_operational_status_feat

zeeshanakram3 · 2023-08-01T15:20:24Z

perhaps we can try to bump version of protobufjs in this PR?

Bumped package versions and updated CHANGELOG.md in d133ffb

mnaamani · 2023-08-02T10:36:20Z

perhaps we can try to bump version of protobufjs in this PR?

Bumped package versions and updated CHANGELOG.md in d133ffb

Great.
I think QN package also needs a version bump.

zeeshanakram3 · 2023-08-02T13:42:11Z

I think QN package also needs a version bump.

Done in ba24cfe

kdembler

Great work @zeeshanakram3 and sorry for doing this review so late 🙇 I think one additional feature we could introduce is for distribution nodes to only query active storage operators when doing pings/asset checks. But we can do that as a followup later, this in itself is quite valuable.
Edit: I see that issue for that is already created and scoped: #4816

kdembler · 2024-01-09T14:15:13Z

metadata-protobuf/proto/Storage.proto

+  }
+
+  // Node's Operational status to set
+  optional OperationalStatus status = 1; 


Shouldn't the status be required here? Full NodeOperationalStatusMetadata is already optional in DistributionBucketOperatorMetadata

@zeeshanakram3 still relevant

I would go further and suggest that we should probably have multiple messages, corresponding with the options in the commands for setting the metadata.

message NodeOperationalStatusNormal { optional string rationale = 1; required string bucketId = 2; }

message NodeOperationalStatusNoService { optional string rationale = 1; required string bucketId = 2; }

NodeOperationalStatusNoServiceFrom { optional string rationale = 1; required string from = 2; // date required string bucketId = 3; }

NodeOperationalStatusNoServiceUntil { optional string rationale = 1; optional string from = 2 // date required string until = 3; // date required string bucketId = 4; }

Doing this I think will help address a couple issues raised in other places in the review as well as simplifying implementation in the commands and squid mappings I presume.

kdembler · 2024-01-09T18:03:48Z

query-node/mappings/src/storage/index.ts

+  if ((await getWorker(store, workingGroup.name, workerId)) === undefined) {
+    return invalidMetadata(`The worker ${workerId} does not exist in the ${workingGroup.name} working group`)
+  }


I'm wondering, do we need to bother with worker for storage buckets? Those only have a single operator so I guess the worker ID doesn't make any difference?

kdembler · 2024-01-09T18:07:49Z

query-node/mappings/src/storage/metadata.ts

+      status.from = new Date(meta.noServiceFrom)
+      status.to = new Date(meta.noServiceTo)


Are those raw dates validated anywhere? If not, we probably should do that

Addressed this in storage squid PR Joystream/storage-squid#19

kdembler · 2024-01-09T18:10:24Z

query-node/schemas/storageEvents.graphql

+  ### SPECIFIC DATA ###
+
+  "Storage Bucket"
+  storageBucket: StorageBucket


Shouldn't that be a required field?

storage-node/src/commands/leader/set-node-operational-status.ts

storage-node/src/commands/operator/set-metadata.ts

kdembler · 2024-01-09T18:15:09Z

storage-node/src/commands/operator/set-metadata.ts

+      metadata = {
+        ...input,
+        operationalStatus: {
+          ...input.operationalStatus,
+          status:
+            input.operationalStatus?.status === 'Normal'
+              ? NodeOperationalStatusMetadata.OperationalStatus.NORMAL
+              : NodeOperationalStatusMetadata.OperationalStatus.NO_SERVICE,
+        },
+      }


I think if somebody will try to set metadata without operational status then this will set it to NO_SERVICE?

Addressed in 6ff4531

kdembler · 2024-01-09T18:16:12Z

distributor-node/src/commands/operator/set-metadata.ts

+      metadata = {
+        ...params,
+        operationalStatus: {
+          ...params.operationalStatus,
+          status:
+            params.operationalStatus?.status === 'Normal'
+              ? NodeOperationalStatusMetadata.OperationalStatus.NORMAL
+              : NodeOperationalStatusMetadata.OperationalStatus.NO_SERVICE,
+        },


I think if somebody will try to set metadata without operational status then this will set it to NO_SERVICE?

Addressed in 6ff4531

kdembler · 2024-01-09T18:24:32Z

storage-node/src/services/queryNode/schema.graphql

@@ -957,6 +962,9 @@ type DistributionBucketOperatorMetadata implements BaseGraphQLObject {
  nodeLocation: NodeLocationMetadata
  nodeLocationId: String

+  """Optional node operational status"""
+  nodeOperationalStatus: NodeOperationalStatus


I think this can be hard to query because of QN nested filtering limitations. I think we need to design a schema in a way that I can easily fetch all the active operators. If I'm not mistaken with the current schema the query would have to be (paraphrasing, I don't remember the query syntax for checking specific union type):

query { distributionBucketOperators( where: { metadata: { nodeOperationalStatus: { status: "OKAY" } } } ) { id } }

And that filter would be too deep for QN to execute. Same applies to storage buckets

Also, would it be possible to make operational status required part of metadata? Because empty operational status currently would also mean "Okay" but it would be hard to query it I think

I think you would not have that problem with storage squid's graphql server

…sus_operational_status_feat

…ve storage nodes

zeeshanakram3 · 2024-02-29T06:42:27Z

@kdembler I have addressed the requested changes. Also, removed the QN mappings needed for this feature as I think having the code at two places makes it difficult to review and maintain

Also, did changes as described in #4816 for both Argus/Colossus

kdembler · 2024-03-11T14:01:39Z

distributor-node/src/commands/leader/set-node-operational-status.ts

+    const leadKey = await this.getDistributorLeadKey()
+
+    let operationalStatus: INodeOperationalStatusMetadata = {}
+    switch (statusType) {


statusType input is not required, should we default to Normal?

It makes more sense to make it required, the command name doesn't imply what the default value should be.

It makes more sense to make it required, the command name doesn't imply what the default value should be.

Done

kdembler · 2024-03-11T14:02:08Z

distributor-node/src/commands/operator/set-metadata.ts

    const workerKey = await this.getDistributorWorkerRoleKey(workerId)

+    let operationalStatus: INodeOperationalStatusMetadata = {}
+    switch (statusType) {


Same question about default status here

kdembler · 2024-03-11T14:06:21Z

distributor-node/src/commands/operator/set-metadata.ts

+                  ? NodeOperationalStatusMetadata.OperationalStatus.NORMAL
+                  : NodeOperationalStatusMetadata.OperationalStatus.NO_SERVICE,
+            }
+          : {},


Would this override the current status? Or skip the update?

It should skip the update but previously it was overriding, I have fixed it

kdembler · 2024-03-11T14:07:42Z

distributor-node/src/services/networking/query-node/queries/queries.graphql

+      operatorMetadata: {
+        OR: [
+          { nodeOperationalStatus_isNull: true }
+          { nodeOperationalStatus: { isTypeOf_eq: "NodeOperationalStatusNormal" } }


This filter would skip any nodes with future maintenance as well. We probably should get all the operators and later only filter those that are under maintenance currently

Done in 0537ee7

kdembler · 2024-03-11T14:10:15Z

metadata-protobuf/proto/Storage.proto

+  }
+
+  // Node's Operational status to set
+  optional OperationalStatus status = 1; 


@zeeshanakram3 still relevant

kdembler · 2024-03-11T14:11:35Z

storage-node/src/services/queryNode/queries/queries.graphql

+      operatorMetadata: {
+        OR: [
+          { nodeOperationalStatus_isNull: true }
+          { nodeOperationalStatus: { isTypeOf_eq: "NodeOperationalStatusNormal" } }
+        ]
+      }


Same comment here as with the query in distribution node

mnaamani

I'm suggesting to make multiple messages rather than a single one. Feels like a better design, but this will require a bit of refactoring here and in the storage-squid mappings.

mnaamani · 2024-03-20T11:03:48Z

distributor-node/src/commands/leader/set-node-operational-status.ts

+    const leadKey = await this.getDistributorLeadKey()
+
+    let operationalStatus: INodeOperationalStatusMetadata = {}
+    switch (statusType) {


It makes more sense to make it required, the command name doesn't imply what the default value should be.

mnaamani · 2024-03-21T06:16:19Z

metadata-protobuf/proto/Storage.proto

+
+message SetNodeOperationalStatus {
+  optional NodeOperationalStatusMetadata operational_status = 1; // Node's operational status to set
+  optional string worker_id = 2; // Storage/Distribution Worker ID


I would argue only the bucket_id is strictly necessary in the message.

Correct for storage nodes, but for distributor nodes we also need to know about the worker_id, since a bucket can be assigned to multiple operators?

mnaamani · 2024-03-21T06:18:08Z

distributor-node/src/commands/leader/set-node-operational-status.ts

+    bucketId: flags.bucketId({
+      required: true,
+    }),
+    workerId: flags.integer({


I suggested that worker id is not needed in metadata. If we agree on that, then this workerId can be dropped.

Please see #4793 (comment)

mnaamani · 2024-03-21T07:07:47Z

metadata-protobuf/proto/Storage.proto

+  }
+
+  // Node's Operational status to set
+  optional OperationalStatus status = 1; 


I would go further and suggest that we should probably have multiple messages, corresponding with the options in the commands for setting the metadata.

message NodeOperationalStatusNormal { optional string rationale = 1; required string bucketId = 2; }

message NodeOperationalStatusNoService { optional string rationale = 1; required string bucketId = 2; }

NodeOperationalStatusNoServiceFrom { optional string rationale = 1; required string from = 2; // date required string bucketId = 3; }

NodeOperationalStatusNoServiceUntil { optional string rationale = 1; optional string from = 2 // date required string until = 3; // date required string bucketId = 4; }

Doing this I think will help address a couple issues raised in other places in the review as well as simplifying implementation in the commands and squid mappings I presume.

mnaamani · 2024-03-21T08:57:46Z

distributor-node/src/command-base/default.ts

+        type: 'datepicker',
+        name: 'result',
+        clearable: true,
+        default: new Date('2017-09-28 17:36:05').toISOString(),


Tried the date picker, works really nicely in the terminal!

Perhaps a more reasonable default would be the current date new Date().toISOString(), so less fiddling with inital year and month for the user.

…sus_operational_status_feat

zeeshanakram3 · 2024-03-27T07:12:08Z

@mnaamani @kdembler I have addressed the requested changes. Also, re-designed the protobuf message schemas as requested.

If the new protobuf messages are acceptable, then please publish the npm package so I would update the storage-squid mappings as well

mnaamani · 2024-04-08T21:19:26Z

@mnaamani @kdembler I have addressed the requested changes. Also, re-designed the protobuf message schemas as requested.

If the new protobuf messages are acceptable, then please publish the npm package so I would update the storage-squid mappings as well

published protobuf and joystream/js packages (for joystream/js I bumped the dependency on protobuf package)

zeeshanakram3 added 6 commits June 21, 2023 10:07

add protobuf schemas for operational status feat

8a10f93

add QN schemas for operational status feat

bb0b5f5

add QN mappings for operational status feat.

407cdeb

add CLI commands to set operational status in Argus

ca8a495

add CLI commands to set operational status in Colossus

e53f696

argus: generate-docs

21bc807

zeeshanakram3 added colossus argus Argus distributor node origo Origo Release labels Jun 23, 2023

Merge remote-tracking branch 'upstream/master' into origo_argus_colos…

a27f656

…sus_operational_status_feat

add inquirer dependency in storage-node package.json

33997c4

mnaamani self-requested a review July 28, 2023 00:00

Merge remote-tracking branch 'upstream/master' into origo_argus_colos…

b07bbec

…sus_operational_status_feat

zeeshanakram3 force-pushed the origo_argus_colossus_operational_status_feat branch from 2063272 to b07bbec Compare August 1, 2023 14:51

bump package versions & update changelog

d133ffb

zeeshanakram3 force-pushed the origo_argus_colossus_operational_status_feat branch from 5cf5bd6 to d133ffb Compare August 2, 2023 13:40

bump QN package version

ba24cfe

kdembler self-assigned this Dec 21, 2023

kdembler requested changes Jan 9, 2024

View reviewed changes

zeeshanakram3 added 4 commits February 26, 2024 21:05

Merge remote-tracking branch 'upstream/master' into origo_argus_colos…

4860658

…sus_operational_status_feat

address requested changes

6ff4531

update generated storage-squid schema

fc04793

[Argus] ensure storage node only syncs from operationally active nodes

b95ae04

zeeshanakram3 added 3 commits February 29, 2024 11:28

update colossus docs

4dd076f

[Argus] ensure distributor node only connects with operationally acti…

b6329f7

…ve storage nodes

revert QN changes (i.e. remove mappings and schema changes)

172bcb7

zeeshanakram3 requested a review from kdembler February 29, 2024 06:42

update graphql queries

16aaa48

kdembler requested changes Mar 11, 2024

View reviewed changes

mnaamani requested changes Mar 21, 2024

View reviewed changes

zeeshanakram3 added 3 commits March 26, 2024 00:05

Merge remote-tracking branch 'upstream/master' into origo_argus_colos…

73dc634

…sus_operational_status_feat

address requested changes

971aaae

fix: only filter currently under maintenance nodes

0537ee7

zeeshanakram3 requested review from mnaamani and kdembler March 27, 2024 07:12

kdembler mentioned this pull request Mar 27, 2024

add schema entities and mappings to process set node operational status protobuf message Joystream/storage-squid#19

Open

		status.from = new Date(meta.noServiceFrom)
		status.to = new Date(meta.noServiceTo)

[Origo] Adds support to mark storage-node/distrubtion-node under maintenance #4793

Are you sure you want to change the base?

[Origo] Adds support to mark storage-node/distrubtion-node under maintenance #4793

Conversation

zeeshanakram3 commented Jun 21, 2023 • edited by mnaamani

vercel bot commented Jun 21, 2023 • edited

mnaamani commented Jul 16, 2023 • edited

zeeshanakram3 commented Jul 26, 2023

mnaamani commented Jul 31, 2023

zeeshanakram3 commented Aug 1, 2023

mnaamani commented Aug 2, 2023

zeeshanakram3 commented Aug 2, 2023

kdembler left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaamani Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeeshanakram3 Feb 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeeshanakram3 commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaamani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaamani Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeeshanakram3 commented Mar 27, 2024

mnaamani commented Apr 8, 2024

zeeshanakram3 commented Jun 21, 2023 •

edited by mnaamani

vercel bot commented Jun 21, 2023 •

edited

mnaamani commented Jul 16, 2023 •

edited

kdembler left a comment •

edited

mnaamani Mar 21, 2024 •

edited

zeeshanakram3 Feb 29, 2024 •

edited

mnaamani Mar 21, 2024 •

edited