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

Introduce useLoopbackInterface to config #236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

karolyi
Copy link

@karolyi karolyi commented Aug 22, 2021

In hopes of tackling jitsi/jitsi-videobridge#1709, this is a patch that introduces a couple new config variables. Not sure this would be the right way though, as it's kinda superfluous in hindsight, having org.ice4j.ice.harvest.ALLOWED_INTERFACES and org.ice4j.ice.harvest.BLOCKED_INTERFACES in place. Nonetheless, here's the patch.

If you decide this to be the right way, the doc/configuration.md file needs to be updated.

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@bgrozev
Copy link
Member

bgrozev commented Aug 24, 2021

Thanks for the contribution! Looks good to me with a couple of small changes:

  1. Remove the org.ice4j.ice.harvest.USE_LOOPBACK_INTERFACE option (we don't want to add more options to the legacy file)
  2. Add a default value (false) for ice4j.harvest.use-loopback-interface in reference.conf with a short explanation of what the option does.
  3. Nit: rename to use-loopback-interfaces (plural)

You'll also need to sign the CLA as Jenkins pointed out.

@karolyi
Copy link
Author

karolyi commented Aug 25, 2021

Hey,

can you tell me how this ice4j project gets bundled into jitsi-videobridge, and how reference.conf (or any similar configuration) gets picked up by ice4j?

I see no reference.conf in the freebsd package (much less in the jitsi-videobridge.jar file where ice4j is packed into. Hence, I'm quite unsure how I can get this working on FreeBSD eventually.

@karolyi
Copy link
Author

karolyi commented Aug 25, 2021

Also because of this, I'd rather leave the 'old' configuration option in place, since FreeBSD might take a while to catch up with the new configuration file, should that be the issue.

I can contact the maintainer of the FreeBSD package, but I can't expect to have a timely reply from him to change the config file to the reference.conf format. A lot of deployments use the old format already.

@karolyi
Copy link
Author

karolyi commented Aug 25, 2021

Alright, I've made the discussed changes.

As for the CLA, I don't want to sign anything. However, I hereby declare this contribution under a WTFPL license. Sorry but I'm just not the kind of guy who signs random CLAs.

@karolyi
Copy link
Author

karolyi commented Aug 31, 2021

a humble bump

@bgrozev
Copy link
Member

bgrozev commented Aug 31, 2021

LGTM, but we can't merge it without a CLA.

@karolyi
Copy link
Author

karolyi commented Aug 31, 2021

You can close this request then and submit the patch under your name, I really don't mind.

@bgrozev
Copy link
Member

bgrozev commented Aug 31, 2021

reference.conf contains the defaults and is included in the ice4j.jar file. Applications use a different file with configuration overrides. For example, jitsi-videobridge ships with overriden default ice4j configuration in application.conf (this is not meant for users of jvb to change). On debian jvb also ships with /etc/jitsi/videobridge/jvb.conf for configuration overrides provided by the user. This file is loaded by adding -Dconfig.file=/etc/jitsi/videobridge/jvb.conf to the java command line.

@karolyi
Copy link
Author

karolyi commented Aug 31, 2021

reference.conf contains the defaults and is included in the ice4j.jar file. Applications use a different file with configuration overrides. For example, jitsi-videobridge ships with overriden default ice4j configuration in application.conf (this is not meant for users of jvb to change). On debian jvb also ships with /etc/jitsi/videobridge/jvb.conf for configuration overrides provided by the user. This file is loaded by adding -Dconfig.file=/etc/jitsi/videobridge/jvb.conf to the java command line.

Thanks, I'll let this information be known to the package maintainer. In the meanwhile, we gotta keep the old option, because currently there is no such configuration passed to the packaged videobridge.

@karolyi
Copy link
Author

karolyi commented Sep 5, 2021

Hey @bgrozev,

Out of curiosity I went ahead and compiled a videobridge with using my modifications on the ice4j project. When I copied it over to my server and tried to start it up, it didn't even start to bind on any UDP port, much less emitting logs about initializing SinglePortUdpHarvesters.

I'm just looking into the code right now, and besides the SinglePortUdpHarvester.java and its test, don't find any reference to SinglePortUdpHarvester. It seems to me that the new ice4j code doesn't even use it.

Can you doublecheck please if this is the case? I'm confused as to why the code therein doesn't even run.

@bgrozev
Copy link
Member

bgrozev commented Sep 8, 2021

Hey @karolyi

The bridge initializes SinglePortUdpHarvesters on its own here: https://github.com/jitsi/jitsi-videobridge/blob/master/jvb/src/main/java/org/jitsi/videobridge/ice/Harvesters.java#L83

@karolyi
Copy link
Author

karolyi commented Sep 9, 2021

Interesting, then I have no idea as to why it's not initializing any harvesters.

That said, the videobridge in FreeBSD seems pretty old (still uses jitsi-jitsi-videobridge-2.1-183-dbddd16_GH0.tar.gz from github), a lot of changes have happenes since.

I just want to get my changes working so it can end up in FreeBSD already.

@bgrozev
Copy link
Member

bgrozev commented Sep 9, 2021

jitsi-jitsi-videobridge-2.1-183-dbddd16_GH0.tar.gz

This is very old, it might not even be compatible with ice4j master.

@karolyi
Copy link
Author

karolyi commented Sep 11, 2021

That shouldn't be a problem, as when I get in touch with the maintainer, we'll update the whole shebang.

I have compiled from the latest videobridge and didn't get any SinglePortUdpHarvesters started up, that's the problem. Until I don't get it working, it makes no sense the contact the maintainer to have it updated in the ports tree as well, because it's not guaranteed to work.

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.

3 participants