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?
Conversation
Signed-off-by: Ming Xu <mingx@vmware.com>
Signed-off-by: Ming Xu <mingx@vmware.com>
Signed-off-by: Ming Xu <mingx@vmware.com>
"worker": volume.worker.DBWorker().Name(), | ||
"size": bcSourceVolume.Size(), | ||
}) | ||
} |
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.
When volume.DBVolume().Size() > 0
, where bcSourceVolume
is initialized?
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.
bcSourceVolume
is initialized in line 719(https://github.com/concourse/concourse/pull/8693/files#diff-2034974614561f2b31bff2d4e9572aa96a162533d58b49a6cc06cfd74625d905R719).
logger.Error("failed-to-find-source-volume-for-streaming", err) | ||
return Volume{}, err | ||
} | ||
if inputPath == "for image" { |
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.
} | ||
} | ||
|
||
func (vs *VolumeServer) GetVolumeWithSize(w http.ResponseWriter, req *http.Request) { |
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 function has completely duplicate to GetVolume()
. We should add an private function:
func (vs *VolumeServer) getVolume(w http.ResponseWriter, req *http.Request, withSize bool) {
}
func (vs *VolumeServer) GetVolume(w http.ResponseWriter, req *http.Request) {
return vs.getVolume(w, req, false)
}
func (vs *VolumeServer) GetVolumeWithSize(w http.ResponseWriter, req *http.Request) {
return vs.getVolume(w, req, true)
}
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.
Where GetVolumeWithSize()
is used?
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.
Sure, updated.
Where GetVolumeWithSize() is used?
It's used ingetVolumeResponse()
for sending GetVolume or GetVolumeWithSize request.
worker/baggageclaim/client/client.go
Signed-off-by: Ming Xu <mingx@vmware.com>
I wonder what is the relationship between this PR and #8756 (the latter being in release https://github.com/concourse/concourse/releases/tag/v7.10.0) ? |
What does this PR accomplish?
Bug Fix | Feature | Documentation
We received multiple user report error like
failed to create container on worker: context deadline exceeded
in our environment recently, the root cause seems to be user pipeline often stream huge volume unintentionally. And sometimes because of intermittent network instability, the volume streaming could be flaky especially when the volume is huge. Adding volume size awareness for volume streaming could be helpful in this situation, for example user could notice the unexpected huge volume size and then optimize the pipeline by remove unnecessary files in the volume.Changes proposed by this PR:
volume
table to avoid duplicate volume size detection.Notes to reviewer:
The screenshot contains
streaming volume volume1 ($volumeSize) from worker1...
Release Note
Contributor Checklist
Reviewer Checklist
BOSH and
Helm packaging; otherwise, ignored for
the integration
tests
(for example, if they are Garden configs that are not displayed in the
--help
text).