TS-2519: handle stat persistence type changes When Traffic Server is upgraded, it is possible for stats to change their persistence type. If it changes from persistent to non-persistent, we don't want to preserve the previous value. Detect this case when we load the records snapshot and also when we register stats.
Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/e2ffb69c Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/e2ffb69c Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/e2ffb69c Branch: refs/heads/5.0.x Commit: e2ffb69c179c7daf65e819b29c90ebde4546b94e Parents: 7eeb5c7 Author: James Peach <[email protected]> Authored: Thu Jan 23 16:21:25 2014 -0800 Committer: James Peach <[email protected]> Committed: Mon Jan 27 09:40:13 2014 -0800 ---------------------------------------------------------------------- lib/records/I_RecCore.h | 1 + lib/records/P_RecCore.cc | 30 ++++++++++++++++++++++++-- lib/records/RecCore.cc | 49 ++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 75 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e2ffb69c/lib/records/I_RecCore.h ---------------------------------------------------------------------- diff --git a/lib/records/I_RecCore.h b/lib/records/I_RecCore.h index e9b1a4f..5bc1358 100644 --- a/lib/records/I_RecCore.h +++ b/lib/records/I_RecCore.h @@ -151,6 +151,7 @@ int RecGetRecordByte(const char *name, RecByte * rec_byte, bool lock = true); //------------------------------------------------------------------------ int RecGetRecordType(const char *name, RecT * rec_type, bool lock = true); int RecGetRecordDataType(const char *name, RecDataT * data_type, bool lock = true); +int RecGetRecordPersistenceType(const char *name, RecPersistT * persist_type, bool lock = true); int RecGetRecordUpdateCount(RecT data_type); int RecGetRecordOrderAndId(const char *name, int *order, int *id, bool lock = true); http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e2ffb69c/lib/records/P_RecCore.cc ---------------------------------------------------------------------- diff --git a/lib/records/P_RecCore.cc b/lib/records/P_RecCore.cc index 152bf05..b90a5d3 100644 --- a/lib/records/P_RecCore.cc +++ b/lib/records/P_RecCore.cc @@ -531,6 +531,7 @@ RecReadStatsFile() RecRecord *r; RecMessage *m; RecMessageItr itr; + RecPersistT persist_type = RECP_NULL; xptr<char> snap_fpath(RecConfigReadPersistentStatsPath()); // lock our hash table @@ -539,8 +540,33 @@ RecReadStatsFile() if ((m = RecMessageReadFromDisk(snap_fpath)) != NULL) { if (RecMessageUnmarshalFirst(m, &itr, &r) != REC_ERR_FAIL) { do { - if ((r->name == NULL) || (!strlen(r->name))) + if ((r->name == NULL) || (!strlen(r->name))) { continue; + } + + // If we don't have a persistence type for this record, it means that it is not a stat, or it is + // not registered yet. Either way, it's ok to just set the persisted value and keep going. + if (RecGetRecordPersistenceType(r->name, &persist_type, false /* lock */) != REC_ERR_OKAY) { + RecDebug(DL_Debug, "restoring value for persisted stat '%s'", r->name); + RecSetRecord(r->rec_type, r->name, r->data_type, &(r->data), &(r->stat_meta.data_raw), false); + continue; + } + + if (!REC_TYPE_IS_STAT(r->rec_type)) { + // This should not happen, but be defensive against records changing their type .. + RecLog(DL_Warning, "skipping restore of non-stat record '%s'", r->name); + continue; + } + + // Check whether the persistence type was changed by a new software version. If the record is + // already registered with an updated persistence type, then we don't want to set it. We should + // keep the registered value. + if (persist_type == RECP_NON_PERSISTENT) { + RecDebug(DL_Debug, "preserving current value of formerly persistent stat '%s'", r->name); + continue; + } + + RecDebug(DL_Debug, "restoring value for persisted stat '%s'", r->name); RecSetRecord(r->rec_type, r->name, r->data_type, &(r->data), &(r->stat_meta.data_raw), false); } while (RecMessageUnmarshalNext(m, &itr, &r) != REC_ERR_FAIL); } @@ -579,7 +605,7 @@ RecSyncStatsFile() r = &(g_records[i]); rec_mutex_acquire(&(r->lock)); if (REC_TYPE_IS_STAT(r->rec_type)) { - if (r->stat_meta.persist_type != RECP_NON_PERSISTENT) { + if (r->stat_meta.persist_type == RECP_PERSISTENT) { m = RecMessageMarshal_Realloc(m, r); sync_to_disk = true; } http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e2ffb69c/lib/records/RecCore.cc ---------------------------------------------------------------------- diff --git a/lib/records/RecCore.cc b/lib/records/RecCore.cc index 484a9c6..1ed4e10 100644 --- a/lib/records/RecCore.cc +++ b/lib/records/RecCore.cc @@ -44,7 +44,7 @@ RecTree *g_records_tree = NULL; // register_record //------------------------------------------------------------------------- static RecRecord * -register_record(RecT rec_type, const char *name, RecDataT data_type, RecData data_default) +register_record(RecT rec_type, const char *name, RecDataT data_type, RecData data_default, RecPersistT persist_type) { RecRecord *r = NULL; @@ -61,6 +61,10 @@ register_record(RecT rec_type, const char *name, RecDataT data_type, RecData dat RecDataSet(r->data_type, &(r->data), &(data_default)); RecDataSet(r->data_type, &(r->data_default), &(data_default)); ink_hash_table_insert(g_records_ht, name, (void *) r); + + if (REC_TYPE_IS_STAT(r->rec_type)) { + r->stat_meta.persist_type = persist_type; + } } // we're now registered @@ -467,6 +471,34 @@ RecGetRecordDataType(const char *name, RecDataT * data_type, bool lock) } int +RecGetRecordPersistenceType(const char *name, RecPersistT * persist_type, bool lock) +{ + int err = REC_ERR_FAIL; + RecRecord *r = NULL; + + if (lock) { + ink_rwlock_rdlock(&g_records_rwlock); + } + + *persist_type = RECP_NULL; + + if (ink_hash_table_lookup(g_records_ht, name, (void **) &r)) { + rec_mutex_acquire(&(r->lock)); + if (REC_TYPE_IS_STAT(r->rec_type)) { + *persist_type = r->stat_meta.persist_type; + err = REC_ERR_OKAY; + } + rec_mutex_release(&(r->lock)); + } + + if (lock) { + ink_rwlock_unlock(&g_records_rwlock); + } + + return err; +} + +int RecGetRecordUpdateCount(RecT data_type) { return g_num_update[data_type]; @@ -690,10 +722,21 @@ RecRegisterStat(RecT rec_type, const char *name, RecDataT data_type, RecData dat RecRecord *r = NULL; ink_rwlock_wrlock(&g_records_rwlock); - if ((r = register_record(rec_type, name, data_type, data_default)) != NULL) { + if ((r = register_record(rec_type, name, data_type, data_default, persist_type)) != NULL) { + // If the persistence type we found in the records hash is not the same as the persistence + // type we are registering, then that means that it changed between the previous software + // version and the current version. If the metric changed to non-persistent, reset to the + // new default value. + if ((r->stat_meta.persist_type == RECP_NULL ||r->stat_meta.persist_type == RECP_PERSISTENT) && + persist_type == RECP_NON_PERSISTENT) { + RecDebug(DL_Debug, "resetting default value for formerly persisted stat '%s'", r->name); + RecDataSet(r->data_type, &(r->data), &(data_default)); + } + r->stat_meta.persist_type = persist_type; } else { ink_assert(!"Can't register record!"); + RecDebug(DL_Warning, "failed to register '%s' record", name); } ink_rwlock_unlock(&g_records_rwlock); @@ -711,7 +754,7 @@ RecRegisterConfig(RecT rec_type, const char *name, RecDataT data_type, { RecRecord *r; ink_rwlock_wrlock(&g_records_rwlock); - if ((r = register_record(rec_type, name, data_type, data_default)) != NULL) { + if ((r = register_record(rec_type, name, data_type, data_default, RECP_NULL)) != NULL) { // Note: do not modify 'record->config_meta.update_required' r->config_meta.update_type = update_type; r->config_meta.check_type = check_type;
