/juju

state,apiserver, cmd/status: Add MeterStatus to the model #7120

Merged
merged 3 commits into from Mar 22, 2017

Conversation

Reviewers

@alesstimec alesstimec

@cmars cmars

@perrito666 perrito666

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@mattyw @nskaggs @cmars @jujubot @perrito666 @alesstimec
@mattyw
Member

mattyw commented Mar 17, 2017

Description of change

With SLAs operating at the conceptual level of models each model now has the concept of a meter status, this is the same concept as the existing unit meter status.

QA steps

juju bootstrap
juju status # No meter status should be reported
# Do something to make the model meter status go red
juju status
# Output should look something like http://paste.ubuntu.com/24196065/

Documentation changes

How users react to meter status on models needs to be documented. This feature is just one of the features under the umbrella of SLAs

Bug reference

n/a
@mattyw
apiserver/metricsender, state/meterstatus: Response from metrics send… 
…er server will respond with a meter status. Here we add code to accept that response and set it within the model
f2dcd27
@perrito666

perrito666 approved these changes Mar 17, 2017
View changes

LGTM but with a small change requested.

state/state.go
+ return model.SetMeterStatus(status, info)
+}
+
+func (st *State) ModelMeterStatus() (MeterStatus, error) {
@perrito666

perrito666 Mar 17, 2017

Contributor

This method and the previous one are exported but not commented

@mattyw
Member

mattyw commented Mar 20, 2017

!!test!!
@mattyw
Member

mattyw commented Mar 20, 2017

!!test!!

@mattyw mattyw referenced this pull request Mar 20, 2017

Merged

state: Model migration for sla and meter status #7128

@nskaggs
Owner

nskaggs commented Mar 20, 2017

!!build!!
@cmars

cmars requested changes Mar 20, 2017
View changes

state/Unit.GetMeterStatus needs to combine mmStatus, the unit's containing model's meter status, and the unit's meter status.

state/meterstatus.go
default:
- return errors.Errorf("invalid meter status %q", code)
+ return MeterNotAvailable, errors.Errorf("invalid meter status %q", codeStr)
@cmars

cmars Mar 20, 2017

Owner

errors.NotValidf("meter status %q", codeStr) ?

state/upgrades_test.go
@@ -100,7 +100,7 @@ func (s *upgradesSuite) TestStripLocalUserDomainModels(c *gc.C) {
delete(initialModel, "txn-revno")
initialModel["owner"] = "test-admin"
- // TODO (mattyw, cmars) We need to confirm that setting sla in this test
+ // TODO (mattyw, cmars) We need to confirm that setting sla and meter-status in this test
@cmars

cmars Mar 20, 2017

Owner

What is the scenario this test attempts to recreate w.r.t SLAs? Upgrading models from a pre-SLA version of Juju?

@mattyw

mattyw Mar 21, 2017

Member

@cmars no upgrade is required for SLAs or meter status. This test is actually checking completely seperate behaviour (local domains like "@Local" being stripped from user names. However it includes checking against a bson representation of the model, which now includes sla and meter status

@cmars

cmars Mar 21, 2017

Owner

@mattyw Thanks for the explanation. I think we can remove this TODO then as this test is unrelated and we're just testing for consistent model migration behavior.

@cmars
Owner

cmars commented Mar 20, 2017

Also need to consider the case where state has an empty model meter status doc, with an empty string for the status code, if that is the possible after migrating a pre-SLA model. Does that case give an unexpected error when the client apiserver tries to query model meter status?
@mattyw
Member

mattyw commented Mar 21, 2017

@cmars in the case where meter status is queried but non is set we return "NOT AVAILABLE" which is what we do for the unit meter status as well. This isn't bubbled up in juju status as it filters it out.

@cmars regarding combining model & unit meter status. This was being done server side as I understand it
@mattyw
Member

mattyw commented Mar 21, 2017

!!test!!
@mattyw
Member

mattyw commented Mar 21, 2017

!!build!!
apiserver/common/modeldestroy.go
+ ModelManagerBackend
+}
+
+func (m modelBackendShim) ForModel(tag names.ModelTag) (metricsender.ModelBackend, error) {
@alesstimec

alesstimec Mar 21, 2017

Member

godoc

apiserver/metricsmanager/metricsmanager.go
+ *state.State
+}
+
+func (m modelBackendShim) ForModel(tag names.ModelTag) (metricsender.ModelBackend, error) {
@alesstimec

alesstimec Mar 21, 2017

Member

godoc

@mattyw
Member

mattyw commented Mar 21, 2017

!!build!!
@cmars
Owner

cmars commented Mar 21, 2017

@mattyw this one currently has compile errors:

# github.com/juju/juju/apiserver/metricsmanager
apiserver/metricsmanager/metricsmanager.go:143: api.state.SLACredential undefined (type metricsender.ModelBackend has no field or method SLACredential)
apiserver/metricsmanager/metricsmanager.go:150: api.state.AllMachines undefined (type metricsender.ModelBackend has no field or method AllMachines)
apiserver/metricsmanager/metricsmanager.go:166: api.state.AddModelMetrics undefined (type metricsender.ModelBackend has no field or method AddModelMetrics)
@cmars
Owner

cmars commented Mar 21, 2017

!!build!!
@mattyw
Member

mattyw commented Mar 21, 2017

!!test!!
@mattyw
Member

mattyw commented Mar 21, 2017

!!build!!
@nskaggs
Owner

nskaggs commented Mar 21, 2017

!!build!!
@nskaggs
Owner

nskaggs commented Mar 21, 2017

add to whitelist
@mattyw
Member

mattyw commented Mar 21, 2017

!!test!!
@cmars

cmars approved these changes Mar 21, 2017
View changes

Still LGTM (even better now!)

@mattyw
Member

mattyw commented Mar 21, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Mar 21, 2017

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

jujubot commented Mar 21, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10513
@cmars
Owner

cmars commented Mar 21, 2017

@mattyw Test failure is legit:

client_test.go:417:
    c.Assert(err, gc.ErrorMatches, test.err)
... error string = "meter status \"NOT AVAILABLE\" not valid"
... regex string = "invalid meter status \"NOT AVAILABLE\""
@mattyw
Member

mattyw commented Mar 21, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Mar 21, 2017

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

jujubot commented Mar 22, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10514
@mattyw
Member

mattyw commented Mar 22, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Mar 22, 2017

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

@jujubot jujubot merged commit 4f95247 into juju:rising-sun Mar 22, 2017

@mattyw mattyw deleted the mattyw:54-model-meter-status branch Mar 22, 2017

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