-
Notifications
You must be signed in to change notification settings - Fork 841
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
WIP - feat(atc): support volume size awareness for volume streaming #8693
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
ALTER TABLE volumes | ||
DROP COLUMN size; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
ALTER TABLE volumes | ||
ADD COLUMN size bigint; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -715,11 +715,33 @@ func (worker *Worker) findOrStreamVolume( | |
return Volume{}, err | ||
} | ||
|
||
delegate.StreamingVolume(logger, inputPath, artifact.Source(), streamedVolume.DBVolume().WorkerName()) | ||
// if volume size not cached, get source volume size through LookupVolumeWithSize | ||
var bcSourceVolume baggageclaim.Volume | ||
if volume.DBVolume().Size() <= 0 { | ||
bcSourceVolume, _, err = volume.worker.bcClient.LookupVolumeWithSize(ctx, volume.DBVolume().Handle()) | ||
if err != nil { | ||
logger.Error("failed-to-find-source-volume-for-streaming", err) | ||
return Volume{}, err | ||
} | ||
if inputPath == "for image" { | ||
volume.worker.db.VolumeRepo.UpdateVolumeSize(volume.DBVolume().Handle(), bcSourceVolume.Size()) | ||
} | ||
|
||
logger.Info("update-worker-volume-size", lager.Data{ | ||
"worker": volume.worker.DBWorker().Name(), | ||
"size": bcSourceVolume.Size(), | ||
}) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
delegate.StreamingVolume(logger, inputPath, artifact.Source(), streamedVolume.DBVolume().WorkerName(), bcSourceVolume.Size()) | ||
if err := worker.streamer.Stream(ctx, artifact, streamedVolume); err != nil { | ||
logger.Error("failed-to-stream-artifact", err) | ||
return Volume{}, err | ||
} | ||
if inputPath == "for image" { | ||
worker.db.VolumeRepo.UpdateVolumeSize(streamedVolume.DBVolume().Handle(), bcSourceVolume.Size()) | ||
} | ||
|
||
logger.Debug("streamed-non-local-volume") | ||
return streamedVolume, nil | ||
} | ||
|
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 think we should open this feature to all volumes. If you only want to apply to image volume, then this check should go to line 720.
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.
Based on the metrics, streamed image volume accounts for more than 80% of the total streamed volume(image & non-image).
Also, calculate&cache image volume size should make more sense than non-image volume, because non-image volume size may change swiftly but image volume size could be considered as unchanged.