-
Notifications
You must be signed in to change notification settings - Fork 151
Implement garbage collection for domains #1021
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
core/adminserver/admin_server.go
Outdated
|
||
// 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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duration not a great name. Should be related to it's use.
core/api/v1/admin.proto
Outdated
// GarbageCollect request. | ||
// Deletes domains that have been deleted longer than `duration`. | ||
message GarbageCollectRequest { | ||
// Domains soft-deleted older than duration will be fully deleted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"older" -> "longer ago"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the name to before
so the semantics can be a slightly simpler timestamp comparison.
core/api/v1/admin.proto
Outdated
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same naming issue with duration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package fake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fake implementations are not usually tested. Perhaps this is more complex than a fake and should be renamed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I'm really suggesting is that it's not really a fake. More of a dummy implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -41,25 +41,26 @@ CREATE TABLE IF NOT EXISTS Domains( | |||
MinInterval BIGINT NOT NULL, | |||
MaxInterval BIGINT NOT NULL, | |||
Deleted INTEGER, | |||
DeleteTimeMillis BIGINT, | |||
DeleteTimeSeconds BIGINT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this changed? I'd suggest storing everything as nanos to avoid potential confusion later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was misnamed before. time.Unix() gives seconds, not Millis or Nanos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you but I still think this will go wrong at some point when it gets compared with some nano value somewhere..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments.
} | ||
|
||
message GarbageCollectResponse { | ||
repeated Domain domains = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How long can this list potentially be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In current implementations, ~10. Maybe in the future it can grow to thousands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM.
core/api/v1/admin.proto
Outdated
@@ -138,4 +150,8 @@ service KeyTransparencyAdmin { | |||
delete: "/v1/domains/{domain_id}:undelete" | |||
}; | |||
} | |||
|
|||
// Fully delete soft-deleted domains that have been deleted for longer than a duration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/for longer than a duration/before the specified timestamp/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
core/domain/storage.go
Outdated
@@ -46,4 +47,6 @@ type Storage interface { | |||
Read(ctx context.Context, domainID string, showDeleted bool) (*Domain, error) | |||
// Delete and undelete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -33,7 +33,8 @@ type Domain struct { | |||
VRFPriv proto.Message | |||
MinInterval, MaxInterval time.Duration | |||
// TODO(gbelvin): specify mutation function | |||
Deleted bool | |||
Deleted bool | |||
DeletedTimestamp time.Time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
core/fake/domain_storage.go
Outdated
@@ -55,6 +55,9 @@ func (a *DomainStorage) Read(ctx context.Context, ID string, showDeleted bool) ( | |||
if !ok { | |||
return nil, status.Errorf(codes.NotFound, "Domain %v not found", ID) | |||
} | |||
if d.Deleted && !showDeleted { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
impl/sql/domain/storage.go
Outdated
@@ -149,7 +150,7 @@ func (s *storage) Write(ctx context.Context, d *domain.Domain) error { | |||
d.MapID, d.LogID, | |||
d.VRF.Der, anyData, | |||
d.MinInterval.Nanoseconds(), d.MaxInterval.Nanoseconds(), | |||
false) | |||
false, time.Time{}.Unix()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you mentioned above, this results in an undefined value (in go playground it's -62135596800
). Maybe use zero instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment explaining that?
impl/sql/domain/storage.go
Outdated
@@ -204,3 +209,9 @@ func (s *storage) SetDelete(ctx context.Context, domainID string, isDeleted bool | |||
_, err := s.db.ExecContext(ctx, setDeletedSQL, isDeleted, time.Now().Unix(), domainID) | |||
return err | |||
} | |||
|
|||
// Delete deletes a domain. | |||
func (s *storage) Delete(ctx context.Context, domainID string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: permanently or hard-deletes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
impl/sql/domain/storage_test.go
Outdated
|
||
func TestList(t *testing.T) { | ||
ctx := context.Background() | ||
admin, closeF := newStorage(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/admin/s/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
for _, tc := range []struct { | ||
domainID string | ||
}{ | ||
{domainID: "test"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above about table tests even for one case.
if err := s.domains.Delete(ctx, d.DomainID); err != nil { | ||
return nil, err | ||
} | ||
deleted = append(deleted, dproto) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't you just append d
instead of loading dproto
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d
just contains the log and map tree IDs. dproto
replaces the treeID with the the trillian tree proto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % couple of nits. Please don't forget to regenerate files.
core/adminserver/admin_server.go
Outdated
@@ -348,3 +348,37 @@ func (s *Server) DeleteDomain(ctx context.Context, in *pb.DeleteDomainRequest) ( | |||
func (s *Server) UndeleteDomain(ctx context.Context, in *pb.UndeleteDomainRequest) (*google_protobuf.Empty, error) { | |||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/for longer than a given duration/before the specified timestamp/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
core/adminserver/admin_server.go
Outdated
return nil, err | ||
} | ||
|
||
// Search for domains older than in.Duration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/older than in.Duration/deleted earlier than in.Before/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -255,6 +255,17 @@ func TestDelete(t *testing.T) { | |||
if got, want := domain.Map.Deleted, true; got != want { | |||
t.Errorf("Map.TreeState: %v, want %v", got, want) | |||
} | |||
// Garbage collect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Redundant comment, the next line is GarbageCollect
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
core/api/v1/admin.proto
Outdated
@@ -138,4 +150,8 @@ service KeyTransparencyAdmin { | |||
delete: "/v1/domains/{domain_id}:undelete" | |||
}; | |||
} | |||
|
|||
// Fully delete soft-deleted domains that have been deleted before the specified timestamp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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.