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.

Sign up for GitHub

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

Jump to bottom

Update objects timestamps on cache hit #542

Merged
merged 4 commits into from
Apr 25, 2022
Merged

Update objects timestamps on cache hit #542

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2c6495
Update objects timestamps on cache hit
PycKamil Apr 15, 2022
f5cfdf9
Update documentation
PycKamil Apr 20, 2022
1eae8f5
Update samples
PycKamil Apr 20, 2022
39d2c06
Use `ReplaceMetadata` to avoid `This copy request is illegal because …
PycKamil Apr 20, 2022
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
51 changes: 37 additions & 14 deletions cache/s3proxy/s3proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ type uploadReq struct {
}

type s3Cache struct {
mcore *minio.Core
prefix string
bucket string
uploadQueue chan<- uploadReq
accessLogger cache.Logger
errorLogger cache.Logger
v2mode bool
objectKey func(hash string, kind cache.EntryKind) string
mcore *minio.Core
prefix string
bucket string
uploadQueue chan<- uploadReq
accessLogger cache.Logger
errorLogger cache.Logger
v2mode bool
updateTimestamps bool
objectKey func(hash string, kind cache.EntryKind) string
}

var (
Expand All @@ -56,6 +57,7 @@ func New(
Prefix string,
Credentials *credentials.Credentials,
DisableSSL bool,
UpdateTimestamps bool,
Region string,

storageMode string, accessLogger cache.Logger,
Expand Down Expand Up @@ -88,12 +90,13 @@ func New(
}

c := &s3Cache{
mcore: minioCore,
prefix: Prefix,
bucket: Bucket,
accessLogger: accessLogger,
errorLogger: errorLogger,
v2mode: storageMode == "zstd",
mcore: minioCore,
prefix: Prefix,
bucket: Bucket,
accessLogger: accessLogger,
errorLogger: errorLogger,
v2mode: storageMode == "zstd",
updateTimestamps: UpdateTimestamps,
}

if c.v2mode {
Expand Down Expand Up @@ -196,6 +199,22 @@ func (c *s3Cache) Put(ctx context.Context, kind cache.EntryKind, hash string, si
}
}

func (c *s3Cache) UpdateModificationTimestamp(ctx context.Context, bucket string, object string) {
src := minio.CopySrcOptions{
Bucket: bucket,
Object: object,
}

dst := minio.CopyDestOptions{
Bucket: bucket,
Object: object,
}

_, err := c.mcore.ComposeObject(context.Background(), dst, src)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that the copying is done server side- nice. Are there any downsides to having this always on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - I don't see any downsides in terms of performance or storage usage but I'm running self hosted s3 storage. As I understand AWS s3 pricing documentation there is some cost for coping object - you pay the S3 charges for storage in the selected destination S3 storage classes, for the primary copy, for replication PUT request. So making this turn on by default could potentially generate costs for people using AWS so maybe it's better to leave it as opt-in?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, let's make it opt-in to be on the safe side.


logResponse(c.accessLogger, "COMPOSE", bucket, object, err)
}

func (c *s3Cache) Get(ctx context.Context, kind cache.EntryKind, hash string) (io.ReadCloser, int64, error) {

rc, info, _, err := c.mcore.GetObject(
Expand All @@ -216,6 +235,10 @@ func (c *s3Cache) Get(ctx context.Context, kind cache.EntryKind, hash string) (i
}
cacheHits.Inc()

if c.updateTimestamps {
c.UpdateModificationTimestamp(ctx, c.bucket, c.objectKey(hash, kind))
}

logResponse(c.accessLogger, "DOWNLOAD", c.bucket, c.objectKey(hash, kind), nil)

if kind == cache.CAS && c.v2mode {
Expand Down
1 change: 1 addition & 0 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,7 @@ func get(ctx *cli.Context) (*Config, error) {
AccessKeyID: ctx.String("s3.access_key_id"),
SecretAccessKey: ctx.String("s3.secret_access_key"),
DisableSSL: ctx.Bool("s3.disable_ssl"),
UpdateTimestamps: ctx.Bool("s3.update_timestamps"),
mostynb marked this conversation as resolved.
Show resolved Hide resolved
IAMRoleEndpoint: ctx.String("s3.iam_role_endpoint"),
Region: ctx.String("s3.region"),
AWSProfile: ctx.String("s3.aws_profile"),
Expand Down
1 change: 1 addition & 0 deletions config/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ func (c *Config) setProxy() error {
c.S3CloudStorage.Prefix,
creds,
c.S3CloudStorage.DisableSSL,
c.S3CloudStorage.UpdateTimestamps,
c.S3CloudStorage.Region,
c.StorageMode, c.AccessLogger, c.ErrorLogger, c.NumUploaders, c.MaxQueuedUploads)
return nil
Expand Down
1 change: 1 addition & 0 deletions config/s3.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type S3CloudStorageConfig struct {
AccessKeyID string `yaml:"access_key_id"`
SecretAccessKey string `yaml:"secret_access_key"`
DisableSSL bool `yaml:"disable_ssl"`
UpdateTimestamps bool `yaml:"update_timestamps"`
IAMRoleEndpoint string `yaml:"iam_role_endpoint"`
Region string `yaml:"region"`
KeyVersion *int `yaml:"key_version"`
Expand Down