Remove name collisions of replaced files in multi-node setups by saleyn · Pull Request #427 · erlware/relx

saleyn · 2015-12-22T17:38:41Z

Generated vm.2.args and sys.2.config files are renamed to vm.${NAME}.args and sys.${NAME}.config to avoid naming collisions in the multi-node startup off of the same release dir.

evanmcc · 2015-12-22T18:22:13Z

Thanks! I can't get this to work right now but have to run. Hopefully I will have some time this afternoon to poke at it.

saleyn · 2015-12-22T18:24:46Z

It seems to work fine in my Linux setup with multiple nodes.

evanmcc · 2015-12-23T17:21:59Z

From reading the code, this seems like it'd work well, and I vlosed my PR #357 in favor of this one. That said, I can't make it play nice with rebar3 on my local setup for reasons that seem unrelated to relx. So can't test right now. Happy to dig into it more after vacation, or just to see if merged if no one else is having issues getting it to work as my problem is likely unrelated.

saleyn · 2015-12-23T17:39:30Z

What you need to do to make it work with rebar3 is to remove the rebar.lock, so that it'd get regenerated, and also apply this (until rebar3 is modified to take the latest relx changes):

diff --git a/rebar.config b/rebar.config
index 276acff..156219c 100644
--- a/rebar.config
+++ b/rebar.config
@@ -7,7 +7,7 @@
         {providers,           "1.6.0"},
         {getopt,              "0.8.2"},
         {bbmustache,          "1.0.4"},
-        {relx,                "3.9.0"},
+        {relx,  ".*",         {git, "https://github.com/saleyn/relx.git", {branch, "vars"}}},
         {cf,                  "0.2.1"},
         {cth_readable,        "1.1.0"},
         {eunit_formatters,    "0.3.1"}]}.

ferd · 2015-12-23T18:13:52Z

you don't need to remove the rebar.lock. You can call rebar3 upgrade <app> or rebar3 unlock <app> so you can modify only the required dependencies without unfreezing the other ones (unless transient dependencies are modified by the upgrade, which then gets reflected in the lock file)

saleyn · 2015-12-23T19:22:41Z

Thanks for the tip.

tsloughter · 2016-02-06T23:48:11Z

@saleyn rebase pls

@evanmcc still planning to give this a try? Would like your feedback.

evanmcc · 2016-06-13T00:26:49Z

 # Generate a random id
 relx_gen_id() {
-    od -t x -N 4 /dev/urandom | head -n1 | awk '{print $2}'
+    od -X -N 4 /dev/urandom | head -n1 | gawk '{print $2}'


is there some behavior of gawk that we need here? it isn't universally installed

It's not needed in this particular line, but may be needed in other place (see comments below), so this one has been renamed to match other gawk invocations.

evanmcc · 2016-06-13T01:06:50Z

Sorry that I took ages to look at this; however it does not seem to work correctly with rebar3. It isn't getting the correct location for my vm.args and thus can't generate per-node versions.

saleyn · 2016-06-13T03:14:58Z

Perhaps you can elaborate a bit more with examples? I have been using this approach/patch in production on multi-node Linux systems for a while now without issues.

lrascao · 2016-09-25T22:34:42Z

@saleyn i know it's been a while but could you please rebase onto master so i could give a try as well?
thanks!

saleyn · 2016-09-26T02:17:26Z

I rebased the branch to master.

lrascao · 2016-10-04T21:45:55Z

@saleyn, we've since put some tests in place that exercise the extended_bin script, could you please rebase onto master (again, i'm sorry) and try those out? Also it would be helpful if you could squash the 3 commits into just 1 please

saleyn · 2016-10-04T22:15:16Z

Done

lrascao · 2016-10-04T23:55:17Z

commit cf5ed97 removed a lot of bash related stuff due to it not being supported in Solaris/SmartOS, is the switch to bash strictly necessary to make the multi-node setup work?

saleyn · 2016-10-05T01:26:28Z

The multi-node setup doesn't depend on bash features. The bash dependency was created to support advanced variable substitution (e.g. ${NODE_NAME,,}, ${SOME_NAME^^}, ${SOME_OTHER_VALUE:-default_value}) - which were the features I was after in the first place. Support for multi-node setup was a secondary feature added into this commit, which is merely naming sys.config and vm.args to include a node name to make it unique.

lrascao · 2016-10-05T09:48:59Z

I see, maybe it's best to separate the two issues, one PR for multi-node setup and a different one for advanced variable substitution (perhaps with a different approach than replace sh with bash)

saleyn · 2016-10-05T12:49:19Z

How about having two separate extended_bin files - one for those OS's that have bash (which is the majority) and one for those that don't. The alternative is to have a more complex script that checks the value of current $SHELL and calls shell-specific functions.

lrascao · 2016-10-05T13:05:29Z

That's one way, i'm sure more people will want to weigh in on this, still i think we should split this into PRs and keep the bash discussion in the new one, what do you think?

saleyn · 2016-10-05T16:02:53Z

I'll look into it some time next week.

GoelDeepak · 2016-10-27T23:04:54Z

@saleyn I agree with you to keep separate script for different OS. It keeps the individual script clean. Also, as @lrascao mentioned we should probably separate bash related changes into a separate PR.

This update extends PR erlware#427 by supporting variable replacement syntax of bash to be used in vm.args and sys.config. The changes are backward-compatible with non-bash shells.

GoelDeepak · 2016-10-28T18:00:51Z

@lrascao @saleyn The latest commit (faddc92) creates files with random name in /tmp directory. Wouldn't that resolve the problem of multi-node? Is there anything else that we still have to achieve?

saleyn · 2016-10-28T22:21:14Z

I believe that the approach of creating random files in /tmp makes the problem more complex than it should be - it creates a ton of files that need to be garbage collected somehow, and also when there are several users testing, the issue of finding the right files is not straight-forward.

Additionally, the proposed version allows for the node name in the vm.args to contain variables as well, and the NAME gets properly evaluated prior to including it in the generated vm.*.args and sys.*.config.

GoelDeepak · 2016-10-28T23:22:01Z

@saleyn Make sense. Can we add a parameter for the location of the modified vm..args (sys..config)? By default, it would use the same location as vm.args. However, if some other location is provided then we could use that.

saleyn · 2016-10-29T00:38:21Z

You are welcome to send a patch.

lrascao · 2016-10-29T12:28:09Z

so to sum up the changes still needed:

a separate env var to specify the location of the replaced vm.args/sys.config, default is same location as original ones
when creating vm.args/sys.config with replaced vars add ${NAME} to the filename

am i missing anything else?

saleyn · 2016-10-30T17:02:41Z

Not sure if you described the changes needed before or after applying my reworked patch. If you meant the changes needed after applying the patch, then it's only the first bullet. The second bullet is already covered by the patch.

lrascao · 2016-10-30T17:17:44Z

i meant before your patch, do you think you can narrow down the changes to only contain the strictly necessary to implement the second bullet? There are a lot of other changes that seem unrelated

saleyn · 2016-10-30T17:30:46Z

If your comment is for changes "before the patch", then the third bullet is to allow for node name in vm.args to contain shell variables as well.

My patch addresses bullets 2 and 3 only. Aside from that there's only one slight change that readlink is used with '-f' option, which canonicalizes the real script's path, so that it's clear where the script actually is if components of the path are links as well. Even though it seems like there are many changes - if you follow the changes, the majority is a result of merely rearranging the same code, so that the formation of ${NAME} takes place early enough to make it available BEFORE naming the OUT_FILE_PATH. Aside from that they bare no functional changes.

GoelDeepak · 2016-10-31T17:37:59Z

@saleyn @lrascao If we do not include first bullet then this patch will undo whatever I did in my patch. I feel we should include env variable in this patch itself.

lrascao · 2016-11-01T13:37:41Z

i also agree that is best to include it, @saleyn could you please make this change? i think it's just a matter of consulting a new env var that defaults to empy string and prefix the .orig files path with it

This update extends PR erlware#427 by supporting variable replacement syntax of bash to be used in vm.args and sys.config. The changes are backward-compatible with non-bash shells.

saleyn · 2016-11-01T17:26:40Z

Added optional RELX_OUT_FILE_PATH variable which customizes the location of generated vm.${NAME}.args and sys.${NAME}.config.

GoelDeepak · 2016-11-02T17:58:01Z

lgtm. Should we also append few tests to cover RELX_OUT_FILE_PATH case?

lrascao · 2016-11-02T18:04:55Z

thanks @saleyn, looks good
@GoelDeepak i've already got some in the works, i'm now trying to figure out how to squash into @saleyn's commit

lrascao · 2016-11-02T18:08:52Z

@saleyn could you please cherry-pick 77450da (lrascao@77450da) into your branch so the new tests can run?

lrascao · 2016-11-02T22:42:19Z

@saleyn sorry, the previous commit was missing the new test in the suite all/0, could you please pick again this time lrascao/relx@5227de18?

lrascao · 2016-11-03T09:49:30Z

cool, looks good, now it's just a matter of squashing 7cef84e and 09dc948 (6bc7af4 can be dropped) and rebasing onto master and it's good to merge

GoelDeepak · 2016-11-09T19:21:05Z

Any update on this one?

lrascao · 2016-11-09T22:24:49Z

@saleyn need a hand with this or just too busy?

saleyn · 2016-11-09T23:59:48Z

Was busy with other stuff. Check out now.

lrascao · 2016-11-11T16:46:56Z

thanks!

This update extends PR erlware#427 by supporting variable replacement syntax of bash to be used in vm.args and sys.config. The changes are backward-compatible with non-bash shells.

saleyn mentioned this pull request Dec 22, 2015

make config names unique #357

Closed

saleyn force-pushed the vars branch from 8bdfdf7 to 34924eb Compare December 25, 2015 15:03

saleyn mentioned this pull request Dec 31, 2015

Enhance evaluation of environment variables #426

Merged

saleyn force-pushed the vars branch from 34924eb to 56c6778 Compare March 6, 2016 14:36

saleyn force-pushed the vars branch from 56c6778 to 68f64a3 Compare May 23, 2016 16:27

evanmcc reviewed Jun 13, 2016
View reviewed changes

saleyn force-pushed the vars branch from 68f64a3 to 27aa59f Compare September 26, 2016 02:14

saleyn force-pushed the vars branch from 0d1dd41 to 901e7b4 Compare October 4, 2016 22:13

saleyn force-pushed the vars branch from 901e7b4 to 8072725 Compare October 4, 2016 22:54

lrascao mentioned this pull request Oct 27, 2016

Patch to fix #523 #526

Merged

saleyn force-pushed the vars branch from ee0478d to bfb6cd6 Compare October 28, 2016 17:39

saleyn force-pushed the vars branch from bfb6cd6 to 7cef84e Compare November 1, 2016 17:08

Remove name collisions of replaced files in multi-node setups

103af5c

saleyn force-pushed the vars branch from 09dc948 to 103af5c Compare November 9, 2016 23:59

lrascao merged commit c305c89 into erlware:master Nov 11, 2016

tsloughter mentioned this pull request Jan 15, 2017

impossible to use config files which point to other config files #562

Closed

Conversation

saleyn commented Dec 22, 2015

Uh oh!

evanmcc commented Dec 22, 2015

Uh oh!

saleyn commented Dec 22, 2015

Uh oh!

evanmcc commented Dec 23, 2015

Uh oh!

saleyn commented Dec 23, 2015

Uh oh!

ferd commented Dec 23, 2015

Uh oh!

saleyn commented Dec 23, 2015

Uh oh!

tsloughter commented Feb 6, 2016

Uh oh!

evanmcc Jun 13, 2016

Choose a reason for hiding this comment

Uh oh!

saleyn Sep 26, 2016

Choose a reason for hiding this comment

Uh oh!

evanmcc commented Jun 13, 2016

Uh oh!

saleyn commented Jun 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrascao commented Sep 25, 2016

Uh oh!

saleyn commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrascao commented Oct 4, 2016

Uh oh!

saleyn commented Oct 4, 2016

Uh oh!

lrascao commented Oct 4, 2016

Uh oh!

saleyn commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrascao commented Oct 5, 2016

Uh oh!

saleyn commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrascao commented Oct 5, 2016

Uh oh!

saleyn commented Oct 5, 2016

Uh oh!

GoelDeepak commented Oct 27, 2016

Uh oh!

GoelDeepak commented Oct 28, 2016

Uh oh!

saleyn commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GoelDeepak commented Oct 28, 2016

Uh oh!

saleyn commented Oct 29, 2016

Uh oh!

lrascao commented Oct 29, 2016

Uh oh!

saleyn commented Oct 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrascao commented Oct 30, 2016

Uh oh!

saleyn commented Oct 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GoelDeepak commented Oct 31, 2016

Uh oh!

lrascao commented Nov 1, 2016

Uh oh!

saleyn commented Nov 1, 2016

Uh oh!

GoelDeepak commented Nov 2, 2016

Uh oh!

saleyn commented Jun 13, 2016 •

edited

Loading

saleyn commented Sep 26, 2016 •

edited

Loading

saleyn commented Oct 5, 2016 •

edited

Loading

saleyn commented Oct 5, 2016 •

edited

Loading

saleyn commented Oct 28, 2016 •

edited

Loading

saleyn commented Oct 30, 2016 •

edited

Loading

saleyn commented Oct 30, 2016 •

edited

Loading