Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require tlog_id when log_id_ranges is passed in #739

Merged
merged 1 commit into from
Mar 26, 2022

Conversation

lkatalin
Copy link
Contributor

@lkatalin lkatalin commented Mar 18, 2022

Summary

Based on the conversation @priyawadhwa in #727 , this separates concerns more cleanly between the log_id_ranges flag and tlog_id flag. tlog_id now specifies the active tree (shard) and can be used by itself. log_id_ranges specifies inactive trees (shards) and must be used in conjunction with tlog_id.

@lkatalin lkatalin requested a review from a team as a code owner March 18, 2022 19:19
}

// The last entry is special and should not have a terminating range, because this is active.
// It is specified by the tlog_id.
inputRanges = append(inputRanges, sharding.LogRange{
Copy link
Contributor

@priyawadhwa priyawadhwa Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of appending the active ID, we can add a ActiveTreeID int field to the LogRangesFlag struct that'll make it easier to access it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@priyawadhwa That's a good idea. This also implies we may have to move methods like func (l *LogRanges) ResolveVirtualIndex(index int) and func (l *LogRanges) TotalLength() int64 to be methods on logRangesFlag instead of LogRanges in order to work across all shards (active + inactive), does that seem reasonable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've actually got a better way of doing it, I'm turning the LogRanges struct into this:

type LogRanges struct {
        inactive []LogRange
        active   int64
}

That should be the simplest way, stay tuned!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

if rangeMap != "" {
if err := logRangeMap.Set(rangeMap); err != nil {
log.Logger.Fatal("unable to set logRangeMap from flag: %v", err)
if tlogID == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this check because we check for it later in logRangeMap.Set again

@lkatalin lkatalin force-pushed the flags branch 6 times, most recently from f3c0a78 to 0f31017 Compare March 21, 2022 17:49
@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2022

Codecov Report

Merging #739 (0f31017) into main (caf126d) will increase coverage by 0.00%.
The diff coverage is 64.06%.

@@           Coverage Diff           @@
##             main     #739   +/-   ##
=======================================
  Coverage   47.25%   47.26%           
=======================================
  Files          66       66           
  Lines        5743     5755   +12     
=======================================
+ Hits         2714     2720    +6     
- Misses       2740     2746    +6     
  Partials      289      289           
Impacted Files Coverage Δ
cmd/rekor-server/app/serve.go 3.27% <0.00%> (-0.06%) ⬇️
pkg/sharding/ranges.go 31.57% <30.00%> (-8.43%) ⬇️
cmd/rekor-server/app/flags.go 68.29% <78.57%> (+3.84%) ⬆️
cmd/rekor-server/app/root.go 43.66% <100.00%> (ø)
pkg/sharding/log_index.go 94.73% <100.00%> (-5.27%) ⬇️
pkg/types/helm/v0.0.1/entry.go 52.41% <0.00%> (-1.21%) ⬇️
pkg/types/rekord/v0.0.1/entry.go 51.51% <0.00%> (+0.60%) ⬆️
... and 1 more

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 caf126d...0f31017. Read the comment docs.

@lkatalin lkatalin force-pushed the flags branch 2 times, most recently from 01c2213 to 1b5a745 Compare March 22, 2022 16:27
Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@@ -63,7 +63,7 @@ func init() {

rootCmd.PersistentFlags().String("trillian_log_server.address", "127.0.0.1", "Trillian log server address")
rootCmd.PersistentFlags().Uint16("trillian_log_server.port", 8090, "Trillian log server port")
rootCmd.PersistentFlags().Uint("trillian_log_server.tlog_id", 0, "Trillian tree id")
rootCmd.PersistentFlags().String("trillian_log_server.tlog_id", "0", "Trillian tree id")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I wasn't clear when we chatted offline, I think this should actually remain a Uint for backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaahhh, got it. No worries, easy to fix.

}

var virtualIndex int64
for _, r := range ranges.GetRanges() {
// TODO: Are the tree IDs guaranteed to be ordered deterministically in the Ranges?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, we can assume the user correctly set up the config so that the shards exist in order

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Thank you!!

@@ -44,6 +45,9 @@ func NewLogRanges(path string, treeID string) (LogRanges, error) {
if err != nil {
return LogRanges{}, errors.Wrapf(err, "%s is not a valid int64", treeID)
}
if id == 0 {
return LogRanges{}, errors.New("non-zero tlog_id required when passing in shard config filepath")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add the fix to this error message to make it easier for users:

please set the active tree ID via the `--trillian_log_server.tlog_id` flag

}

func (l *LogRanges) NoInactive() bool {
return l.inactive == nil
}

func (l *LogRanges) Empty() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this might be unused at this point and can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

@lkatalin lkatalin force-pushed the flags branch 2 times, most recently from 787903a to ab51bfa Compare March 22, 2022 16:57
tlog_id specifes the active shard and is kept for backwards compatibility.
To avoid replicating information, the shard config file is used only to
specify inactive shards and must be used in conjunction with a tlog_id flag.
Together, these build the logRanges type in the sharding module.

Signed-off-by: Lily Sturmann <[email protected]>
Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dlorenc dlorenc merged commit befbcc0 into sigstore:main Mar 26, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants