Fixed lp:1635464: infinite recursion in worker/peergroper #6495

Merged
merged 1 commit into from Oct 25, 2016

Conversation

Projects
None yet
4 participants
Contributor

dimitern commented Oct 25, 2016

When validating the "mongo" space name (common among all controllers),
the way stateShim.Space() was implemented in worker/peergrouper/shim.go
caused infinite recursion when called. This fixes http://pad.lv/1635464.

While I couldn't reproduce this normally, it's easy to trigger the case
when state.ControllerInfo.MongoSpaceState is "valid" by modifying the
mongo controllers collection directly.

QA steps (if you do those same steps without the fix, the bug happens):

  1. juju bootstrap vmaas-21 m21
  2. juju switch controller
  3. juju ssh 0
  4. ubuntu@maas-21-node-0:~$ sudo grep 'fatal error: stack overflow' /var/log/juju/machine-0.log

    verify no matches are found initially

  5. ubuntu@maas-21-node-0:~$ export STATE_PASSWORD=$(sudo grep statepassword: /var/lib/juju/agents/machine-0/agent.conf | cut -b16-)
  6. ubuntu@maas-21-node-0:~$ sudo /usr/lib/juju/mongo3.2/bin/mongo 127.0.0.1:37017/admin --ssl --sslAllowInvalidCertificates --sslPEMKeyPassword /var/lib/juju/shared-secret --sslPEMKeyFile /var/lib/juju/server.pem -u machine-0 -p $STATE_PASSWORD
  7. Run the following commands in the mongo shell:
    juju:PRIMARY> use juju;
    juju:PRIMARY> db.controllers.update({_id:"e"}, {"$set": {"mongo-space-name": "space-0", "mongo-space-state": "valid"}})

    output: WriteResult({ "nMatched" : 1, "nUpserted" : 0, "nModified" : 1 })

    juju:PRIMARY> exit;
  8. ubuntu@maas-21-node-0:~$ sudo grep 'selecting mongo peer hostPort in space space-0 from' /var/log/juju/machine-0.log

    verify the peergrouper picked up the change (>1 matches expected)

  9. ubuntu@maas-21-node-0:~$ sudo grep 'fatal error: stack overflow' /var/log/juju/machine-0.log

    verify the bug does not occur after the changes

Looks good, and the method is defined (even if State is embedded) for testing/stubbing purposes I guess. Thank you!

I am worried that this change touches no tests, even thiugh it is impractical to test for coding errors it is mote worthy that there seems to be no test exercising this path in a way that triggers the previous error.

Contributor

dimitern commented Oct 25, 2016

$$fixes-1635464$$

Contributor

jujubot commented Oct 25, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Oct 25, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9554

Contributor

dimitern commented Oct 25, 2016

$$again$$

Contributor

jujubot commented Oct 25, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 5983ff2 into juju:develop Oct 25, 2016

1 check passed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy.
Details

@dimitern dimitern deleted the dimitern:lp-1635464-peergrouper-infrec branch Oct 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment