From a92c0e30163d4ca403e5e2f4bd1d4fed226ac708 Mon Sep 17 00:00:00 2001 From: "Ciro S. Costa" Date: Mon, 11 May 2020 13:18:30 -0500 Subject: [PATCH 1/5] [fixes #5572] WIP - Tracing for Image Streaming Signed-off-by: Sameer Vohra Co-authored-by: Zoe Tian Signed-off-by: Zoe Tian --- atc/worker/artifact_source.go | 9 +++++++++ atc/worker/image/image.go | 4 ++++ atc/worker/volume.go | 6 ++++++ atc/worker/worker.go | 32 ++++++++++++++++++++------------ 4 files changed, 39 insertions(+), 12 deletions(-) diff --git a/atc/worker/artifact_source.go b/atc/worker/artifact_source.go index 0ba500194..10bc303a1 100644 --- a/atc/worker/artifact_source.go +++ b/atc/worker/artifact_source.go @@ -8,6 +8,7 @@ import ( "code.cloudfoundry.org/lager" "github.com/concourse/concourse/atc/compression" "github.com/concourse/concourse/atc/runtime" + "github.com/concourse/concourse/tracing" "github.com/hashicorp/go-multierror" ) @@ -60,6 +61,14 @@ func (source *artifactSource) StreamTo( logger lager.Logger, destination ArtifactDestination, ) error { + ctx, span := tracing.StartSpan(ctx, "artifactSource.StreamTo", nil) + defer span.End() + + _, outSpan := tracing.StartSpan(ctx, "volume.StreamOut", tracing.Attrs{ + "origin-volume": source.volume.Handle(), + "origin-worker": source.volume.WorkerName(), + }) + defer outSpan.End() out, err := source.volume.StreamOut(ctx, ".", source.compression.Encoding()) if err != nil { return err diff --git a/atc/worker/image/image.go b/atc/worker/image/image.go index cdc4421cb..fa498e73d 100644 --- a/atc/worker/image/image.go +++ b/atc/worker/image/image.go @@ -2,6 +2,7 @@ package image import ( "context" + "github.com/concourse/concourse/tracing" "io" "net/url" "path" @@ -77,6 +78,9 @@ func (i *imageProvidedByPreviousStepOnDifferentWorker) FetchForContainer( logger lager.Logger, container db.CreatingContainer, ) (worker.FetchedImage, error) { + ctx, span := tracing.StartSpan(ctx, "imageProvidedByPreviousStepOnDifferentWorker.FetchForContainer", tracing.Attrs{"container_id": container.Handle()}) + defer span.End() + imageVolume, err := i.volumeClient.FindOrCreateVolumeForContainer( logger, worker.VolumeSpec{ diff --git a/atc/worker/volume.go b/atc/worker/volume.go index ebb6f032f..3353ed777 100644 --- a/atc/worker/volume.go +++ b/atc/worker/volume.go @@ -2,6 +2,7 @@ package worker import ( "context" + "github.com/concourse/concourse/tracing" "io" "code.cloudfoundry.org/lager" @@ -86,6 +87,11 @@ func (v *volume) SetPrivileged(privileged bool) error { } func (v *volume) StreamIn(ctx context.Context, path string, encoding baggageclaim.Encoding, tarStream io.Reader) error { + _, span := tracing.StartSpan(ctx, "volume.StreamIn", tracing.Attrs{ + "destination-volume": v.Handle(), + "destination-worker": v.WorkerName(), + }) + defer span.End() return v.bcVolume.StreamIn(ctx, path, encoding, tarStream) } diff --git a/atc/worker/worker.go b/atc/worker/worker.go index 84b23ec04..35e1b50b0 100644 --- a/atc/worker/worker.go +++ b/atc/worker/worker.go @@ -8,6 +8,7 @@ import ( "context" "errors" "fmt" + "github.com/concourse/concourse/tracing" "path/filepath" "sort" "strings" @@ -504,16 +505,19 @@ func (worker *gardenWorker) createVolumes( return nil, err } - streamedMounts, err := worker.cloneRemoteVolumes( - ctx, - logger, - spec.TeamID, - isPrivileged, - creatingContainer, - nonlocalInputs, - ) - if err != nil { - return nil, err + streamedMounts := []VolumeMount{} + if len(nonlocalInputs) > 0 { + streamedMounts, err = worker.cloneRemoteVolumes( + ctx, + logger, + spec.TeamID, + isPrivileged, + creatingContainer, + nonlocalInputs, + ) + if err != nil { + return nil, err + } } ioVolumeMounts = append(ioVolumeMounts, cowMounts...) @@ -595,6 +599,10 @@ func (worker *gardenWorker) cloneRemoteVolumes( container db.CreatingContainer, nonLocals []mountableRemoteInput, ) ([]VolumeMount, error) { + + ctx, span := tracing.StartSpan(ctx, "worker.cloneRemoteVolumes", tracing.Attrs{"container_id": container.Handle()}) + defer span.End() + mounts := make([]VolumeMount, len(nonLocals)) g, groupCtx := errgroup.WithContext(ctx) @@ -615,8 +623,8 @@ func (worker *gardenWorker) cloneRemoteVolumes( return []VolumeMount{}, err } destData := lager.Data{ - "dest-volume": inputVolume.Handle(), - "dest-worker": inputVolume.WorkerName(), + "destination-volume": inputVolume.Handle(), + "destination-worker": inputVolume.WorkerName(), } g.Go(func() error { From ba8019940299bca352cd5b6c3a526dc80d4739fa Mon Sep 17 00:00:00 2001 From: Zoe Tian Date: Tue, 2 Jun 2020 11:15:00 -0400 Subject: [PATCH 2/5] [#5572] atc/worker - Add errors to volume tracing - add volume stream out error to trace - add volume stream in error to trace Signed-off-by: Sameer Vohra Co-authored-by: Sameer Vohra Signed-off-by: Zoe Tian --- atc/worker/artifact_source.go | 8 ++++---- atc/worker/volume.go | 7 +++++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/atc/worker/artifact_source.go b/atc/worker/artifact_source.go index 10bc303a1..021b27e12 100644 --- a/atc/worker/artifact_source.go +++ b/atc/worker/artifact_source.go @@ -70,17 +70,17 @@ func (source *artifactSource) StreamTo( }) defer outSpan.End() out, err := source.volume.StreamOut(ctx, ".", source.compression.Encoding()) + if err != nil { + tracing.End(outSpan, err) return err } defer out.Close() err = destination.StreamIn(ctx, ".", source.compression.Encoding(), out) - if err != nil { - return err - } - return nil + + return err } // TODO: figure out if we want logging before and after streams, I remove logger from private methods diff --git a/atc/worker/volume.go b/atc/worker/volume.go index 3353ed777..36eafc102 100644 --- a/atc/worker/volume.go +++ b/atc/worker/volume.go @@ -91,8 +91,11 @@ func (v *volume) StreamIn(ctx context.Context, path string, encoding baggageclai "destination-volume": v.Handle(), "destination-worker": v.WorkerName(), }) - defer span.End() - return v.bcVolume.StreamIn(ctx, path, encoding, tarStream) + + err := v.bcVolume.StreamIn(ctx, path, encoding, tarStream) + tracing.End(span, err) + + return err } func (v *volume) StreamOut(ctx context.Context, path string, encoding baggageclaim.Encoding) (io.ReadCloser, error) { From 05136e40d157a181a921cadb7c7c06d0790c5e14 Mon Sep 17 00:00:00 2001 From: Zoe Tian Date: Tue, 2 Jun 2020 11:20:01 -0400 Subject: [PATCH 3/5] [#5572] tracing - Fix End func - change error attribute from `error` to `error-message` The `error` attribute appears to be a reserved field when the status is set to `internal` and this resulted in the error message set by Concourse being overriden. To prevent this clash, the Concourse error message was moved to `error-message` Signed-off-by: Sameer Vohra Co-authored-by: Sameer Vohra Signed-off-by: Zoe Tian --- tracing/tracer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tracing/tracer.go b/tracing/tracer.go index 555d24ab5..abcce7f82 100644 --- a/tracing/tracer.go +++ b/tracing/tracer.go @@ -144,7 +144,7 @@ func End(span trace.Span, err error) { if err != nil { span.SetStatus(codes.Internal) span.SetAttributes( - key.New("error").String(err.Error()), + key.New("error-message").String(err.Error()), ) } From caedf9fd105deb1d2835c36e4cbb6117b61a0b20 Mon Sep 17 00:00:00 2001 From: Zoe Tian Date: Tue, 2 Jun 2020 12:10:00 -0400 Subject: [PATCH 4/5] [Fixes #5572] atc/worker - refactor cloneRemoteVolumes Signed-off-by: Zoe Tian Co-authored-by: Sameer Vohra Signed-off-by: Zoe Tian --- atc/worker/worker.go | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/atc/worker/worker.go b/atc/worker/worker.go index 35e1b50b0..48d919d44 100644 --- a/atc/worker/worker.go +++ b/atc/worker/worker.go @@ -8,7 +8,6 @@ import ( "context" "errors" "fmt" - "github.com/concourse/concourse/tracing" "path/filepath" "sort" "strings" @@ -23,6 +22,7 @@ import ( "github.com/concourse/concourse/atc/resource" "github.com/concourse/concourse/atc/runtime" "github.com/concourse/concourse/atc/worker/gclient" + "github.com/concourse/concourse/tracing" "github.com/cppforlife/go-semi-semantic/version" "golang.org/x/sync/errgroup" ) @@ -505,19 +505,16 @@ func (worker *gardenWorker) createVolumes( return nil, err } - streamedMounts := []VolumeMount{} - if len(nonlocalInputs) > 0 { - streamedMounts, err = worker.cloneRemoteVolumes( - ctx, - logger, - spec.TeamID, - isPrivileged, - creatingContainer, - nonlocalInputs, - ) - if err != nil { - return nil, err - } + streamedMounts, err := worker.cloneRemoteVolumes( + ctx, + logger, + spec.TeamID, + isPrivileged, + creatingContainer, + nonlocalInputs, + ) + if err != nil { + return nil, err } ioVolumeMounts = append(ioVolumeMounts, cowMounts...) @@ -600,10 +597,14 @@ func (worker *gardenWorker) cloneRemoteVolumes( nonLocals []mountableRemoteInput, ) ([]VolumeMount, error) { + mounts := make([]VolumeMount, len(nonLocals)) + if len(nonLocals) <= 0 { + return mounts, nil + } + ctx, span := tracing.StartSpan(ctx, "worker.cloneRemoteVolumes", tracing.Attrs{"container_id": container.Handle()}) defer span.End() - mounts := make([]VolumeMount, len(nonLocals)) g, groupCtx := errgroup.WithContext(ctx) for i, nonLocalInput := range nonLocals { @@ -647,9 +648,7 @@ func (worker *gardenWorker) cloneRemoteVolumes( return nil, err } - if len(nonLocals) > 0 { - logger.Debug("streamed-non-local-volumes", lager.Data{"volumes-streamed": len(nonLocals)}) - } + logger.Debug("streamed-non-local-volumes", lager.Data{"volumes-streamed": len(nonLocals)}) return mounts, nil } From 764753f0b946335863d981834797c4bbdcbaee1e Mon Sep 17 00:00:00 2001 From: Zoe Tian Date: Tue, 2 Jun 2020 12:22:54 -0400 Subject: [PATCH 5/5] add release notes Signed-off-by: Zoe Tian Co-authored-by: Sameer Vohra --- release-notes/latest.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/release-notes/latest.md b/release-notes/latest.md index 497b8420c..8e313976f 100644 --- a/release-notes/latest.md +++ b/release-notes/latest.md @@ -69,3 +69,7 @@ Currently the only API action that can be limited in this way is `ListAllJobs` - #### :link: fix * Fixed a bug where fly would no longer tell you if the team you logged in with was invalid + +#### :link: feature + +* Add tracing to allow users and developers to observe volume streaming from source to destination volumes. #5579