[RTC-345] Introduce JF dedicated env vars for running in a distributed mode #92

mickel8 · 2023-09-20T17:21:53Z

This PR introduces our own env variables for running JF in a cluster.

When we run erlang/elixir locally, we can setup distribution using flags like --sname --name --cookie etc. However, when we make a release, we are provided with special env variables e.g. RELEASE_NODE RELEASE_COOKIE etc. which results in multiple configuration options depending on environment we run Jellyfish in.

Intoducing our own env vars we:

unify how JF is configured
have better env names e.g. JF_COOKIE instead of strange RELEASE_COOKIE
make future documentation easier to understand as our env vars are always available and always the same. RELEASE_* vars are only available after doing mix release so this would add unnecessary complexity in the documentation
we also follow livebook behaviour, see this PR Support setting node name/sname using environment vars livebook-dev/livebook#1672

codecov · 2023-09-20T17:27:56Z

Codecov Report

Merging #92 (27f163c) into main (5888e33) will decrease coverage by 0.90%.
Report is 1 commits behind head on main.
The diff coverage is 60.71%.

@@            Coverage Diff             @@
##             main      #92      +/-   ##
==========================================
- Coverage   84.27%   83.37%   -0.90%     
==========================================
  Files          44       44              
  Lines         744      764      +20     
==========================================
+ Hits          627      637      +10     
- Misses        117      127      +10

Files	Coverage Δ
lib/jellyfish/config_reader.ex	`100.00% <100.00%> (ø)`
lib/jellyfish/application.ex	`36.84% <21.42%> (-43.16%)`	⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5888e33...27f163c. Read the comment docs.

Rados13

As we are making that many change in configuration, maybe we should also add possibility to change the port on which epmd runs ERL_EPMD_PORT env (more about epmd envs here). Other variables worth setting IMO are inet_dist_listen_min and inet_dist_listen_max. We already use these variables in jellyfish_videoroom, but we configure it by passing the option to elixir, which passes it as an option to erlang. Both of these changes we could add in the next PR.

.env.sample

lib/jellyfish/application.ex

sgfn

A general idea I have is to group all of the distribution env variables under the same prefix, e.g.

JF_DIST_ENABLED -> no change
JF_NODE_NAME -> JF_DIST_NODE_NAME
JF_COOKIE -> JF_DIST_COOKIE
JF_NODES -> JF_DIST_NODES

IMO this isn't overly verbose, and would clearly communicate that these variables are meant to be used together, and that users need to pay attention to them only if they wish to run Jellyfish in a cluster.

.env.sample

sgfn · 2023-09-25T09:20:11Z

lib/jellyfish/config_reader.ex

+    if read_boolean("JF_DIST_ENABLED") do
+      node_name_value = System.get_env("JF_NODE_NAME")
+      cookie_value = System.get_env("JF_COOKIE")
+      nodes_value = System.get_env("JF_NODES") || ""


Nitpick

Suggested change

nodes_value = System.get_env("JF_NODES") || ""

nodes_value = System.get_env("JF_NODES", "")

lib/jellyfish/config_reader.ex

sgfn · 2023-09-25T09:24:53Z

.env.sample

+# The second part has to be Jellyfish address
+# it is accessible on


Suggested change

# The second part has to be Jellyfish address

# it is accessible on

# The second part has to be the address Jellyfish is accessible on

sgfn · 2023-09-25T09:27:48Z

rel/env.sh.eex

+# release always starts in a distributed mode
+# but to provide unified way of configuring Jellyfish
+# distribution both in the local and production environment
+# we introduce our own env vars and use them to
+# set RELASE_NODE and RELASE_COOKIE


Suggested change

# release always starts in a distributed mode

# but to provide unified way of configuring Jellyfish

# distribution both in the local and production environment

# we introduce our own env vars and use them to

# set RELASE_NODE and RELASE_COOKIE

# We introduce our own env vars and use them to set

# RELEASE_NODE and RELEASE_COOKIE.

#

# This is to provide a unified way of configuring Jellyfish distribution

# in both development and production environments

The info that release always starts in a distributed mode is crucial here

Actually maybe not 🤔

mickel8 · 2023-09-25T09:39:33Z

JF_DIST_ENABLED -> no change
JF_NODE_NAME -> JF_DIST_NODE_NAME
JF_COOKIE -> JF_DIST_COOKIE
JF_NODES -> JF_DIST_NODES

@sgfn Can do that but what about WebRTC or RTSP? I think we should prefix them too

sgfn · 2023-09-25T09:57:01Z

@sgfn Can do that but what about WebRTC or RTSP? I think we should prefix them too

Yeah, I've no problem with that, we should go for it

roznawsk · 2023-09-25T14:12:11Z

+1 from for comments from @sgfn - other than that LGTM

roznawsk · 2023-09-27T13:59:48Z

.env.sample

@@ -20,8 +20,7 @@ JF_HOST=localhost:8080

 # Node name used in a cluster.
 # The first part can be any string.
-# The second part has to be Jellyfish address
-# it is accessible on
+# The second part has to be Jellyfish address # it is accessible on.


Suggested change

# The second part has to be Jellyfish address # it is accessible on.

# The second part has to be Jellyfish address it is accessible on.

Rados13

Besides one issue LGTM

rel/env.sh.eex

sgfn

Approved, with one small note:

Regarding the .env.sample file: I'm still firmly against making the core Jellyfish documentation spread out in two or more places -- could you just confirm we will duplicate all of this info in the docs, and not have the docs say: "for more info, refer to the .env.sample file"?

mickel8 · 2023-09-28T09:42:28Z

Approved, with one small note:

Regarding the .env.sample file: I'm still firmly against making the core Jellyfish documentation spread out in two or more >places -- could you just confirm we will duplicate all of this info in the docs, and not have the docs say: "for more info, >refer to the .env.sample file"?

Sure, I will revisit this once I write official docs

mickel8 force-pushed the dist-config branch 2 times, most recently from d390a96 to a565b54 Compare September 20, 2023 17:24

mickel8 force-pushed the dist-config branch 13 times, most recently from 187881c to 13600f8 Compare September 21, 2023 07:36

mickel8 force-pushed the prefix-envs branch 3 times, most recently from fcca80d to 75452e3 Compare September 22, 2023 14:22

Base automatically changed from prefix-envs to main September 22, 2023 15:02

mickel8 force-pushed the dist-config branch 4 times, most recently from 9df13c4 to b417a03 Compare September 22, 2023 15:53

mickel8 marked this pull request as ready for review September 22, 2023 15:53

Introduce JF dedicated env vars for running in a distributed mode

8765fe9

mickel8 force-pushed the dist-config branch from b417a03 to 8765fe9 Compare September 22, 2023 16:02

mickel8 requested review from Rados13, sgfn and roznawsk September 22, 2023 16:02

Rados13 reviewed Sep 25, 2023

View reviewed changes

.env.sample Outdated Show resolved Hide resolved

lib/jellyfish/application.ex Show resolved Hide resolved

sgfn reviewed Sep 25, 2023

View reviewed changes

mickel8 force-pushed the main branch from 77d8596 to 4a3173b Compare September 26, 2023 08:17

Add JF_DIST_MIN_PORT and JF_DIST_MAX_PORT

61dc6b8

mickel8 force-pushed the dist-config branch from a03a062 to 61dc6b8 Compare September 26, 2023 14:15

Force longnames

a85ad9c

mickel8 force-pushed the dist-config branch 2 times, most recently from d99b0f6 to 0c669c3 Compare September 26, 2023 15:08

Set default cookie value

c33e893

mickel8 force-pushed the dist-config branch from 0c669c3 to c33e893 Compare September 26, 2023 15:17

mickel8 added 2 commits September 26, 2023 17:20

Add JF_DIST_MODE

1d96682

Small docs improvements

4437fc9

roznawsk approved these changes Sep 27, 2023

View reviewed changes

mickel8 force-pushed the dist-config branch from b87b5a9 to 316e5d0 Compare September 27, 2023 15:34

Group env vars

eedfb37

mickel8 force-pushed the dist-config branch from 316e5d0 to eedfb37 Compare September 27, 2023 15:39

mickel8 added 4 commits September 27, 2023 17:59

Explain JF_DIST_MODE

ce89422

Change default cookie

5fe98dd

Mention about JF_DIST_MIN/MAX_PORT availability

8f070dc

Refactor .env.sample

27f163c

mickel8 requested review from sgfn and Rados13 September 27, 2023 16:09

Rados13 approved these changes Sep 28, 2023

View reviewed changes

rel/env.sh.eex Show resolved Hide resolved

sgfn approved these changes Sep 28, 2023

View reviewed changes

mickel8 merged commit 0fd776b into main Sep 28, 2023
7 checks passed

mickel8 deleted the dist-config branch September 28, 2023 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTC-345] Introduce JF dedicated env vars for running in a distributed mode #92

[RTC-345] Introduce JF dedicated env vars for running in a distributed mode #92

mickel8 commented Sep 20, 2023 •

edited

codecov bot commented Sep 20, 2023 •

edited

Rados13 left a comment

sgfn left a comment

sgfn Sep 25, 2023

sgfn Sep 25, 2023

sgfn Sep 25, 2023

mickel8 Sep 26, 2023

mickel8 Sep 26, 2023

mickel8 commented Sep 25, 2023 •

edited

sgfn commented Sep 25, 2023

roznawsk commented Sep 25, 2023 •

edited

roznawsk Sep 27, 2023

Rados13 left a comment

sgfn left a comment

mickel8 commented Sep 28, 2023

	nodes_value = System.get_env("JF_NODES") \|\| ""
	nodes_value = System.get_env("JF_NODES", "")

		# The second part has to be Jellyfish address
		# it is accessible on

	# The second part has to be Jellyfish address
	# it is accessible on
	# The second part has to be the address Jellyfish is accessible on

[RTC-345] Introduce JF dedicated env vars for running in a distributed mode #92

[RTC-345] Introduce JF dedicated env vars for running in a distributed mode #92

Conversation

mickel8 commented Sep 20, 2023 • edited

codecov bot commented Sep 20, 2023 • edited

Codecov Report

Rados13 left a comment

Choose a reason for hiding this comment

sgfn left a comment

Choose a reason for hiding this comment

sgfn Sep 25, 2023

Choose a reason for hiding this comment

sgfn Sep 25, 2023

Choose a reason for hiding this comment

sgfn Sep 25, 2023

Choose a reason for hiding this comment

mickel8 Sep 26, 2023

Choose a reason for hiding this comment

mickel8 Sep 26, 2023

Choose a reason for hiding this comment

mickel8 commented Sep 25, 2023 • edited

sgfn commented Sep 25, 2023

roznawsk commented Sep 25, 2023 • edited

roznawsk Sep 27, 2023

Choose a reason for hiding this comment

Rados13 left a comment

Choose a reason for hiding this comment

sgfn left a comment

Choose a reason for hiding this comment

mickel8 commented Sep 28, 2023

mickel8 commented Sep 20, 2023 •

edited

codecov bot commented Sep 20, 2023 •

edited

mickel8 commented Sep 25, 2023 •

edited

roznawsk commented Sep 25, 2023 •

edited