#1830 new defect

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

Reported by: davidsarah Owned by: kmarkley86
Priority: major Milestone: 1.15.0
Component: code-encoding Version: 1.8.1
Keywords: regression upload servers-of-happiness Cc:
Launchpad Bug:

Description (last modified by zooko)

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?

Change History (14)

comment:1 Changed at 2012-10-26T02:46:02Z by davidsarah

  • Description modified (diff)

comment:2 Changed at 2012-10-26T02:51:40Z by davidsarah

  • Component changed from unknown to code-encoding
  • Version changed from 1.9.2 to 1.8.1

comment:3 Changed at 2012-12-20T17:03:03Z by warner

  • Milestone changed from 1.10.0 to 1.11.0

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

comment:4 Changed at 2013-05-09T21:44:50Z by zooko

  • Description modified (diff)

Could this be related to #1847?

comment:5 Changed at 2013-05-11T00:23:10Z by 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.

comment:6 Changed at 2013-08-12T14:24:30Z by 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
Last edited at 2013-08-12T14:33:30Z by markberger (previous) (diff)

comment:7 Changed at 2013-08-28T15:22:02Z by daira

  • Milestone changed from 1.11.0 to 1.12.0
  • Priority changed from critical to major

comment:8 Changed at 2013-08-28T16:42:25Z by daira

  • Milestone changed from 1.12.0 to 1.11.0

comment:9 follow-up: Changed at 2013-09-16T14:32:56Z by 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.

comment:10 in reply to: ↑ 9 Changed at 2013-09-16T14:49:27Z by 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.

comment:11 Changed at 2014-09-23T17:25:54Z by warner

  • Milestone changed from 1.11.0 to 1.12.0

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.

comment:12 Changed at 2016-03-22T05:02:25Z by warner

  • Milestone changed from 1.12.0 to 1.13.0

Milestone renamed

comment:13 Changed at 2016-06-28T18:17:14Z by warner

  • Milestone changed from 1.13.0 to 1.14.0

renaming milestone

comment:14 Changed at 2020-06-30T14:45:13Z by exarkun

  • Milestone changed from 1.14.0 to 1.15.0

Moving open issues out of closed milestones.

Note: See TracTickets for help on using tickets.