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

Set total/needed/happy from command-line #376

Merged
merged 2 commits into from
Dec 14, 2016

Conversation

meejah
Copy link
Contributor

@meejah meejah commented Nov 11, 2016

This allows you to specify values on the CLI for needed/happy/total shares configuration parameters. This eases scripts that are creating nodes (so they don't have to know how to edit tahoe.cfg immediately after creating a node).

@pataquets
Copy link
Contributor

Also useful for Docker-based workflows, since you can configure nodes using a handful of env vars. Definitely +1.

@meejah
Copy link
Contributor Author

meejah commented Nov 14, 2016

Hmm, it seems that something changed in how the defaults stuff works. e.g. config.get("introducer", "") returns None instead of ""...

@codecov-io
Copy link

codecov-io commented Nov 14, 2016

Current coverage is 89.82% (diff: 100%)

Merging #376 into master will decrease coverage by 0.01%

@@             master       #376   diff @@
==========================================
  Files           136        136          
  Lines         26835      26840     +5   
  Methods           0          0          
  Messages          0          0          
  Branches       3746       3747     +1   
==========================================
+ Hits          24107      24108     +1   
- Misses         2025       2026     +1   
- Partials        703        706     +3   

Powered by Codecov. Last update 065f12c...37c7d54

@david415
Copy link
Member

looks good to me!

@crwood
Copy link
Member

crwood commented Nov 16, 2016

Shouldn't -N be used as the short flag for --total (instead of -t), and -K for --needed (instead of -N)?

From Q2 in the Tahoe-LAFS FAQ:

The number of overall storage servers, we call N and the number needed K and write the parameters such that it is "K-of-N".

@lpirl
Copy link
Contributor

lpirl commented Nov 16, 2016

I always felt the extra mapping (total shares → N, needed shares → k) adds mental load without benefit (also when reading the FAQ @crwood cited).
Vote for t and N.
Oh and may t could be a capital letter as well for consistency!?

@meejah
Copy link
Contributor Author

meejah commented Nov 24, 2016

So actually I think there shouldn't be any short-options at all for this, but I didn't find a way to do that with Twisted usage.Options...so I picked some.

I think that since we do seem to have to have short-options, using the same single-letters as other documents (per @crwood's suggestion) seems the best/most-consistent.

@meejah
Copy link
Contributor Author

meejah commented Nov 24, 2016

Okay, I changed the short-options to: --total = N, --needed = K and --happy = H

@meejah
Copy link
Contributor Author

meejah commented Nov 28, 2016

I found a way to take out the short options, so I did. So, the only way to specify these now is via the (hopefully not confusing) long-options.

@meejah
Copy link
Contributor Author

meejah commented Dec 13, 2016

See corresponding ticket: https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2848#ticket

@meejah
Copy link
Contributor Author

meejah commented Dec 13, 2016

What about "--shares-needed" etc, since that's what tahoe.cfg calls them? (well, shares.needed in the config).

@meejah
Copy link
Contributor Author

meejah commented Dec 13, 2016

okay, changed to --shares-needed, --shares-happy etc and squashed everything.

@warner
Copy link
Member

warner commented Dec 13, 2016

@warner
Copy link
Member

warner commented Dec 14, 2016

Any particular reason to put the defaults in a config.get(x,DEFAULT) statement, rather than putting them into the Usage.optParameters default slot? I think if they were in the latter, then tahoe create-node --help would display the defaults.

diff --git a/src/allmydata/scripts/create_node.py b/src/allmydata/scripts/create_node.py
index 49aa0a0ba..06a93a310 100644
--- a/src/allmydata/scripts/create_node.py
+++ b/src/allmydata/scripts/create_node.py
@@ -152,9 +152,9 @@ class CreateClientOptions(_CreateBaseOptions):
          "Specify which TCP port to run the HTTP interface on. Use 'none' to disable."),
         ("basedir", "C", None, "Specify which Tahoe base directory should be used. This has the same effect as the global --node-directory option. [default: %s]"
          % quote_local_unicode_path(_default_nodedir)),
-        ("shares-total", None, None, "Total shares required for uploaded files."),
-        ("shares-needed", None, None, "Needed shares required for uploaded files."),
-        ("shares-happy", None, None, "How many servers new files must be placed on."),
+        ("shares-total", None, "10", "Total shares created for uploaded files."),
+        ("shares-needed", None, "3", "Number of shares required for uploaded files."),
+        ("shares-happy", None, "7", "How many servers new files must be placed on."),
         ]
 
     # This is overridden in order to ensure we get a "Wrong number of
@@ -291,9 +291,8 @@ def write_client_config(c, config):
     c.write("# This can be changed at any time: the encoding is saved in\n")
     c.write("# each filecap, and we can download old files with any encoding\n")
     c.write("# settings\n")
-    for verb, default in [('needed', 3), ('happy', 7), ('total', 10)]:
-        value = config.get('shares-{}'.format(verb)) or default
-        c.write("shares.{} = {}\n".format(verb, value))
+    for v in ['needed', 'happy', 'total']:
+        c.write("shares.{} = {}\n".format(v, config['shares-{}'.format(v)]))
     c.write("\n")
 
     boolstr = {True:"true", False:"false"}

@warner
Copy link
Member

warner commented Dec 14, 2016

Also you might flip the order of the keys, making it (needed, happy, total), so the --help listing shows the numbers in smallest-to-largest order (which also matches the order they're written into tahoe.cfg). It seems that Usage.optParameters retain their order when translated into --help output.

(some day we should look at the way optParameters inherits/combines with the parent classes, and make sure the merged --help output is grouped meaningfully.. at present, the --shares-total= stuff is jammed in-between two port-selection -related items, which is kind of awkward)

@warner
Copy link
Member

warner commented Dec 14, 2016

oh, and maybe add a check that the parameters are really integers, so --shares-happy=funball fails properly

@meejah
Copy link
Contributor Author

meejah commented Dec 14, 2016

@warner it seems there was a test passing a plain dict to write_client_config but I'm not sure if that's "a thing that's supported" or not. I could add the .get(..)s back in but then the defaults would be in two spots .. or just leave my above fix in.

@warner
Copy link
Member

warner commented Dec 14, 2016

Ah, right, dicts are a lazy substitute for the Config object.. your fix looks great. Will land it shortly.

@warner warner merged commit 37c7d54 into tahoe-lafs:master Dec 14, 2016
@meejah meejah deleted the create-node-cli-args branch December 14, 2016 19:23
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.

7 participants