fix: mount metadata in wakucanary #2793

darshankabariya · 2024-06-09T14:38:33Z

Add cluster ID and shards in the Wakucanary configuration, and then mount this metadata information to Wakunode.

github-actions · 2024-06-09T14:42:58Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2793-rln-v2

Built from 70f3696

github-actions · 2024-06-09T14:43:16Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2793-rln-v1

Built from 70f3696

github-actions · 2024-06-11T13:39:33Z

This PR may contain changes to configuration options of one of the apps.

If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed.

Please also make sure the label release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

gabrielmer

Ooh thanks so much! added a small comment :))

gabrielmer · 2024-06-12T07:54:57Z

waku/node/waku_node.nim

+proc mountMetadata*(
+    node: WakuNode, clusterId: uint32, shards: seq[uint16] = @[]
+): Result[void, string] =
  if not node.wakuMetadata.isNil():
    return err("Waku metadata already mounted, skipping")

  let metadata = WakuMetadata.new(clusterId, node.enr, node.topicSubscriptionQueue)
-
+  for shard in shards:
+    metadata.shards.incl(uint32(shard))


I think this change is not necessary and might be problematic.
The shards are added to metadata in WakuMetadata.newby using the parameter node.enr

If the node used in the Canary is subscribed to the right shards, then they should be in the ENR and added from there

Thanks Gabriel !

Then i should try to add this shards information in ENR.

Ivansete-status

It looks well so far! Good work 🙌
Nevertheless, there are a couple of points to be considered. See my comments:)
Also, kindly revert the change around vendor/nim-bearssl. We shouldn't change that in this PR

Ivansete-status · 2024-06-12T07:52:03Z

apps/wakucanary/README.md

     --websocket-secure-key-path  Secure websocket key path:   '/path/to/key.txt' .
     --websocket-secure-cert-path  Secure websocket Certificate path:   '/path/to/cert.txt' .
-
+ -c, --cluster-id     Cluster ID of the fleet node to check status [Default=1].


In the README.md I think is missing explaining about the shards parameter and give an example on how to use it and why

Ivansete-status · 2024-06-12T07:56:13Z

waku/node/waku_node.nim

+  for shard in shards:
+    metadata.shards.incl(uint32(shard))


That seems correct from my PoV but let's ask SP about it:

@SionoiS - Do you think there is a more idiomatic way to achieve that?

Yes? I find it weird to add this to waku_node.nim when this PR is about adapting the Canary to it. I would expect that if this line would be missing, then we would have some bug open about it

Yes? I find it weird to add this to waku_node.nim when this PR is about adapting the Canary to it. I would expect that if this line would be missing, then we would have some bug open about it

I think that @darshankabariya's suggestion sounds very reasonable and looks more explicit. Also, it seems correct too because we are already passing the clusterId as a parameter in mountMetadata.

Nevertheless, you and SP might have more context than me about that and there might be a more idiomatic way to achieve that, i.e. the way you suggested (which I didn't see while I was adding my first comment .)

The less we add to wakunode the better IMO. In this case, you can have WakuMetadata add shard by subscribing to them.

Thanks for the feedback @SionoiS, @gabrielmer, and @Ivansete-status. I'll update the implementation to add shard information via ENR.

gabrielmer

Looks amazing! Thanks so much! 🚀

Ivansete-status

LGTM! Thanks for it! 💯
Beautiful changes and great work! Congrats 🥳
I'm just adding a small suggestion to have the code more aligned with the style that we use to have (use valueOr and isOkOr.)

Ivansete-status · 2024-06-14T08:15:12Z

apps/wakucanary/wakucanary.nim

+  let relayShards = RelayShards.init(conf.clusterId, conf.shards)
+  if relayShards.isok():
+    enrBuilder.withWakuRelaySharding(relayShards.get()).isOkOr:
+      error "Building ENR with relay sharding failed", error = error
+  else:
+    error "Relay shards initialization failed", error = relayShards.error


Just a nitpick comment where we use valueOr and, in case of error, we stop the wakucanary execution.

Notice how valueOr is very similar to isOkOr, in concept, but in this case, it allows to return an item.

Suggested change

let relayShards = RelayShards.init(conf.clusterId, conf.shards)

if relayShards.isok():

enrBuilder.withWakuRelaySharding(relayShards.get()).isOkOr:

error "Building ENR with relay sharding failed", error = error

else:

error "Relay shards initialization failed", error = relayShards.error

let relayShards = RelayShards.init(conf.clusterId, conf.shards).valueOr:

error "Relay shards initialization failed", error = error

return 1

enrBuilder.withWakuRelaySharding(relayShards.get()).isOkOr:

error "Building ENR with relay sharding failed", error = error

return 1

github-actions · 2024-06-14T09:47:48Z

This PR may contain changes to database schema of one of the drivers.

If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues.

Please make sure the label release-notes is added to make sure upgrade instructions properly highlight this change.

* chore: integrate cluster id and shards to waku node.

github-actions bot assigned darshankabariya Jun 9, 2024

darshankabariya requested a review from Ivansete-status June 12, 2024 05:36

darshankabariya force-pushed the Bug_2720 branch from a9bbe91 to f14b908 Compare June 12, 2024 06:12

darshankabariya requested review from gabrielmer and NagyZoltanPeter June 12, 2024 06:13

gabrielmer reviewed Jun 12, 2024

View reviewed changes

Ivansete-status reviewed Jun 12, 2024

View reviewed changes

darshankabariya force-pushed the Bug_2720 branch from 7b31e0c to f14b908 Compare June 12, 2024 09:49

darshankabariya closed this Jun 12, 2024

darshankabariya force-pushed the Bug_2720 branch from f14b908 to dcff7e9 Compare June 12, 2024 09:59

darshankabariya reopened this Jun 12, 2024

darshankabariya requested review from SionoiS, Ivansete-status and gabrielmer June 14, 2024 05:52

gabrielmer approved these changes Jun 14, 2024

View reviewed changes

Ivansete-status approved these changes Jun 14, 2024

View reviewed changes

darshankabariya force-pushed the Bug_2720 branch from b7bca0c to 942ffa9 Compare June 14, 2024 09:03

darshankabariya closed this Jun 14, 2024

darshankabariya force-pushed the Bug_2720 branch from 4e3d790 to ab4b5b2 Compare June 14, 2024 09:54

chore: integrate cluster id and shards to wakunode

c6b5d6e

darshankabariya reopened this Jun 14, 2024

chore: update READ me

f4a66b3

darshankabariya merged commit 3b27aee into master Jun 14, 2024
13 of 15 checks passed

darshankabariya deleted the Bug_2720 branch June 14, 2024 12:59

rymnc pushed a commit that referenced this pull request Jun 20, 2024

fix: mount metadata in wakucanary (#2793)

c1dcf7f

* chore: integrate cluster id and shards to waku node.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: mount metadata in wakucanary #2793

fix: mount metadata in wakucanary #2793

darshankabariya commented Jun 9, 2024 •

edited

Loading

github-actions bot commented Jun 9, 2024 •

edited

Loading

github-actions bot commented Jun 9, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024

gabrielmer left a comment

gabrielmer Jun 12, 2024

darshankabariya Jun 12, 2024

Ivansete-status left a comment

Ivansete-status Jun 12, 2024

Ivansete-status Jun 12, 2024

gabrielmer Jun 12, 2024

Ivansete-status Jun 12, 2024 •

edited by darshankabariya

Loading

SionoiS Jun 12, 2024

darshankabariya Jun 12, 2024

gabrielmer left a comment

Ivansete-status left a comment

Ivansete-status Jun 14, 2024

github-actions bot commented Jun 14, 2024

fix: mount metadata in wakucanary #2793

fix: mount metadata in wakucanary #2793

Conversation

darshankabariya commented Jun 9, 2024 • edited Loading

github-actions bot commented Jun 9, 2024 • edited Loading

github-actions bot commented Jun 9, 2024 • edited Loading

github-actions bot commented Jun 11, 2024

gabrielmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status 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

Ivansete-status Jun 12, 2024 • edited by darshankabariya Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmer left a comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 14, 2024

darshankabariya commented Jun 9, 2024 •

edited

Loading

github-actions bot commented Jun 9, 2024 •

edited

Loading

github-actions bot commented Jun 9, 2024 •

edited

Loading

Ivansete-status Jun 12, 2024 •

edited by darshankabariya

Loading