You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/03/27 23:14:21 UTC

[GitHub] [couchdb] kocolosk opened a new pull request #3468: Improve search for FDB cluster files

kocolosk opened a new pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468


   ## Overview
   
   This patch introduces a handful of related improvements to the code that chooses the FoundationDB cluster file. We now check for a cluster file in up to 4 locations, in priority order:
   
   1. The value of `[erlfdb] cluster_file` in the server config
   2. The value of the FDB_CLUSTER_FILE environment variable
   3. /usr/local/etc/foundationdb/fdb.cluster (MacOS package default)
   4. /etc/foundationdb/fdb.cluster (Linux packages default)
   
   The second and fourth locations are new. We also check that the file has appropriate access permissions, namely that is RW accessible by the user running CouchDB. Finally, we add some more detailed logging to help users debug any problems in this department on startup.
   
   If a location is specified using either one of the first two "custom" approaches and there is a problem with that location, CouchDB will abort rather than try any further option. This avoids a situation where a user intended to override the default cluster file but CouchDB starts up with the default one because e.g. the user failed to actually install the new file with the correct permissions.
   
   ## Testing recommendations
   
   Still need to add a test suite here, and really some documentation too (although that would probably be a larger project involving documentation of the 4.0 stack as a whole). I could use a bit of guidance on the testing. Should I just verify the behavior of `find_cluster_file/1` with a mix of valid and invalid input? Is it better to create fixtures with bad permissions or just mock `file:read_file_info/2`?
   
   I've tested several aspects of this manually:
   - [x] checked that a custom cluster file with bad permissions causes a crash
   - [x] checked that FDB_CLUSTER_FILE can be used instead of the server config setting
   - [x] checked that CouchDB will find /etc/foundationdb/fdb.cluster without any config help
   
   ## Checklist
   
   - [x] Code is written and works correctly
   - [ ] Changes are covered by tests
   - [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
   - [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] nickva commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r602834049



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},
+        {custom, os:getenv("FDB_CLUSTER_FILE")},
+        {default, ?CLUSTER_FILE_MACOS},
+        {default, ?CLUSTER_FILE_LINUX}
+    ],
+    case find_cluster_file(Locations) of
+        {ok, Location} ->
+            Location;
+        {error, Reason} ->
+            erlang:error(Reason)
+    end.
+
+find_cluster_file([]) ->
+    {error, cluster_file_missing};
+find_cluster_file([{custom, false} | Rest]) ->

Review comment:
       Minor style nit: for consistency with the rest of the file, let's use double lines between functions and single lines between clauses.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] nickva commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r602825270



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},

Review comment:
       As a minor nit, perhaps we could switch to `undefined` since in more cases in our code we rely on `config:get(Section, Key)` returning `undefined` but are not as familiar with `os:getenv/1`. In order to use `undefined` we could use [`os:getenv/2`](https://erlang.org/doc/man/os.html#getenv-2) as in `os:getenv("FDB_CLUSTER_FILE", undefined)` then handle the `undefined` in `find_cluster_file` clauses.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] nickva commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r602832114



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},
+        {custom, os:getenv("FDB_CLUSTER_FILE")},
+        {default, ?CLUSTER_FILE_MACOS},
+        {default, ?CLUSTER_FILE_LINUX}
+    ],
+    case find_cluster_file(Locations) of
+        {ok, Location} ->
+            Location;
+        {error, Reason} ->
+            erlang:error(Reason)
+    end.
+
+find_cluster_file([]) ->
+    {error, cluster_file_missing};
+find_cluster_file([{custom, false} | Rest]) ->
+    find_cluster_file(Rest);
+find_cluster_file([{Type, Location} | Rest]) ->
+    case file:read_file_info(Location, [posix]) of
+        {ok, #file_info{access = read_write}} ->

Review comment:
       This might break some development setups where FDB server system packages are installed (deb, rpm, etc) and run as `root ` (or `foundationdb` user), and CouchDB instance is run as the logged in user. ~One way to address that edge case is perhaps having the `./dev/run` script make a local writable copy somewhere and use that when starting CouchDB, or may be just have a config setting set by `./dev/run` to skip the `writable` check.~ EDIT: setting `FDB_CLUSTER_FILE` to point to a writable copy of the cluster file did the trick




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] nickva commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r604154311



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},

Review comment:
       Ah makes sense then, if the dializer complains let's keep it as is. Maybe with a short comment about it?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] bessbd commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r603015796



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->

Review comment:
       Nit: based on the name only I wasn't able to tell if it returns the path or the contents of a cluster file.
   How about something like `get_cluster_file_path` here?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] nickva commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r602825270



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},

Review comment:
       As a minor nit, I wonder if we could switch to use `undefined` since in more cases in our code we rely on `config:get(Section, Key)` returning `undefined` but are not as familiar with `os:getenv/1`. In order to use `undefined` we could use [`os:getenv/2`](https://erlang.org/doc/man/os.html#getenv-2) as in `os:getenv("FDB_CLUSTER_FILE", undefined)` then handle the `undefined` in `find_cluster_file` clauses.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] kocolosk commented on pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
kocolosk commented on pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#issuecomment-810344773


   I would be open to downgrading the permissions check to a non-fatal error message. I suspect we could end up with a lot of situations where CouchDB refuses to start but the ability to modify the cluster file is a non-issue; e.g. it's a single-server deployment, or a cluster where the FDB server nodes are co-located with the CouchDB nodes (and likely able to modify the cluster file themselves).
   
   The situation we're concerned about is where CouchDB is running as a stateless node and only using the local FDB client to communicate with a remote FDB cluster, _and_ that cluster's coordinators end up changing over time, _and_ there's no out-of-band mechanism to ensure the cluster file gets updated on the CouchDB nodes. It's a fairly sophisticated scenario, and I'd be comfortable with CouchDB logging an error message on startup along with some documentation about the appropriate permissions.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] bessbd commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r603015796



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->

Review comment:
       Based on the name only I wasn't able to tell if it returns the path or the contents of a cluster file.
   How about `get_cluster_file_path` here?

##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->

Review comment:
       Based on the name only I wasn't able to tell if it returns the path or the contents of a cluster file.
   How about something like `get_cluster_file_path` here?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] kocolosk commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r604495867



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},
+        {custom, os:getenv("FDB_CLUSTER_FILE")},
+        {default, ?CLUSTER_FILE_MACOS},
+        {default, ?CLUSTER_FILE_LINUX}
+    ],
+    case find_cluster_file(Locations) of
+        {ok, Location} ->
+            Location;
+        {error, Reason} ->
+            erlang:error(Reason)
+    end.
+
+find_cluster_file([]) ->
+    {error, cluster_file_missing};
+find_cluster_file([{custom, false} | Rest]) ->
+    find_cluster_file(Rest);
+find_cluster_file([{Type, Location} | Rest]) ->
+    case file:read_file_info(Location, [posix]) of
+        {ok, #file_info{access = read_write}} ->
+            couch_log:info("Using ~s FDB cluster file: ~s", [Type, Location]),
+            {ok, Location};
+        {ok, _} ->
+            couch_log:error("CouchDB needs read/write access to FDB cluster file: ~s", [Location]),

Review comment:
       New behavior logs this message instead when it finds a read-only cluster file:
   
   > Using read-only default FDB cluster file: /usr/local/etc/foundationdb/fdb.cluster -- if coordinators are changed without updating this file CouchDB may be unable to connect to the FDB cluster!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] nickva commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r602832520



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},
+        {custom, os:getenv("FDB_CLUSTER_FILE")},
+        {default, ?CLUSTER_FILE_MACOS},
+        {default, ?CLUSTER_FILE_LINUX}
+    ],
+    case find_cluster_file(Locations) of
+        {ok, Location} ->
+            Location;
+        {error, Reason} ->
+            erlang:error(Reason)
+    end.
+
+find_cluster_file([]) ->
+    {error, cluster_file_missing};
+find_cluster_file([{custom, false} | Rest]) ->
+    find_cluster_file(Rest);
+find_cluster_file([{Type, Location} | Rest]) ->
+    case file:read_file_info(Location, [posix]) of
+        {ok, #file_info{access = read_write}} ->
+            couch_log:info("Using ~s FDB cluster file: ~s", [Type, Location]),
+            {ok, Location};
+        {ok, _} ->
+            couch_log:error("CouchDB needs read/write access to FDB cluster file: ~s", [Location]),
+            {error, cluster_file_permissions};
+        {error, Reason} when Type =:= custom ->
+            couch_log:error("Encountered ~p error looking for FDB cluster file: ~s", [Reason, Location]),
+            {error, Reason};
+        {error, Reason} when Type =:= default ->

Review comment:
       On Linux hosts this would emit an `Encountered enoent error looking for FDB cluster file: /usr/local/etc/foundationdb/fdb.cluster` warning. Maybe we could add an explicit `enoent` clause to issue an info log message instead of the `enoent` error, since it's an expected scenario.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] kocolosk commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r604181663



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},

Review comment:
       I didn't get a complaint so I went ahead and made that change.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] ksnavely commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
ksnavely commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r603429201



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},
+        {custom, os:getenv("FDB_CLUSTER_FILE")},
+        {default, ?CLUSTER_FILE_MACOS},
+        {default, ?CLUSTER_FILE_LINUX}
+    ],
+    case find_cluster_file(Locations) of
+        {ok, Location} ->
+            Location;
+        {error, Reason} ->
+            erlang:error(Reason)
+    end.
+
+find_cluster_file([]) ->
+    {error, cluster_file_missing};
+find_cluster_file([{custom, false} | Rest]) ->
+    find_cluster_file(Rest);
+find_cluster_file([{Type, Location} | Rest]) ->
+    case file:read_file_info(Location, [posix]) of
+        {ok, #file_info{access = read_write}} ->
+            couch_log:info("Using ~s FDB cluster file: ~s", [Type, Location]),
+            {ok, Location};
+        {ok, _} ->
+            couch_log:error("CouchDB needs read/write access to FDB cluster file: ~s", [Location]),

Review comment:
       This might seem alarming, maybe add a comment that the FDB C client is the thing that may need to change the contents of the cluster-file.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] nickva commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r603455776



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},
+        {custom, os:getenv("FDB_CLUSTER_FILE")},
+        {default, ?CLUSTER_FILE_MACOS},
+        {default, ?CLUSTER_FILE_LINUX}
+    ],
+    case find_cluster_file(Locations) of
+        {ok, Location} ->
+            Location;
+        {error, Reason} ->
+            erlang:error(Reason)
+    end.
+
+find_cluster_file([]) ->
+    {error, cluster_file_missing};
+find_cluster_file([{custom, false} | Rest]) ->
+    find_cluster_file(Rest);
+find_cluster_file([{Type, Location} | Rest]) ->
+    case file:read_file_info(Location, [posix]) of
+        {ok, #file_info{access = read_write}} ->
+            couch_log:info("Using ~s FDB cluster file: ~s", [Type, Location]),
+            {ok, Location};
+        {ok, _} ->
+            couch_log:error("CouchDB needs read/write access to FDB cluster file: ~s", [Location]),

Review comment:
       Good idea to mention it in erlfdb somewhere referencing https://apple.github.io/foundationdb/administration.html#required-permissions perhaps




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] wohali commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
wohali commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r603446792



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -42,9 +42,10 @@
 
 
 -include_lib("couch/include/couch_db.hrl").
+-include_lib("kernel/include/file.hrl").
 
-
--define(CLUSTER_FILE, "/usr/local/etc/foundationdb/fdb.cluster").
+-define(CLUSTER_FILE_MACOS, "/usr/local/etc/foundationdb/fdb.cluster").
+-define(CLUSTER_FILE_LINUX, "/etc/foundationdb/fdb.cluster").

Review comment:
       I wasn't planning on doing WIndows support until all of this stuff settled down... I only want to have to touch it **once**.
   
   Are we really close to a 4.0 release now?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] kocolosk merged pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
kocolosk merged pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] rnewson commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r603418541



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -42,9 +42,10 @@
 
 
 -include_lib("couch/include/couch_db.hrl").
+-include_lib("kernel/include/file.hrl").
 
-
--define(CLUSTER_FILE, "/usr/local/etc/foundationdb/fdb.cluster").
+-define(CLUSTER_FILE_MACOS, "/usr/local/etc/foundationdb/fdb.cluster").
+-define(CLUSTER_FILE_LINUX, "/etc/foundationdb/fdb.cluster").

Review comment:
       cc @wohali 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] nickva commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r602824561



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -42,9 +42,10 @@
 
 
 -include_lib("couch/include/couch_db.hrl").
+-include_lib("kernel/include/file.hrl").
 
-
--define(CLUSTER_FILE, "/usr/local/etc/foundationdb/fdb.cluster").
+-define(CLUSTER_FILE_MACOS, "/usr/local/etc/foundationdb/fdb.cluster").
+-define(CLUSTER_FILE_LINUX, "/etc/foundationdb/fdb.cluster").

Review comment:
       Wonder if we could add Windows to the list. Not sure how supported it is, but I found the default cluster file location in the Rust library https://docs.rs/foundationdb/0.5.0/src/foundationdb/lib.rs.html#171-174




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] kocolosk commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r604089933



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -42,9 +42,10 @@
 
 
 -include_lib("couch/include/couch_db.hrl").
+-include_lib("kernel/include/file.hrl").
 
-
--define(CLUSTER_FILE, "/usr/local/etc/foundationdb/fdb.cluster").
+-define(CLUSTER_FILE_MACOS, "/usr/local/etc/foundationdb/fdb.cluster").
+-define(CLUSTER_FILE_LINUX, "/etc/foundationdb/fdb.cluster").

Review comment:
       I don't think a 4.0 release is imminent, but I can take a shot at extending this PR to do best-effort Windows support.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] nickva commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r602832114



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},
+        {custom, os:getenv("FDB_CLUSTER_FILE")},
+        {default, ?CLUSTER_FILE_MACOS},
+        {default, ?CLUSTER_FILE_LINUX}
+    ],
+    case find_cluster_file(Locations) of
+        {ok, Location} ->
+            Location;
+        {error, Reason} ->
+            erlang:error(Reason)
+    end.
+
+find_cluster_file([]) ->
+    {error, cluster_file_missing};
+find_cluster_file([{custom, false} | Rest]) ->
+    find_cluster_file(Rest);
+find_cluster_file([{Type, Location} | Rest]) ->
+    case file:read_file_info(Location, [posix]) of
+        {ok, #file_info{access = read_write}} ->

Review comment:
       This might break some development setups where FDB server system packages are installed (deb, rpm, etc) and run as `root ` (or `foundationdb` user), and CouchDB instance is run as the logged in user. One way to address that edge case is perhaps having the `./dev/run` script make a local writable copy somewhere and use that when starting CouchDB, or may be just have a config setting set by `./dev/run` to skip the `writable` check.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] kocolosk commented on a change in pull request #3468: Improve search for FDB cluster files

Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3468:
URL: https://github.com/apache/couchdb/pull/3468#discussion_r604091186



##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -212,14 +213,47 @@ get_db_and_cluster(EunitDbOpts) ->
         {ok, true} ->
             {<<"eunit_test">>, erlfdb_util:get_test_db(EunitDbOpts)};
         undefined ->
-            ClusterFileStr = config:get("erlfdb", "cluster_file", ?CLUSTER_FILE),
+            ClusterFileStr = get_cluster_file_string(),
             {ok, ConnectionStr} = file:read_file(ClusterFileStr),
             DbHandle = erlfdb:open(iolist_to_binary(ClusterFileStr)),
             {string:trim(ConnectionStr), DbHandle}
     end,
     apply_tx_options(Db, config:get(?TX_OPTIONS_SECTION)),
     {Cluster, Db}.
 
+get_cluster_file_string() ->
+    Locations = [
+        {custom, config:get("erlfdb", "cluster_file", false)},

Review comment:
       I had started with that approach. Technically it violates the spec for `os:getenv/2` to use an atom as the second argument, but it does work. Will check if dialyzer complains.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org