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

v3: Analyze layer content in parallel #741

Merged
merged 1 commit into from
Mar 20, 2019
Merged

v3: Analyze layer content in parallel #741

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 41 additions & 23 deletions api/v3/rpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
package v3

import (
"sync"

"golang.org/x/net/context"
"golang.org/x/sync/errgroup"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

Expand Down Expand Up @@ -83,32 +86,47 @@ func (s *AncestryServer) PostAncestry(ctx context.Context, req *pb.PostAncestryR
}

builder := clair.NewAncestryBuilder(clair.EnabledDetectors())
for _, layer := range req.Layers {
if layer == nil {
err := status.Error(codes.InvalidArgument, "ancestry layer is invalid")
return nil, err
}

if layer.GetHash() == "" {
return nil, status.Error(codes.InvalidArgument, "ancestry layer hash should not be empty")
}

if layer.GetPath() == "" {
return nil, status.Error(codes.InvalidArgument, "ancestry layer path should not be empty")
layerMap := map[string]*database.Layer{}
layerMapLock := sync.RWMutex{}
g, analyzerCtx := errgroup.WithContext(ctx)
for i := range req.Layers {
layer := req.Layers[i]
if _, ok := layerMap[layer.Hash]; !ok {
layerMap[layer.Hash] = nil
if layer == nil {
err := status.Error(codes.InvalidArgument, "ancestry layer is invalid")
return nil, err
}

if layer.GetHash() == "" {
return nil, status.Error(codes.InvalidArgument, "ancestry layer hash should not be empty")
}

if layer.GetPath() == "" {
return nil, status.Error(codes.InvalidArgument, "ancestry layer path should not be empty")
}

g.Go(func() error {
clairLayer, err := clair.AnalyzeLayer(analyzerCtx, s.Store, layer.Hash, req.Format, layer.Path, layer.Headers)
if err != nil {
return err
}

layerMapLock.Lock()
layerMap[layer.Hash] = clairLayer
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a lock even though the keys are independent because of races when the runtime is expanding the size of the map.

Even if we added the size to the make,layerMap := make(map[string]*database.Layer, len(req.Layers)), I wouldn't trust this race without a lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense

layerMapLock.Unlock()

return nil
})
}
}

// TODO(sidac): make AnalyzeLayer to be async to ensure
// non-blocking downloads.
// We'll need to deal with two layers post by the same or different
// requests that may have the same hash. In that case, since every
// layer/feature/namespace is unique in the database, it may introduce
// deadlock.
clairLayer, err := clair.AnalyzeLayer(ctx, s.Store, layer.Hash, req.Format, layer.Path, layer.Headers)
if err != nil {
return nil, newRPCErrorWithClairError(codes.Internal, err)
}
if err = g.Wait(); err != nil {
return nil, newRPCErrorWithClairError(codes.Internal, err)
}

builder.AddLeafLayer(clairLayer)
for _, layer := range req.Layers {
builder.AddLeafLayer(layerMap[layer.Hash])
}

if err := clair.SaveAncestry(s.Store, builder.Ancestry(req.AncestryName)); err != nil {
Expand Down