Skip to content
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

Added interface for connecting parachain nodes to relaychain nodes. #35

Merged
merged 14 commits into from
Jan 17, 2024

Conversation

Maharacha
Copy link
Contributor

Became a bit harder for several reasons:

  1. Found out that multiple relay nodes can be configured which will then work automatically as fallback. Example: --relay-chain-rpc-urls ws://10.207.153.56:9933 ws://10.207.153.189:9933
    And of course the interface should support that.

  2. Seems to be a bug in Juju where the relation data is not available in the departed hook. A problem since using multiple relay nodes we need to know which one to remove. I have reported this in the Matrix chat. I made a work around storing some extra info in _stored_.

@@ -70,11 +72,12 @@ def __init__(self, *args):

self._stored.set_default(binary_url=self.config.get('binary-url'),
docker_tag=self.config.get('docker-tag'),
service_args=self.config.get('service-args'))
service_args=self.config.get('service-args'),
relay_rpc_urls=dict())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the data bug in departed hook we need a dict to for each url also store information which unit it is.

@@ -8,8 +8,9 @@

class ServiceArgs():

def __init__(self, service_args: str):
def __init__(self, service_args: str, relay_rpc_urls: dict):
Copy link
Contributor Author

@Maharacha Maharacha Jan 11, 2024

Choose a reason for hiding this comment

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

I'm not very happy with this solution. But could not come up with something more beautiful but still easy. We might probably need to do some surgency to this class anyway when we bring in snap support.

src/charm.py Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
@Maharacha Maharacha requested review from erik78se, jonathanudd and jakobilobi and removed request for erik78se January 11, 2024 16:31
@erik78se
Copy link
Contributor

Is there a link to the bug in the data passed in the interfaces?

@Maharacha
Copy link
Contributor Author

Is there a link to the bug in the data passed in the interfaces?

It's only in the Element chat so far. One guy there was able to reproduce this behavior. Now waiting for the Juju gurus to wake up.

@Maharacha
Copy link
Contributor Author

Another way of doing this would be to use the peer-relation. So that only the leader unit communicates with the interface and updates the app-data that the other units can read. It's a good use case to practice on.

@Maharacha
Copy link
Contributor Author

@jonathanudd good news. You can mix the flags however you want. You can have multiple --relay-chain-rpc-urls flags in the service args and it will use all of the URLs. Which means that the interface can work together with manually added URLs. Worth knowing is that the URLs from the interface is added to the beginning of the service args so they will be used before any manually added URLs are used. As long as they are not manually added in front of the interface URLs but that is up to the operator to understand I would say.

Copy link
Collaborator

@jonathanudd jonathanudd left a comment

Choose a reason for hiding this comment

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

Nice 👍

src/interface_rpc_url_requirer.py Outdated Show resolved Hide resolved
Comment on lines 34 to 36
except KeyError:
event.defer()
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

What feedback from juju do you get when adding the relation if this happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. Can try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some logging messages if it receives or can not receive an URL. Not sure how we can make the feedback better. I mean receiving the URL is not a guarantee that the connection actually works. Might be other stuff that makes the Parachain node not be able to use the Relaychain node. So then the question is should we make it clear for the operator that it is not able to use one of the Relaynodes in the list?

Co-authored-by: Jonathan Udd <[email protected]>
@jakobilobi
Copy link
Collaborator

Found out that multiple relay nodes can be configured which will then work automatically as fallback.

I guess we should have suspected this since the setting is --relay-chain-rpc-urls - e.g. in plural! Nice find though 🙆‍♂️

src/charm.py Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/service_args.py Show resolved Hide resolved
@Maharacha
Copy link
Contributor Author

Is there a link to the bug in the data passed in the interfaces?

There is now canonical/operator#1109
I have added it to the comment in the code.

Copy link
Collaborator

@jakobilobi jakobilobi left a comment

Choose a reason for hiding this comment

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

Looking good! Some small comments/questions but otherwise ready for merge 🚀

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
metadata.yaml Show resolved Hide resolved
src/interface_rpc_url_provider.py Outdated Show resolved Hide resolved
src/interface_rpc_url_requirer.py Outdated Show resolved Hide resolved
@Maharacha Maharacha merged commit 0b65202 into dwellir-public:develop Jan 17, 2024
@Maharacha Maharacha deleted the rpc-interface branch January 17, 2024 09:05
@jakobilobi jakobilobi mentioned this pull request Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants