Log spam generated when tree created but not initialized

[mhutchinson]

This is related to issue #1660.

If a tree is created but not initialized, the log signer sequencer spams the following error in a tight loop:

trillian-log-signer_1 | E0605 16:09:04.778580 1 operation_manager.go:432] ExecutePass(7743739687928485950) failed: failed to integrate batch for 7743739687928485950: 7743739687928485950: Sequencer failed to unmarshal latest root: logRootBytes too short

I had a quick look, but there's nothing in the trillian.Tree instance that indicates whether the tree is initialized or not. It feels like a created but not initialized tree should have a special tree_state (its state is ACTIVE, but this goes against the docs which say "Active trees are able to respond to both read and write requests.")

[Martin2112]

I don't know how we got into this. The original intention was that the create operation would initialize the tree. Instead we now have a lot of special case error code scattered around.

[AlCutter]

Martin, could you have a think about options to address the underlying issue?

[Martin2112]

Can do.

"not initialized" is also a bit of a misnomer as it just means there's no roots created for the tree. All that InitLog() does is sign a root, after some checks that none already exist.

[Martin2112]

I think the options are (in increasing order of frightfulness):

1. Make the sequencer ignore the "tree needs init error"
2. Ensure that all created trees have an initial root created
3. Add a new state that's not visible to apps until the root has been created, then bump to ACTIVE

I think either of the last two options mean we can remove the InitLog API call and all the special case logic to handle a tree that returns ErrTreeNeedsInit. Adding new states is possibly more intrusive IIRC from doing the FROZEN one.

[pav-kv]

4. Make the sequencer init the log when it encounters "tree needs init" error, and remove the API call.

[Martin2112]

That would be an improvement but could still result in code duplication e.g. in migrillian.

[pav-kv]

Migrillian doesn't create trees currently, it's done manually by the operator. But I agree that it would reappear in a bunch of places, e.g. in client lib.

[AlCutter]

I worry about the option of having the sequencer auto-init a tree; that seems outside of the responsibility of the sequencer - if the tree's not in a good state the sequencer should not attempt to modify it, if it's a bug/error triggering the error then we want the operator to intervene.

The underlying reason for why this is split is that there is no single storage-layer API which can transactionally both create a tree and write an STH.

So I guess either that has to change (and as a consequence we enforce that admin data is stored in the same place as tree data), or we acknowledge that there's an intermediate state and add that.

[pav-kv]

Having a special state seems the most canonical.

[pav-kv]

This seems closeable.