use simpler error code checking for upgrade in progress #5954

Merged
merged 1 commit into from Aug 9, 2016

Conversation

Projects
None yet
3 participants
Owner

rogpeppe commented Aug 9, 2016

This comment is inaccurate.

This is necessary as the error type returned from a facade
call is rpc.RequestError and we cannot use params.IsCodeUpgradeInProgress

In face, IsCodeUpgradeInProgress works just fine on rpc.RequestError,
so we change the code to use it and avoid the dubious DeepEquals
calls on errors.

Also remove some no-longer-used code from the apiserver package.
and fix a test in the apiserver package that could sometimes take 5 minutes
to run (it now takes a maximum of 5 seconds).

(Review request: http://reviews.vapour.ws/r/5398/)

@@ -67,7 +58,7 @@ func prepareRestore(newClient ClientConnection) error {
if err == nil && remoteError == nil {
return nil
}
- if !isUpgradeInProgressErr(err) || remoteError != nil {
+ if !params.IsCodeUpgradeInProgress(err) || remoteError != nil {
@frankban

frankban Aug 9, 2016

Member

Good simplification!

@@ -520,7 +520,7 @@ func (s *loginSuite) TestLoginValidationDuringUpgrade(c *gc.C) {
c.Assert(err, jc.ErrorIsNil)
err = st.APICall("Client", 1, "", "ModelSet", params.ModelSet{}, nil)
- c.Assert(errors.Cause(err), gc.DeepEquals, &rpc.RequestError{Message: params.CodeUpgradeInProgress, Code: params.CodeUpgradeInProgress})
+ c.Assert(err, jc.Satisfies, params.IsCodeUpgradeInProgress)
@frankban

frankban Aug 9, 2016

Member

Better.

@@ -191,7 +192,11 @@ func (s *serverSuite) TestNewServerDoesNotAccessState(c *gc.C) {
proxy := testing.NewTCPProxy(c, mongoInfo.Addrs[0])
mongoInfo.Addrs = []string{proxy.Addr()}
- st, err := state.Open(s.State.ModelTag(), mongoInfo, mongotest.DialOpts(), nil)
+ dialOpts := mongo.DialOpts{
@frankban

frankban Aug 9, 2016

Member

So, what's in mongotest that caused the discrepancies in test run time?

@rogpeppe

rogpeppe Aug 9, 2016

Owner

The timeout in mongotest is 5 minutes. This test deliberately shuts down the mongodb connection. Because the state package calls mgo.Session.Copy before it does any db operation and that causes mgo to reconnect automatically (subject to the timeout) and closing State synchronously terminates its workers, if the worker happens to be executing a db operation, it can take as long as the timeout before it responds to the termination request, thus blocking the State.Close method for that long - i.e. 5 minutes.

Member

frankban commented Aug 9, 2016

👍

use simpler error code checking
This comment is inaccurate.

> This is necessary as the error type returned from a facade
> call is rpc.RequestError and we cannot use params.IsCodeUpgradeInProgress

In face, IsCodeUpgradeInProgress works just fine on rpc.RequestError,
so we change the code to use it and avoid the dubious DeepEquals
calls on errors.

Also remove some no-longer-used code from the apiserver package.
Owner

rogpeppe commented Aug 9, 2016

$$merge$$

Contributor

jujubot commented Aug 9, 2016

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

@jujubot jujubot merged commit c03a602 into juju:master Aug 9, 2016

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