Upload (sometimes?) ignores shares.happy in tahoe.cfg #1830

New Issue

tahoe-lafs · 2012-10-26T02:45:03Z

davidsarah commented

2012-10-26 02:45:03 +00:00

kmarkley86 at /tahoe-lafs/trac-2024-07-25/issues/6274#comment:41:

I'm affected by the same fundamental problem [#1212]as, but by a different path. The fix identified earlier was to immutable/repairer.py, but I'm getting an error from immutable/upload.py.

Scenario: I'm using 2-of-4 encoding with shares.happy=4 on tahoe 1.8.1. From the CLI I do a tahoe check --repair on a file with shares {0, 2, 3} already existing on the grid but share 1 not existing, and I get an UploadUnhappinessError complaining that "we were asked to place shares on at least 7" servers. There are only 4 servers on my grid -- hence my choice of shares.happy=4.

I observed that in immutable/upload.py, BaseUploadable has a statement "default_encoding_param_happy = 7". I tried the experiment of changing this value to 4 (the shares.happy value in my tahoe.cfg) and then the repair succeeds without error.

So there must be a path through this code where the default_encoding_param_happy value is actually used instead of being overridden by the value in tahoe.cfg. (I think it smells a little that this object has defaults at all, instead of requiring the parameters to be provided.)

A subsequent patch on trunk added assertions to try to catch the problem:

In changeset:196bd583b6c4959c:
Add assertions to make sure that set_default_encoding_parameters is always called, rather than using hardcoded 3/7/10 defaults. Also update affected tests. Note that this by itself cannot fix the bug mentioned in /tahoe-lafs/trac-2024-07-25/issues/6274#comment:41, but it might make it easier to reproduce. refs #1212

kmarkley86: can you try again to reproduce the problem [] using trunk?

kmarkley86 at [/tahoe-lafs/trac-2024-07-25/issues/6274](/tahoe-lafs/trac-2024-07-25/issues/6274)#comment:41: > I'm affected by the same fundamental problem [#1212]as, but by a different path. The fix identified earlier was to immutable/repairer.py, but I'm getting an error from immutable/upload.py. > Scenario: I'm using 2-of-4 encoding with shares.happy=4 on tahoe 1.8.1. From the CLI I do a tahoe check --repair on a file with shares {0, 2, 3} already existing on the grid but share 1 not existing, and I get an UploadUnhappinessError complaining that "we were asked to place shares on at least 7" servers. There are only 4 servers on my grid -- hence my choice of shares.happy=4. > I observed that in immutable/upload.py, BaseUploadable has a statement "default_encoding_param_happy = 7". I tried the experiment of changing this value to 4 (the shares.happy value in my tahoe.cfg) and then the repair succeeds without error. > So there must be a path through this code where the default_encoding_param_happy value is actually used instead of being overridden by the value in tahoe.cfg. (I think it smells a little that this object has defaults at all, instead of requiring the parameters to be provided.) A subsequent patch on trunk added assertions to try to catch the problem: > In changeset:196bd583b6c4959c: > Add assertions to make sure that set_default_encoding_parameters is always called, rather than using hardcoded 3/7/10 defaults. Also update affected tests. Note that this by itself cannot fix the bug mentioned in [/tahoe-lafs/trac-2024-07-25/issues/6274](/tahoe-lafs/trac-2024-07-25/issues/6274)#comment:41, but it might make it easier to reproduce. refs #1212 kmarkley86: can you try again to reproduce the problem [] using trunk?

tahoe-lafs added the

labels 2012-10-26 02:45:03 +00:00

tahoe-lafs added this to the 1.10.0 milestone 2012-10-26 02:45:03 +00:00

tahoe-lafs added

and removed

labels 2012-10-26 02:51:40 +00:00

warner commented

2012-12-20 17:03:03 +00:00

kicking to 1.11 until we get this reproduced with the new assertions

warner modified the milestone from 1.10.0 to 1.11.0

2012-12-20 17:03:03 +00:00

zooko commented

2013-05-09 21:44:50 +00:00

Could this be related to #1847?

daira commented

2013-05-11 00:23:10 +00:00

No, the proposed cleanup on #1847 does not affect the behaviour:

>>> class Foo(object):
...     DEP = {1:2}
...     def __init__(self, x):
...         self.DEP = self.DEP.copy()
...         self.DEP[x] = 42
...         print Foo.DEP, self.DEP
... 
>>> Foo(3)
{1: 2} {1: 2, 3: 42}
<__main__.Foo object at 0x7f638528bd10>
>>> Foo(1)
{1: 2} {1: 42}
<__main__.Foo object at 0x7f638528bb50>

as expected. That is, modifying self.DEP does not affect the shadowed Foo.DEP, and there's nothing else that the proposed change on #1847 would fix.

No, the proposed cleanup on #1847 does not affect the behaviour: ``` >>> class Foo(object): ... DEP = {1:2} ... def __init__(self, x): ... self.DEP = self.DEP.copy() ... self.DEP[x] = 42 ... print Foo.DEP, self.DEP ... >>> Foo(3) {1: 2} {1: 2, 3: 42} <__main__.Foo object at 0x7f638528bd10> >>> Foo(1) {1: 2} {1: 42} <__main__.Foo object at 0x7f638528bb50> ``` as expected. That is, modifying `self.DEP` does not affect the shadowed `Foo.DEP`, and there's nothing else that the proposed change on #1847 would fix.

markberger commented

2013-08-12 14:24:30 +00:00

I'm unable to reproduce this problem on trunk with a unit test. Here is the test I've written:

    def test_cli_ignores_happy(self):
        self.basedir = "cli/Check/cli_ignores_happy"
        self.set_up_grid(num_servers=4)
        c0 = self.g.clients[0]
        c0.DEFAULT_ENCODING_PARAMETERS["k"] = 2
        c0.DEFAULT_ENCODING_PARAMETERS["happy"] = 4
        c0.DEFAULT_ENCODING_PARAMETERS["n"] = 4
        data = upload.Data("data" * 10000, convergence="")
        d = c0.upload(data)
        def _setup(ur):
            self.uri = ur.get_uri()
            self.delete_shares_numbered(self.uri, [1])
        d.addCallback(_setup)
        d.addCallback(lambda ign: self.do_cli("check", "--repair", self.uri))
        def _check((rc, out, err)):
            self.failUnlessReallyEqual(err, "")
            self.failUnlessReallyEqual(rc, 0)
            lines = out.splitlines()
            self.failUnless("Summary: not healthy" in lines, out)
            self.failUnless(" good-shares: 3 (encoding is 2-of-4)" in lines, out)
        d.addCallback(_check)
        return d

I'm unable to reproduce this problem on trunk with a unit test. Here is the test I've written: ``` def test_cli_ignores_happy(self): self.basedir = "cli/Check/cli_ignores_happy" self.set_up_grid(num_servers=4) c0 = self.g.clients[0] c0.DEFAULT_ENCODING_PARAMETERS["k"] = 2 c0.DEFAULT_ENCODING_PARAMETERS["happy"] = 4 c0.DEFAULT_ENCODING_PARAMETERS["n"] = 4 data = upload.Data("data" * 10000, convergence="") d = c0.upload(data) def _setup(ur): self.uri = ur.get_uri() self.delete_shares_numbered(self.uri, [1]) d.addCallback(_setup) d.addCallback(lambda ign: self.do_cli("check", "--repair", self.uri)) def _check((rc, out, err)): self.failUnlessReallyEqual(err, "") self.failUnlessReallyEqual(rc, 0) lines = out.splitlines() self.failUnless("Summary: not healthy" in lines, out) self.failUnless(" good-shares: 3 (encoding is 2-of-4)" in lines, out) d.addCallback(_check) return d ```

tahoe-lafs added

major

and removed

critical

labels 2013-08-28 15:22:02 +00:00

tahoe-lafs modified the milestone from 1.11.0 to 1.12.0

2013-08-28 15:22:02 +00:00

tahoe-lafs modified the milestone from 1.12.0 to 1.11.0

2013-08-28 16:42:25 +00:00

markberger commented

2013-09-16 14:32:56 +00:00

After thinking about this a little bit, it might be better to refactor and remove all default constants in the source. That way this problem could not occur.

zooko commented

2013-09-16 14:49:27 +00:00

Replying to markberger:

After thinking about this a little bit, it might be better to refactor and remove all default constants in the source. That way this problem could not occur.

+1 ! That was my thinking in #1847.

Replying to [markberger](/tahoe-lafs/trac-2024-07-25/issues/1830#issuecomment-89861): > After thinking about this a little bit, it might be better to refactor and remove all default constants in the source. That way this problem could not occur. +1 ! That was my thinking in #1847.

warner commented

2014-09-23 17:25:54 +00:00

At the meeting today we decided to punt this into 1.12 . The suggested cleanup is a great idea (if it's not already cleaned up.. I thought we'd done a pass on this once already). The goal will be to have exactly one place where default k/h/N are specified, in client.py where the config file is read.

warner modified the milestone from 1.11.0 to 1.12.0

2014-09-23 17:25:54 +00:00

warner commented

2016-03-22 05:02:25 +00:00

Milestone renamed

warner modified the milestone from 1.12.0 to 1.13.0

2016-03-22 05:02:25 +00:00

warner commented

2016-06-28 18:17:14 +00:00

renaming milestone

warner modified the milestone from 1.13.0 to 1.14.0

2016-06-28 18:17:14 +00:00