Allow OperationId to be passed through Image mgmt code#8147
Allow OperationId to be passed through Image mgmt code#8147lcastellano merged 1 commit intovmware:masterfrom
Conversation
lcastellano
force-pushed
the
props
branch
3 times, most recently
from
9b0c801 to
0347c9d
Compare
| "archive/tar" | ||
| "bytes" | ||
| "compress/gzip" | ||
| context "context" |
There was a problem hiding this comment.
This is aliasing context to context and should not be necessary.
| @@ -407,7 +407,7 @@ func (s *SystemBackend) AuthenticateToRegistry(ctx context.Context, authConfig * | |||
| log.Debugf("logging onto %s", authURL.String()) | |||
There was a problem hiding this comment.
This and a couple of other log.XXX calls should be updated to use the op.XXX equivalent.
lib/imagec/docker.go
Outdated
|
|
||
| if err != nil && options.InsecureAllowHTTP { | ||
| // try https without verification | ||
| log.Debugf("Trying https without verification, last error: %+v", err) |
There was a problem hiding this comment.
replace log.XXX with op.XXX, here and elsewhere in the function.
| @@ -104,7 +104,8 @@ func (s *serialProgressOutput) stop() { | |||
| // It handles existing and simultaneous layer download de-duplication | |||
| // This code is utilizes Docker's xfer package: https://github.com/docker/docker/tree/v1.11.2/distribution/xfer | |||
| func (ldm *LayerDownloader) DownloadLayers(ctx context.Context, ic *ImageC) error { | |||
There was a problem hiding this comment.
I'm inclined to either:
a. have the exported methods using plain context.Context and calling trace.FromContext, or
b. have all of the methods taking trace.Operation
At the moment we have a mix (see lib/imagec/docker.go)
lib/imagec/download.go
Outdated
| // Use the new context (created as part of Transfer) and the operation ID | ||
| // passed in from the callers | ||
| opID := op.ID() | ||
| op := trace.NewOperationFromID(d.Transfer.Context(), &opID, "makeDownloadFunc") |
There was a problem hiding this comment.
I'm not sure this shadowing of the op from the outer-outer function is a good idea. Can we make this new operation just after we initialize the new downloadTransfer structure and/or call it something different depending on whether it's supposed to override the outermost op.
| func (ldm *LayerDownloader) makeDownloadFuncFromDownload(layer *ImageWithMeta, sourceDownload, parentDownload *downloadTransfer, layers []*ImageWithMeta) xfer.DoFunc { | ||
| func (ldm *LayerDownloader) makeDownloadFuncFromDownload(op trace.Operation, layer *ImageWithMeta, sourceDownload, parentDownload *downloadTransfer, layers []*ImageWithMeta) xfer.DoFunc { | ||
|
|
||
| return func(progressChan chan<- progress.Progress, start <-chan struct{}, inactive chan<- struct{}) xfer.Transfer { |
There was a problem hiding this comment.
It looks like this function may need exactly the same change as made for makeDownloadFunc above.
There was a problem hiding this comment.
It may need it in the future, but right now "op" is not used inside the body of the returned function
| ic := imagec.NewImageC(options, streamformatter.NewJSONStreamFormatter()) | ||
| if err := ic.PullImage(); err != nil { | ||
| if err := ic.PullImage(op); err != nil { | ||
| log.Fatalf("Pulling image failed due to %s\n", err) |
There was a problem hiding this comment.
It seems like it'd be useful to update the calls to log.* to use op.* in methods where you've introduced an op.
hickeng
left a comment
hickeng
left a comment
There was a problem hiding this comment.
The unit test failures need to be addressed before this can merge: https://ci-vic.vmware.com/vmware/vic/19624
lcastellano
force-pushed
the
props
branch
2 times, most recently
from
bbd42da to
968b660
Compare
(cherry picked from commit a43b205)
4 participants
This PR implements code to pass the Operation ID through the subsystem responsible for downloading images.