Use active tree on server startup

[lkatalin]

Summary

• Fixes logic in createAndInitTree() to use in order of precedence: 1. the ActiveIndex tree passed in via the logRanges flag, 2. the most recently created tree, 3. create a new tree
• Adds safeguards for ActiveTreeID() in case the range is empty and adds a unit test

Create a new tree if there is no active tree specified with the tlog_id flag. Do not look for existing trees to use because these may be inactive.

Ticket Link

Fixes #726

[codecov-commenter]

Codecov Report

Merging #727 (2b7a2d6) into main (befbcc0) will decrease coverage by 0.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #727      +/-   ##
==========================================
- Coverage   48.43%   48.35%   -0.09%     
==========================================
  Files          61       61              
  Lines        5492     5493       +1     
==========================================
- Hits         2660     2656       -4     
- Misses       2544     2547       +3     
- Partials      288      290       +2     

Impacted Files	Coverage Δ
pkg/sharding/ranges.go	41.81% <0.00%> (-0.78%)	⬇️
pkg/types/rpm/v0.0.1/entry.go	60.07% <0.00%> (-2.20%)	⬇️
pkg/types/rekord/v0.0.1/entry.go	51.51% <0.00%> (+0.60%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update befbcc0...2b7a2d6. Read the comment docs.

[lkatalin]

Rebased and ready for review!
@dlorenc @bobcallaway @priyawadhwa @lukehinds

[priyawadhwa]

I wonder if instead of searching for the most recent tree, we should just enforce people use the trillian_log_server.tlog_id flag to specify the active tree.

So if someone passes in:

• ranges and tlog_id --> use tlog_id as active log
• only ranges --> error
• only tlog_id --> use tlog_id as active log
• neither ranges nor tlog_id --> create a new log

This way we don't have to guess which tree is active. WDYT?

[dlorenc]

+1 on keeping the logic simpler

[lkatalin]

I wonder if instead of searching for the most recent tree, we should just enforce people use the trillian_log_server.tlog_id flag to specify the active tree.

So if someone passes in:

* `ranges` and `tlog_id` --> use `tlog_id` as active log

* only `ranges` --> error

* only `tlog_id` --> use `tlog_id` as active log

* neither `ranges` nor `tlog_id` --> create a new log

This way we don't have to guess which tree is active. WDYT?

Thanks @priyawadhwa , I was hoping to get some thoughts on how the tlog_id and ranges should relate to each other. I'll push a change to this that will implement the logic above.

[lkatalin]

@priyawadhwa In thinking about this more, I'm not sure that starting the server without the ranges or tlog_id flags should automatically start a new log - you might end up starting a log accidentally just by forgetting to pass in the flags. I want to remove the "look for any existing tree" logic from createAndInitTree() because once you have more than one shard, this will always return the wrong (inactive) one (assuming we don't implement my reverse-search for the latest tree).

The other thing I'm thinking about is that since ranges is a superset of the same info as tlog_id, does it make sense to error on passing in only ranges? We could grab the ranges.ActiveTreeID() and use that in the same way as tlog_id to determine whether to create a tree.

I realized I also need to add in a guard to make sure we're not appending the active tree to ranges twice in the case that both tlog_id and ranges are passed in.

[priyawadhwa]

you might end up starting a log accidentally just by forgetting to pass in the flags.

personally I think this is probably ok, since we do have logic in there to create the tree (this is also useful for testing). maybe we could print a warning that this is happening so users are aware?

I want to remove the "look for any existing tree" logic from createAndInitTree()

+1!

since ranges is a superset of the same info as tlog_id, does it make sense to error on passing in only ranges?

So ranges at the moment can be a superset of tlog_id, but what feels more intuitive to me as a user is that ranges would contain info for only inactive shards, and then tlog_id would point to the active shard. Then, in the code, we could reconcile them. Maybe we can enforce that all ranges need to be inactive with a start and end point, and then in the code we tack on tlog_id for all new entries. That way you also prevent appending the active tree to ranges twice because users won't be expected to do it anymore. WDYT?

[lkatalin]

you might end up starting a log accidentally just by forgetting to pass in the flags.

personally I think this is probably ok, since we do have logic in there to create the tree (this is also useful for testing). maybe we could print a warning that this is happening so users are aware?

I want to remove the "look for any existing tree" logic from createAndInitTree()

+1!

since ranges is a superset of the same info as tlog_id, does it make sense to error on passing in only ranges?

So ranges at the moment can be a superset of tlog_id, but what feels more intuitive to me as a user is that ranges would contain info for only inactive shards, and then tlog_id would point to the active shard. Then, in the code, we could reconcile them. Maybe we can enforce that all ranges need to be inactive with a start and end point, and then in the code we tack on tlog_id for all new entries. That way you also prevent appending the active tree to ranges twice because users won't be expected to do it anymore. WDYT?

That all sounds good to me. Unless @dlorenc or @bobcallaway have any other thoughts on it I'll implement this.

[lkatalin]

@priyawadhwa I just opened #739 to update some of the flag logic. I can rebase this on top of that one.

[lkatalin]

Ignore the first commit; it's a duplicate of #739 because I wanted to get the rebase done. Once #739 is merged, I can remove it.

[priyawadhwa]

LGTM