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

proto: support timestamps #88

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions context.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@ import (
"os"
"path/filepath"
"strings"
"time"

"github.com/containerd/continuity/devices"
driverpkg "github.com/containerd/continuity/driver"
"github.com/containerd/continuity/pathdriver"
"github.com/djherbis/times"
"github.com/opencontainers/go-digest"
)

Expand Down Expand Up @@ -138,6 +140,8 @@ func (c *context) Resource(p string, fi os.FileInfo) (Resource, error) {
return nil, err
}

c.resolveTimestamps(fi, base)

// TODO(stevvooe): Handle windows alternate data streams.

if fi.Mode().IsRegular() {
Expand Down Expand Up @@ -190,6 +194,26 @@ func (c *context) Resource(p string, fi os.FileInfo) (Resource, error) {
return nil, ErrNotFound
}

func (c *context) verifyTimestamps(resource, target Resource) error {
unixZero := func(t time.Time) bool {
Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with t.IsZero()?

Copy link
Member

Choose a reason for hiding this comment

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

More interestingly, why do we care about unix time?

// Note: t.IsZero() returns true if t == 0001-01-01 00:00:00, not Unix epoch
return t.Unix() == 0 && t.UnixNano() == 0
}
if !unixZero(resource.ATime()) && !resource.ATime().Equal(target.ATime()) {
return fmt.Errorf("resource %q has incorrect atime: %v != %v", target.Path(), target.ATime(), resource.ATime())
}
if !unixZero(resource.MTime()) && !resource.MTime().Equal(target.MTime()) {
return fmt.Errorf("resource %q has incorrect mtime: %v != %v", target.Path(), target.MTime(), resource.MTime())
}
if !unixZero(resource.CTime()) && !resource.CTime().Equal(target.CTime()) {
return fmt.Errorf("resource %q has incorrect ctime: %v != %v", target.Path(), target.CTime(), resource.CTime())
}
if !unixZero(resource.BTime()) && !resource.BTime().Equal(target.BTime()) {
return fmt.Errorf("resource %q has incorrect btime: %v != %v", target.Path(), target.BTime(), resource.BTime())
}
return nil
}

func (c *context) verifyMetadata(resource, target Resource) error {
if target.Mode() != resource.Mode() {
return fmt.Errorf("resource %q has incorrect mode: %v != %v", target.Path(), target.Mode(), resource.Mode())
Expand All @@ -203,6 +227,10 @@ func (c *context) verifyMetadata(resource, target Resource) error {
return fmt.Errorf("unexpected gid for %q: %v != %v", target.Path(), target.GID(), target.GID())
}

if err := c.verifyTimestamps(resource, target); err != nil {
return err
}

if xattrer, ok := resource.(XAttrer); ok {
txattrer, tok := target.(XAttrer)
if !tok {
Expand Down Expand Up @@ -541,6 +569,8 @@ func (c *context) Apply(resource Resource) error {
return err
}

// TODO(AkihiroSuda): set timestamps

if xattrer, ok := resource.(XAttrer); ok {
// For xattrs, only ensure that we have those defined in the resource
// and their values are set. We can ignore other xattrs. In other words,
Expand Down Expand Up @@ -639,3 +669,18 @@ func (c *context) resolveXAttrs(fp string, fi os.FileInfo, base *resource) (map[

return nil, nil
}

// resolveTimestamps set base.[AMCB]Times.
// When BTime is unsupported, it is set to the Unix epoch.
func (c *context) resolveTimestamps(fi os.FileInfo, base *resource) {
ts := times.Get(fi)
base.atime = ts.AccessTime()
base.mtime = ts.ModTime()
base.ctime = ts.ChangeTime()
if ts.HasBirthTime() {
base.btime = ts.BirthTime()
} else {
// Note: time.Time{} stands for 0001-01-01 00:00:00 UTC
base.btime = time.Unix(0, 0)
Copy link
Member

Choose a reason for hiding this comment

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

I'm very worried about the redefinition of the zero time. This should really be time.Time{}.

Copy link
Member

Choose a reason for hiding this comment

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

Does that get serialized correctly?

}
}
3 changes: 3 additions & 0 deletions driver/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ var ErrNotSupported = fmt.Errorf("not supported")
type Driver interface {
// Note that Open() returns a File interface instead of *os.File. This
// is because os.File is a struct, so if Open was to return *os.File,

// the only way to fulfill the interface would be to call os.Open()
Open(path string) (File, error)
OpenFile(path string, flag int, perm os.FileMode) (File, error)
Expand All @@ -43,6 +44,8 @@ type Driver interface {
// interface in the future as more platforms are added.
Mknod(path string, mode os.FileMode, major int, minor int) error
Mkfifo(path string, mode os.FileMode) error

// TODO(AkihiroSuda): setter for timestamps (Stat already works as getter)
}

// File is the interface for interacting with files returned by continuity's Open
Expand Down
83 changes: 62 additions & 21 deletions proto/manifest.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions proto/manifest.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ syntax = "proto3";

package proto;

import "google/protobuf/timestamp.proto";

// Manifest specifies the entries in a container bundle, keyed and sorted by
// path.
message Manifest {
Expand Down Expand Up @@ -65,6 +67,13 @@ message Resource {
// Ads stores one or more alternate data streams for the target resource.
repeated ADSEntry ads = 13;

// Timestamps.
// btime (birthtime) is available on Darwin, FreeBSD, NetBSD, and Windows.
// When btime is unknown, it SHOULD be set to zero.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not omit the field instead of setting it to zero? I believe these fields are nullable.

google.protobuf.Timestamp atime = 14;
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to document the fields.

google.protobuf.Timestamp mtime = 15;
google.protobuf.Timestamp ctime = 16;
google.protobuf.Timestamp btime = 17;
}

// XAttr encodes extended attributes for a resource.
Expand Down
85 changes: 77 additions & 8 deletions resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ import (
"os"
"reflect"
"sort"
"time"

pb "github.com/containerd/continuity/proto"
"github.com/golang/protobuf/ptypes"
tspb "github.com/golang/protobuf/ptypes/timestamp"
"github.com/opencontainers/go-digest"
)

Expand All @@ -28,6 +31,11 @@ type Resource interface {

UID() int64
GID() int64

ATime() time.Time
MTime() time.Time
CTime() time.Time
BTime() time.Time
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 it's fine to use these abbreviated forms - they're pretty widely used. But it would be useful to have a comment explaining what each one means.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it might be okay to follow the times library and just have AccessTime, ModTime and ChangeTime.

Copy link
Member

Choose a reason for hiding this comment

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

Document these. Please change to the expanded versions to match golang os package.

}

// ByPath provides the canonical sort order for a set of resources. Use with
Expand Down Expand Up @@ -108,6 +116,22 @@ func Merge(fs ...Resource) (Resource, error) {
return nil, fmt.Errorf("gid does not match: %v != %v", f.GID(), prototype.GID())
}

if f.ATime() != prototype.ATime() {
return nil, fmt.Errorf("atime does not match: %v != %v", f.ATime(), prototype.ATime())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little worried this will be racy. If a hardlinked file is accessed in between the Stats, atime wouldn't match. The existing checks for things like mode and UID/GID are much less likely to change in the narrow window between two Stats.

Should we avoid considering atime when merging hardlinks?

}

if f.MTime() != prototype.MTime() {
return nil, fmt.Errorf("mtime does not match: %v != %v", f.MTime(), prototype.MTime())
}

if f.CTime() != prototype.CTime() {
return nil, fmt.Errorf("ctime does not match: %v != %v", f.CTime(), prototype.CTime())
}

if f.BTime() != prototype.BTime() {
return nil, fmt.Errorf("btime does not match: %v != %v", f.BTime(), prototype.BTime())
}

if xattrer, ok := f.(XAttrer); ok {
fxattrs := xattrer.XAttrs()
if !reflect.DeepEqual(fxattrs, xattrs) {
Expand Down Expand Up @@ -170,6 +194,10 @@ func Merge(fs ...Resource) (Resource, error) {
mode: first.Mode(),
uid: first.UID(),
gid: first.GID(),
atime: first.ATime(),
mtime: first.MTime(),
ctime: first.CTime(),
btime: first.BTime(),
xattrs: xattrs,
}

Expand Down Expand Up @@ -238,10 +266,11 @@ type Device interface {
}

type resource struct {
paths []string
mode os.FileMode
uid, gid int64
xattrs map[string][]byte
paths []string
mode os.FileMode
uid, gid int64
atime, mtime, ctime, btime time.Time
xattrs map[string][]byte
}

var _ Resource = &resource{}
Expand All @@ -266,6 +295,22 @@ func (r *resource) GID() int64 {
return r.gid
}

func (r *resource) ATime() time.Time {
return r.atime
}

func (r *resource) MTime() time.Time {
return r.mtime
}

func (r *resource) CTime() time.Time {
return r.ctime
}

func (r *resource) BTime() time.Time {
return r.btime
}

type regularFile struct {
resource
size int64
Expand Down Expand Up @@ -454,15 +499,27 @@ func (d device) Minor() uint64 {
return d.minor
}

func timestampProto(t time.Time) *tspb.Timestamp {
x, err := ptypes.TimestampProto(t)
if err != nil {
panic(err) // FIXME
}
return x
}

// toProto converts a resource to a protobuf record. We'd like to push this
// the individual types but we want to keep this all together during
// prototyping.
func toProto(resource Resource) *pb.Resource {
b := &pb.Resource{
Path: []string{resource.Path()},
Mode: uint32(resource.Mode()),
Uid: resource.UID(),
Gid: resource.GID(),
Path: []string{resource.Path()},
Mode: uint32(resource.Mode()),
Uid: resource.UID(),
Gid: resource.GID(),
Atime: timestampProto(resource.ATime()),
Mtime: timestampProto(resource.MTime()),
Ctime: timestampProto(resource.CTime()),
Btime: timestampProto(resource.BTime()),
}

if xattrer, ok := resource.(XAttrer); ok {
Expand Down Expand Up @@ -503,13 +560,25 @@ func toProto(resource Resource) *pb.Resource {
return b
}

func timestampFromProto(ts *tspb.Timestamp) time.Time {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's return time.Time{} when ts == nil. This will clearly distinguish missing timestamps, for example from old manifests that don't implement timestamping, or ones generated on OSs that don't support every field. Otherwise we would return the unix epoch here.

Copy link
Member

Choose a reason for hiding this comment

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

Just use the protobuf helper function here.

x, err := ptypes.Timestamp(ts)
if err != nil {
panic(err) // FIXME
}
return x
}

// fromProto converts from a protobuf Resource to a Resource interface.
func fromProto(b *pb.Resource) (Resource, error) {
base := &resource{
paths: b.Path,
mode: os.FileMode(b.Mode),
uid: b.Uid,
gid: b.Gid,
atime: timestampFromProto(b.Atime),
mtime: timestampFromProto(b.Mtime),
ctime: timestampFromProto(b.Ctime),
btime: timestampFromProto(b.Btime),
}

base.xattrs = make(map[string][]byte, len(b.Xattr))
Expand Down