Reduce SessionTTLMin to 1 second

[Amit-PivotalLabs]

Reducing the minimum Session TTL.

We originally tried to make it configurable but you could accidentally have your agents and servers allow different values. We felt this was the better approach.

Signed-off-by: Michael Fraenkel [email protected]

[ryanuber]

Thanks for the PR. I think we are going to need this to be configurable though, with the defaults unchanged, as discussed in #796. The current 10s minimum promotes using session TTLs conservatively, and not overwhelming the leader with client heartbeats, which becomes possible by default with this change. We would also need some unit tests for the changed behavior as well.

[Amit-PivotalLabs]

We see the TTL validated in three places:

• agent: command/agent/session_endpoint.go
• server: consul/session_endpoint.go
• store: consul/state_store.go

Should it still validate in all three places? Also, assuming the server and store get the same configuration, it still needs to be configured in at least two places -- agent and server. What should happen if agent and server are configured differently?

What if we remove the validation from the agent, configure in the server, and ensure the config is shared between server and store?

[Amit-PivotalLabs]

@ryanuber Any feedback about the previous comment?

[ryanuber]

@Amit-PivotalLabs sorry about the delay, I've been gone for a few days. The agent configuration should be automatically copied into the server config at start time, so there shouldn't be any disconnect there.

I think it's probably fine to remove the validation from the agent, since the server does the same exact check during the next RPC call, and returns the same error. Invalid requests shouldn't be repeated, so I think this is sane.

Really it seems like this should be checked in one place (the session create code path). I'm not entirely sure why it is re-checked in the state store, and what the implications of removing that would be if servers are configured with different values. @armon any ideas on this?

[armon]

@ryanuber @Amit-PivotalLabs Primarily the check was done in a few places for defense in depth. I think it's probably fine to leave just a single check for min/max values in the session create path on the server. The state store checks can probably be reduces to a very basic sanity check instead of bounds check.

[ryanuber]

Fixed in #821.