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

Store trillian map,log trees in DB #1167

Merged
merged 8 commits into from Jan 24, 2019

Conversation

Projects
None yet
2 participants
@Zyqsempai
Copy link
Contributor

Zyqsempai commented Jan 23, 2019

  • Romeved from directory table:
    MapId, LogId fields
  • Added to directory table:
    Map, Log fields with type BLOB
  • Removed fecth request to TrillianTree

Fixes: #1152

@Zyqsempai Zyqsempai requested a review from gdbelvin as a code owner Jan 23, 2019

@Zyqsempai Zyqsempai changed the title Store trillian map,log trees in DB [WIP]Store trillian map,log trees in DB Jan 23, 2019

Zyqsempai added some commits Jan 23, 2019

@Zyqsempai

This comment has been minimized.

Copy link
Contributor Author

Zyqsempai commented Jan 23, 2019

@gdbelvin Hi, can you help a little bit with failing tests, i am confused with tests reports i get.
Also, how can I run test ENV for e-to-e tests?

@@ -23,11 +23,11 @@ import (
"github.com/golang/protobuf/proto"
"github.com/google/go-cmp/cmp"
"github.com/google/keytransparency/core/directory"
"github.com/google/trillian"

This comment has been minimized.

@gdbelvin

gdbelvin Jan 23, 2019

Collaborator

nit: because the trillian package is a proto, we prefer to import rename it to something ending in "pb"

Suggested change
"github.com/google/trillian"
tpb "github.com/google/trillian"
@gdbelvin

This comment has been minimized.

Copy link
Collaborator

gdbelvin commented Jan 23, 2019

Your changes so far look great.
To run end to end tests: go test ./impl/integration. This creates a test Env and passes it to the integration tests in core/integration.

The errors CI is currently generating suggest some kind of data corruption. Perhaps the log and map trees are getting swapped?

tlog.GetLatestSignedLogRoot(9171319461164051086): rpc error: code = NotFound desc = tree 9171319461164051086 not found

NewLogVerifierFromTree(): TreeType: MAP, want LOG or PREORDERED_LOG
@Zyqsempai

This comment has been minimized.

Copy link
Contributor Author

Zyqsempai commented Jan 23, 2019

@gdbelvin Not sure if that only my problem, but docker-compose up doesn't work, so i cannot build up all infra for tests.

@gdbelvin

This comment has been minimized.

Copy link
Collaborator

gdbelvin commented Jan 23, 2019

Please see #1169 for the docker-compose fix.
Please ensure that the continuous integration tests and the following commands pass:

go test ./...
golangci-lint run
@Zyqsempai

This comment has been minimized.

Copy link
Contributor Author

Zyqsempai commented Jan 24, 2019

Unfortunately still have problems with docker-compose even after your fix.

Step 1/8 : FROM golang:1.10 as build
ERROR: Service 'log-server' failed to build: Get https://registry-1.docker.io/v2/: Service Unavailable```
Seems like i doesn't have access to the registry.

Zyqsempai added some commits Jan 24, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 24, 2019

Codecov Report

Merging #1167 into master will increase coverage by 0.24%.
The diff coverage is 65.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1167      +/-   ##
==========================================
+ Coverage   64.68%   64.92%   +0.24%     
==========================================
  Files          41       41              
  Lines        3112     3122      +10     
==========================================
+ Hits         2013     2027      +14     
+ Misses        757      751       -6     
- Partials      342      344       +2
Impacted Files Coverage Δ
core/adminserver/admin_server.go 69.27% <100%> (+1.51%) ⬆️
core/sequencer/trillian_client.go 59.18% <100%> (+2.82%) ⬆️
core/keyserver/revisions.go 63.92% <50%> (ø) ⬆️
core/keyserver/keyserver.go 55.81% <55.55%> (+1.44%) ⬆️
impl/sql/directory/storage.go 67.66% <62.5%> (-2.81%) ⬇️

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 d068f14...479f2bc. Read the comment docs.

@Zyqsempai

This comment has been minimized.

Copy link
Contributor Author

Zyqsempai commented Jan 24, 2019

So, the tests finaly fixed, but i found important issue.
The problem is that we cannot update diretories in DB, but we need it as far as we now store trees there, and for example on Delete action we should somehow update Deleted status in those trees, but we can't.
So looks like we should extend storage interface with Update method, or make Write method upsert type.
I can do it in separate PR.

@Zyqsempai Zyqsempai changed the title [WIP]Store trillian map,log trees in DB Store trillian map,log trees in DB Jan 24, 2019

@gdbelvin
Copy link
Collaborator

gdbelvin left a comment

Wow. This is awesome. Thanks so much.

@gdbelvin

This comment has been minimized.

Copy link
Collaborator

gdbelvin commented Jan 24, 2019

As for updates, I think its debatable. The deleted column would be the only relevant field that could change, but I think a good argument could be made to remove that field from the filter list. At the KT layer, we should care about deleted / undeleted directories, not trees.

@gdbelvin gdbelvin merged commit 7c351be into google:master Jan 24, 2019

5 checks passed

GolangCI No issues found!
Details
cla/google All necessary CLAs are signed
codecov/patch 65.57% of diff hit (target 64.68%)
Details
codecov/project 64.92% (+0.24%) compared to d068f14
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Zyqsempai

This comment has been minimized.

Copy link
Contributor Author

Zyqsempai commented Jan 24, 2019

@gdbelvin Agree, if directory marked as delete I assume we can consider trees deleted as well.
I can remove those fields in separate PR.

gdbelvin added a commit to gdbelvin/keytransparency that referenced this pull request Jan 28, 2019

Merge branch 'master' into mutator
* master:
  Bump the golangci-lint version to v1.13.1 (google#1171)
  Remove deleted status from map and log trees (google#1172)
  tinkio.ProtoKeysetFile (google#1173)
  Update API links (google#1170)
  Store trillian map,log trees in DB (google#1167)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.