Status history pruning age and size is configurable #7239

Merged
merged 1 commit into from Apr 19, 2017

Conversation

Projects
None yet
3 participants
Owner

wallyworld commented Apr 13, 2017

Description of change

We add 2 new model config parameters

  • max-status-history-age
  • max-status-history-size

These control how status history records are removed during pruning; the age and the max size of the collection holding the records.

The status history pruning worker manifold is also improved, plus tests are added.

There are also fixes to tests for the previously landed code to control log pruning.

Still todo - the pruning worker does not react to changes in the pruning config.

QA steps

bootstrap
juju model-config, verify that default status history pruning params are shown

bootstrap with values set via --config
juju model-config, verify that the specified status history pruning params are shown

bootstrap beta2
upgrade
juju model-config, verify that default status history pruning params are shown

Documentation changes

There are 2 new model config attributes:

  • max-status-history-age
    "The maximum age for status history entries before they are pruned, in human-readable time format"

  • max-status-history-size
    "The maximum size for the status history collection, in human-readable memory format"

Bug reference

http://pad.lv/1681352

Looks good overall, just a few things.

environs/config/config.go
@@ -345,6 +354,10 @@ var defaultConfigValues = map[string]interface{}{
AptFTPProxyKey: "",
AptNoProxyKey: "127.0.0.1,localhost,::1",
"apt-mirror": "",
+
+ // Status history settings
+ MaxStatusHistoryAge: "336h", // 2 weeks
@axw

axw Apr 18, 2017

Member

make these constants, and then you can use them in the upgrade code

environs/config/config.go
+
+// MaxStatusHisotrySizeMB is the maximum size in MiB which the status history
+// collection can grow to before being pruned.
+func (c *Config) MaxStatusHisotrySizeMB() uint {
@axw

axw Apr 18, 2017

Member

s/Hisotry/History/

@wallyworld

wallyworld Apr 18, 2017

Owner

doh, fixed

state/upgrades.go
+
+ settingsChanged := false
+
+ iter := coll.Find(bson.M{"_id": bson.M{"$regex": "^.*:e$"}}).Iter()
@axw

axw Apr 18, 2017

Member

This feels like it'd be a bit too easy to stomp on unrelated settings docs. i.e., we might introduce some settings key with a prefix like model-uuid:foo:e, where e is some key that has nothing to do with environments.

Can we please use AllModels to iterate models, and then look up the settings docs for those? I think you could then refactor and share some code between AddControllerLogPruneSettings and AddStatusHistoryPruneSettings: call some common function with a collection, doc ID, and a function with the signature:

func(*settingsDoc) (changed bool)
@wallyworld

wallyworld Apr 18, 2017

Owner

I don't see this as a possibility really, but changes made

+ if err := context.Get(config.EnvironName, &environ); err != nil {
+ return nil, errors.Trace(err)
+ }
+ cfg := environ.Config()
@axw

axw Apr 18, 2017

Member

can you please put a TODO here to react to config changes?

@wallyworld

wallyworld Apr 18, 2017

Owner

I have a PR immediately ready to land after this one which addresses the issue. The code here will disappear.

axw approved these changes Apr 18, 2017

Owner

wallyworld commented Apr 18, 2017

$$merge$$

Contributor

jujubot commented Apr 18, 2017

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

Contributor

jujubot commented Apr 19, 2017

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

Owner

wallyworld commented Apr 19, 2017

$$merge$$

Contributor

jujubot commented Apr 19, 2017

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

Contributor

jujubot commented Apr 19, 2017

Build failed: Generating tarball failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10686

Owner

wallyworld commented Apr 19, 2017

$$merge$$

Contributor

jujubot commented Apr 19, 2017

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

@jujubot jujubot merged commit 1e831a0 into juju:develop Apr 19, 2017

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