stats: bump hot restart version given the data structure change.

[jmarantz]

Signed-off-by: Joshua Marantz jmarantz@google.com

Description:

In #3480 I changed some data structure sizes that affect
the shared memory layout, requiring a version bump. Here it is.

Risk Level: Low
Testing: //test/...
Docs Changes: N/A
Release Notes: N/A

[mattklein123]

@jmarantz would a better fix here (even if done as a follow up) be to include stats_set_size

envoy/source/server/hot_restart_impl.cc

Line 40 in 577ff2c

SharedMemory& SharedMemory::initialize(uint64_t stats_set_size, Options& options) {

)

in the hot restart version?

[mattklein123]

Though, I guess if we are going to do that I would rather do it now so we don't change the hot restart version twice...

[alyssawilk]

That and/or we could add a really stupid unit test which is hard-coded to current size with a note if the test is updated, the hot restart version must be updated too.

[mattklein123]

@alyssawilk yes +1 (let's do both!)

[alyssawilk]

FWIW I'd totally be fine landing this as-is with a (mental) TODO to increment and fix in a (fast) follow-up. My reasoning being if the longer fix would land tomorrow (given 30 minutes left to the EST day) any hot restarts done between now and then would be broken in a non-obvious way, where if we rev twice they'll at least fail in a well understood way. And if no one hot restarts between now and then, the double increment will do no harm.

[mattklein123]

Yeah, that's fine to to just get the fix in there now as long as @jmarantz is willing to do a quick follow up. The other option is to revert the original change and reapply it with a better fix?

[jmarantz]

Sorry for the delay; I've been going down the path suggested which makes sense to me, and ran into an integration test failure that requires me to page in some code from a few months ago. I'll follow up when I'm ready if you want to merge this as is.

[mattklein123]

@jmarantz I think my preference is to revert rather than do this fix and than another one if the other fix won't be ready today. Any objection to that? Would rather you not be rushed.

[jmarantz]

I should be able to get it ready today...this was just a question of 10s of minutes rather than 1s of minutes, I hope :)

[jmarantz]

I figured out the problem; static initialized data, the usual suspect :) In this case, the max object name length (which now affects the signature) was not being initialized in the path to HotRestartImpl::versionHelper.

Testing now.

[mattklein123]

Nice, thank you!

[jmarantz]

coverage test failure: OptionsImplTest.HotRestartTest failed because of static data not getting cleared out, now that the hot-restart-version callback configures stats by setting a static. static data makes me sad.

Fix coming (checking locally now).

[jmarantz]

Is this related to #3503? I assume that the earlier uint32_t/uint64_t change wasn't the trigger for that bug because those are both older versions, but maybe this also fixes it?

[jmarantz]

I've locally confirmed that all tests & coverage work. Hopefully this goes through.