/juju

state: pass in mgo.Session when opening State #8056

Merged
merged 5 commits into from Nov 14, 2017

Conversation

Reviewers

@wallyworld wallyworld

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@axw @jujubot @nskaggs @wallyworld
@axw
Member

axw commented Nov 13, 2017

Description of change

  • update state openers (Open, OpenController, Initialize) to accept an mgo.Session, rather than dialling from those methods. The session is copied, so the caller retains ownership of the session it passes in.
  • remove the State.MongoConnectionInfo and State.CACert methods, as they were relying on recording mongo connection info on the State object

QA steps

Smoke test: bootstrap, destroy.

Documentation changes

None.

Bug reference

None.
@axw
Member

axw commented Nov 13, 2017

Move Mongo dialling outside of state
apiserver/common/addresses.go
@@ -15,7 +17,7 @@ import (
// controller addresses and the CA public certificate.
type AddressAndCertGetter interface {
@wallyworld

wallyworld Nov 13, 2017

Owner

This interface name is now a bit inaccurate

@axw

axw Nov 14, 2017

Member

I've renamed it, although the types in this file aren't very well scoped anyway. CA certs have bugger all to do with addresses, and what's the ModelUUID doing in there? Anyway.

apiserver/facades/client/applicationoffers/mock_test.go
@@ -492,8 +493,8 @@ func (m *mockState) APIHostPorts() ([][]network.HostPort, error) {
}, nil
}
-func (m *mockState) CACert() string {
- return testing.CACert
+func (m *mockState) ControllerConfig() (controller.Config, error) {
@wallyworld

wallyworld Nov 13, 2017

Owner

Is this used anywhere?

@axw

axw Nov 14, 2017

Member

It was used for the backend of the APIAddresser. I've updated the PR with a commit to remove CACert from that interface.

apiserver/facades/client/client/instanceconfig.go
+ }
+ caCert, ok := controllerConfig.CACert()
+ if !ok {
+ return nil, errors.New("CA certificate missing from controller config")
@wallyworld

wallyworld Nov 13, 2017

Owner

Let's define NoCACertErr or something instead of the same string in several places.
Perhaps CACert() method itself on controllerConfig should return an error?

@axw

axw Nov 14, 2017

Member

Ideally it would return neither a bool nor an error, since it's not possible to have controller config without a CA certificate past bootstrap. I'm ignoring the bool now, will add a card to change the controller.Config method.

apiserver/root.go
@@ -79,6 +79,7 @@ func newAPIHandler(srv *Server, st *state.State, rpcConn *rpc.Conn, modelUUID st
modelUUID: modelUUID,
serverHost: serverHost,
}
+
@wallyworld

wallyworld Nov 13, 2017

Owner

do we need these?

@axw

axw Nov 14, 2017

Member

need what? whitespace? accidentally snuck in as I added a resource, then removed it again - reverted

state/initialize.go
- MongoInfo *mongo.MongoInfo
+ // MongoSession is the mgo.Session to use for storing and
+ // accessing state data. The caller remains responsible
+ // for closing the session.
@wallyworld

wallyworld Nov 13, 2017

Owner

for closing this session ??

(to distinguish from the copy that is made)

@axw

axw Nov 14, 2017

Member

done

state/open.go
- MongoDialOpts mongo.DialOpts
+ // MongoSession is the mgo.Session to use for storing and
+ // accessing state data. The caller remains responsible
+ // for closing the session; Open will copy it.
@wallyworld

wallyworld Nov 13, 2017

Owner

s/the/the passed in session?

Not sure if that clarification is needed? Makes sense to make the change to me anyway. YMMV

@axw

axw Nov 14, 2017

Member

s/the/this/ like in the other case

state/state.go
+ } `bson:"authenticatedUsers"`
+ } `bson:"authInfo"`
+ }
+ if err := st.session.DB(jujuDB).Run(bson.D{{"connectionStatus", 1}}, &connectionStatus); err != nil {
@wallyworld

wallyworld Nov 13, 2017

Owner

Can you add a comment here? I don't quite grok this at first read.

@axw

axw Nov 14, 2017

Member

yup - sorry, done

axw added some commits Nov 12, 2017

@axw
state: remove State.MongoConnectionInfo method 
We shouldn't need to know about how to connect
to MongoDB given a State object, we just need
a connection.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
1957a16
@axw
state: remove State.CACert method 
This is a property of controller config, so
use that where necessary, rather than relying
on the CACert value in the mongo info.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
f347fbb
@axw
state: stop storing MongoInfo 
No longer needed, so stop storing it.
Next step is to stop passing it into
State altogether, and instead just
pass in a mgo.Session.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
5ad3d78
@axw
state: pass mgo.Session into openers 
Functions in the State package that create
State objects now accept an mgo.Session,
rather than the mongo connection info and
dial options. For now at least, they copy
the session.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
3f69037
@axw
Member

axw commented Nov 14, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Nov 14, 2017

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju
@jujubot
Contributor

jujubot commented Nov 14, 2017

Build failed: Tests failed
build url: http://ci.jujucharms.com/job/github-merge-juju/505
@axw
apiserver/common: remove CACert from APIAddresser 
Remove CACert from the APIAddresser type, which
is embedded and exposed by various facades. Add
CACert as a method directly to those facades
that require it. Also remove CACert method from
client-side APIAddresser type, and move to clients
that require it.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
67aba93
@axw
Member

axw commented Nov 14, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Nov 14, 2017

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju
@jujubot
Contributor

jujubot commented Nov 14, 2017

Build failed: Tests failed
build url: http://ci.jujucharms.com/job/github-merge-juju/508
@axw
Member

axw commented Nov 14, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Nov 14, 2017

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju
@axw
Member

axw commented Nov 14, 2017

Build failed: it's a mystery
@axw
Member

axw commented Nov 14, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Nov 14, 2017

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju
@nskaggs
Owner

nskaggs commented Nov 14, 2017

$$merge$$

@jujubot jujubot merged commit eff693a into juju:state-controller-refactor Nov 14, 2017
1 check passed

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment