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

[davidsarah]

kmarkley86 at 1212#comment:41:

I'm affected by the same fundamental problem [as #1212], 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 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 ticket:1212#comment:41, but it might make it easier to reproduce. refs #1212

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

[warner]

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

[zooko]

Could this be related to #1847?

[daira]

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]

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

[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.

[zooko]

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.

[warner]

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]

Milestone renamed

[warner]

renaming milestone

[exarkun]

Moving open issues out of closed milestones.

[meejah]

Ticket retargeted after milestone closed