Implement garbage collection for domains by gdbelvin · Pull Request #1021 · google/keytransparency

gdbelvin · 2018-07-18T09:51:37Z

Currently each test is started with the benefit of a brand new database for maximal test isolation.
However, this is also wasteful, and also not possible in all testing environments. Servers -- and tests should cleanup after themselves. This PR adds the cleanup features needed.

codecov · 2018-07-18T11:55:29Z

Codecov Report

Merging #1021 into master will increase coverage by 0.88%.
The diff coverage is 73.68%.

@@            Coverage Diff             @@
##           master    #1021      +/-   ##
==========================================
+ Coverage   65.12%   66.01%   +0.88%     
==========================================
  Files          39       39              
  Lines        2707     2742      +35     
==========================================
+ Hits         1763     1810      +47     
+ Misses        630      613      -17     
- Partials      314      319       +5

Impacted Files	Coverage Δ
impl/sql/domain/storage.go	`70.47% <100%> (+2.43%)`	⬆️
core/adminserver/admin_server.go	`67.4% <61.9%> (+0.52%)`	⬆️
core/fake/domain_storage.go	`85.71% <71.42%> (+85.71%)`	⬆️

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 122f7ab...d5f8702. Read the comment docs.

Martin2112 · 2018-07-18T12:17:54Z

core/adminserver/admin_server.go

+
+// GarbageCollect looks for domains that have been deleted for longer than a given duration and fully deletes them.
+func (s *Server) GarbageCollect(ctx context.Context, in *pb.GarbageCollectRequest) (*pb.GarbageCollectResponse, error) {
+	duration, err := ptypes.Duration(in.GetDuration())


duration not a great name. Should be related to it's use.

Martin2112 · 2018-07-18T12:19:42Z

core/api/v1/admin.proto

+// GarbageCollect request.
+// Deletes domains that have been deleted longer than `duration`.
+message GarbageCollectRequest {
+  // Domains soft-deleted older than duration will be fully deleted.


"older" -> "longer ago"

I'll change the name to before so the semantics can be a slightly simpler timestamp comparison.

Martin2112 · 2018-07-18T12:19:55Z

core/api/v1/admin.proto

+// Deletes domains that have been deleted longer than `duration`.
+message GarbageCollectRequest {
+  // Domains soft-deleted older than duration will be fully deleted.
+  google.protobuf.Duration duration = 1;


Same naming issue with duration.

Martin2112 · 2018-07-18T12:21:46Z

core/fake/domain_storage_test.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package fake


Fake implementations are not usually tested. Perhaps this is more complex than a fake and should be renamed?

Fakes are indeed not usually tested. However:

The tests did find and help correct a logic bug in the fake.

All the untested fakes are pulling down the code coverage metrics :-)

All I'm really suggesting is that it's not really a fake. More of a dummy implementation.

Well, that's also the definition of a fake. Ideally I would just use the real mysql implementation, but I've structured the repo such that things in core shouldn't have outgoing references to implementation specific things in impl

Martin2112 · 2018-07-18T12:22:47Z

impl/sql/domain/storage.go

  MaxInterval           BIGINT NOT NULL,
  Deleted               INTEGER,
-  DeleteTimeMillis      BIGINT,
+  DeleteTimeSeconds      BIGINT,


Not sure why this changed? I'd suggest storing everything as nanos to avoid potential confusion later.

It was misnamed before. time.Unix() gives seconds, not Millis or Nanos.

Up to you but I still think this will go wrong at some point when it gets compared with some nano value somewhere..

I could use UnixNanos, but this leaves the zero time object undefined. Is there a reason you think that second granularity could be insufficient?

https://godoc.org/time#Time.UnixNano:
UnixNano returns t as a Unix time, the number of nanoseconds elapsed since January 1, 1970 UTC. The result is undefined if the Unix time in nanoseconds cannot be represented by an int64 (a date before the year 1678 or after 2262). Note that this means the result of calling UnixNano on the zero Time is undefined.

I second Martin that second precision is a bit wacky, but I also buy the "undefined" argument. I think it's okay to leave as is for the purposes of this PR.

gdbelvin · 2018-07-23T15:15:17Z

PTAL

pav-kv

Some comments.

pav-kv · 2018-07-24T13:19:05Z

core/api/v1/admin.proto

+}
+
+message GarbageCollectResponse {
+  repeated Domain domains = 1;


How long can this list potentially be?

In current implementations, ~10. Maybe in the future it can grow to thousands.

pav-kv · 2018-07-24T13:19:46Z

core/api/v1/admin.proto

    };
  }
+
+  // Fully delete soft-deleted domains that have been deleted for longer than a duration.


nit: s/for longer than a duration/before the specified timestamp/

pav-kv · 2018-07-24T13:36:16Z

core/domain/storage.go

@@ -46,4 +47,6 @@ type Storage interface {
 	Read(ctx context.Context, domainID string, showDeleted bool) (*Domain, error)
 	// Delete and undelete.


While you are here, could you reword this comment? and looks confusing to me, maybe or? Could be also something like "Soft-delete or undelete the domain."

pav-kv · 2018-07-24T13:39:51Z

core/domain/storage.go

 	// TODO(gbelvin): specify mutation function
-	Deleted bool
+	Deleted          bool
+	DeletedTimestamp time.Time


Just an idea: Maybe we could use DeletedTimestamp.IsZero() as an indicator instead of explicit bool Deleted.

time.Time doc says:

The zero value of type Time is January 1, year 1, 00:00:00.000000000 UTC. As this time is unlikely to come up in practice, the IsZero method gives a simple way of detecting a time that has not been initialized explicitly.

IsZero works in Go, but it's harder to do a SQL query on.
Also, given the somewhat fragile nature of the serialization of the zero date, a boolean is safer.

pav-kv · 2018-07-24T13:41:56Z

core/fake/domain_storage.go

 	if !ok {
 		return nil, status.Errorf(codes.NotFound, "Domain %v not found", ID)
 	}
+	if d.Deleted && !showDeleted {


personal taste nit: How about?

if !ok || d.Deleted && !showDeleted { return nil, status.Errorf(codes.NotFound, "Domain %v not found", ID) }

... to avoid copy-pasting the error.

pav-kv · 2018-07-24T14:16:05Z

impl/sql/domain/storage.go

 		d.VRF.Der, anyData,
 		d.MinInterval.Nanoseconds(), d.MaxInterval.Nanoseconds(),
-		false)
+		false, time.Time{}.Unix())


As you mentioned above, this results in an undefined value (in go playground it's -62135596800). Maybe use zero instead?

This is a bit subtle:

This is the zero value for time.Time. When represented as unix seconds, we get -62135596800. This is small enough that it fits in an int64 and is therefore defined. If we were to use UnixNanos, the number would underflow and be undefined.

Could you add a comment explaining that?

pav-kv · 2018-07-24T14:17:46Z

impl/sql/domain/storage.go

 }
+
+// Delete deletes a domain.
+func (s *storage) Delete(ctx context.Context, domainID string) error {


nit: permanently or hard-deletes

pav-kv · 2018-07-24T14:20:22Z

impl/sql/domain/storage_test.go

+
+func TestList(t *testing.T) {
+	ctx := context.Background()
+	admin, closeF := newStorage(t)


pav-kv · 2018-07-24T14:21:44Z

impl/sql/domain/storage_test.go

+	for _, tc := range []struct {
+		domainID string
+	}{
+		{domainID: "test"},


Same thing here with table-driven test. Either make it not such, or add some more cases. I think one more case can test the showDeleted flag of Read.

See comment above about table tests even for one case.

pav-kv · 2018-07-24T14:25:35Z

core/adminserver/admin_server.go

+			if err := s.domains.Delete(ctx, d.DomainID); err != nil {
+				return nil, err
+			}
+			deleted = append(deleted, dproto)


Why can't you just append d instead of loading dproto?

d just contains the log and map tree IDs. dproto replaces the treeID with the the trillian tree proto.

Oh I see, thought they were the same protos.

…y into f/garbagecollect * 'f/garbagecollect' of github.com:gdbelvin/keytransparency: Remove service-key (google#1022)

pav-kv

LGTM % couple of nits. Please don't forget to regenerate files.

pav-kv · 2018-07-25T12:26:55Z

core/adminserver/admin_server.go

 	return nil, status.Errorf(codes.Unimplemented, "not implemented")
 }
+
+// GarbageCollect looks for domains that have been deleted for longer than a given duration and fully deletes them.


s/for longer than a given duration/before the specified timestamp/

pav-kv · 2018-07-25T12:27:36Z

core/adminserver/admin_server.go

+		return nil, err
+	}
+
+	// Search for domains older than in.Duration.


s/older than in.Duration/deleted earlier than in.Before/

pav-kv · 2018-07-25T12:29:37Z

core/adminserver/admin_server_test.go

 		if got, want := domain.Map.Deleted, true; got != want {
 			t.Errorf("Map.TreeState: %v, want %v", got, want)
 		}
+		// Garbage collect


nit: Redundant comment, the next line is GarbageCollect.

pav-kv · 2018-07-25T12:31:12Z

core/api/v1/admin.proto

    };
  }
+
+  // Fully delete soft-deleted domains that have been deleted before the specified timestamp.


nit: Fully delete the domains that have been soft-deleted before the specified timestamp.

* master: Implement garbage collection for domains (google#1021) Remove TRILLIAN_MYSQL_DRIVER (google#1023) Rename gometalinter gas to gosec (google#1024) Remove service-key (google#1022)

gdbelvin added 4 commits July 18, 2018 03:22

Add GarbageCollection API

1e7c534

Add delete to domain API

96e5de7

Store and retrieve delete timestamp

07adcc6

Add garbage collect function

07b9977

gdbelvin requested review from Martin2112 and pav-kv July 18, 2018 09:53

Make 0 duration explicit

3279217

Martin2112 reviewed Jul 18, 2018

View reviewed changes

gdbelvin and others added 2 commits July 18, 2018 13:55

Use 'before' in API

874accd

Merge branch 'master' into f/garbagecollect

3960468

pav-kv reviewed Jul 24, 2018

View reviewed changes

gdbelvin and others added 5 commits July 25, 2018 10:51

Comments and tests

887d9e3

Merge branch 'f/garbagecollect' of github.com:gdbelvin/keytransparenc…

5f10720

…y into f/garbagecollect * 'f/garbagecollect' of github.com:gdbelvin/keytransparency: Remove service-key (google#1022)

Merge branch 'master' into f/garbagecollect

4005c44

Merge branch 'master' into f/garbagecollect

205af74

Add unix time comment

164b17c

pav-kv approved these changes Jul 25, 2018

View reviewed changes

gdbelvin added 2 commits July 25, 2018 13:38

More comments

24e5673

Regenerate with new comments

d5f8702

gdbelvin merged commit d205da2 into google:master Jul 25, 2018

gdbelvin deleted the f/garbagecollect branch July 25, 2018 13:01

		@@ -46,4 +47,6 @@ type Storage interface {
		Read(ctx context.Context, domainID string, showDeleted bool) (*Domain, error)
		// Delete and undelete.

Conversation

gdbelvin commented Jul 18, 2018

Uh oh!

codecov bot commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin commented Jul 23, 2018

Uh oh!

pav-kv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 18, 2018 •

edited

Loading

gdbelvin Jul 18, 2018 •

edited

Loading