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

[MM-62064] Leave nodeID empty when using RTCD service #912

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

streamer45
Copy link
Collaborator

Summary

This is slightly embarrassing. I introduced a regression in #908 because I cannot read my own code. The second statement in that PR (that we'd be starting up the embedded RTC server regardless of whether RTCD is used) is wrong as there was a return statement in the block so that change wasn't required at all.

And while making that change, I forgot the importance of setting p.nodeID , which should only be done when RTCD is not in use.

Unfortunately, e2e tests couldn't spot this since they run in a non-HA (created https://mattermost.atlassian.net/browse/MM-62065). I ran some basic manual test in HA myself but didn't test multiple users joining a call.

In practice, this means we'll need to prepare a couple more dot releases (1.3.2 and 0.29.6) to remedy the previous mistake, along with backports to respective MM versions.

@DHaussermann For QA, I'd like to ensure we can connect at least a couple of people to the same call in a HA installation, and ensure they can communicate both ways. Repeating this a couple of times may be beneficial since it's always possible they'd connect to the same HA node which is a case not affected by the issue we are fixing here.

Ticket Link

https://mattermost.atlassian.net/browse/MM-62064

@streamer45 streamer45 added 2: Dev Review Requires review by a core committer 2: QA Review Requires review by a QA tester labels Dec 3, 2024
@streamer45 streamer45 added this to the v1.4.0 / MM v10.4 milestone Dec 3, 2024
@streamer45 streamer45 self-assigned this Dec 3, 2024
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Hmm, interesting. I didn't catch that either. Thank you for the comments, that definitely helps.

@cpoile cpoile removed the 2: Dev Review Requires review by a core committer label Dec 3, 2024
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

I have a local HA setup in place (thanks to Agniva) where I can explicitly send users to a specific cluster instance.

  • I joined a few calls from 4 different browser sessions. 2 sessions where on each instance.
  • No issues using calls across the 4 sessions
  • By selectively adding and removing users I was able to see that the audio is making it back and forth across the 2 instances.
  • Tested with and without RTCD
  • Repeated the testing on a /cloud HA server as well as a precaution. No guarantee who is on which instance but I had 4 unique sessions.

LGTM!

@DHaussermann DHaussermann added 3: Reviews Complete All reviewers have approved the pull request and removed 2: QA Review Requires review by a QA tester labels Dec 4, 2024
@streamer45 streamer45 merged commit 4ae46fb into main Dec 4, 2024
18 checks passed
@streamer45 streamer45 deleted the MM-62064 branch December 4, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants