Skip to content

[image-builder] Avoid returning "Unknown" #20854

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

Merged
merged 3 commits into from
Jun 2, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
152 changes: 0 additions & 152 deletions .clinerules/learning-journal.md

This file was deleted.

50 changes: 39 additions & 11 deletions components/image-builder-mk3/pkg/orchestrator/orchestrator.go
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
BaseRef: req.BaseImageNameResolved,
})
if err != nil {
return err
return handleFailedBuildStreamResponse(err, "cannot send build response")
}
return nil
}
Expand Down Expand Up @@ -307,7 +307,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
BaseRef: baseref,
})
if err != nil {
return err
return handleFailedBuildStreamResponse(err, "cannot send build response")
}
return nil
}
Expand All @@ -322,11 +322,12 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil

randomUUID, err := uuid.NewRandom()
if err != nil {
return
return status.Errorf(codes.Internal, "failed to generate build ID: %v", err)
}
buildID := randomUUID.String()
log := log.WithField("buildID", buildID)

var (
buildID = randomUUID.String()
buildBase = "false"
contextPath = "."
dockerfilePath = "Dockerfile"
Expand Down Expand Up @@ -368,7 +369,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil

pbaseref, err := reference.ParseNormalizedNamed(baseref)
if err != nil {
return xerrors.Errorf("cannot parse baseref: %v", err)
return status.Errorf(codes.InvalidArgument, "cannot parse baseref: %v", err)
}
bobBaseref := "localhost:8080/base"
if r, ok := pbaseref.(reference.Digested); ok {
Expand All @@ -384,7 +385,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
})
additionalAuth, err = json.Marshal(ath)
if err != nil {
return xerrors.Errorf("cannot marshal additional auth: %w", err)
return status.Errorf(codes.InvalidArgument, "cannot marshal additional auth: %v", err)
}
}

Expand Down Expand Up @@ -432,7 +433,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
Name: "WORKSPACEKIT_BOBPROXY_ADDITIONALAUTH",
Value: string(additionalAuth),
},
{Name: "SUPERVISOR_DEBUG_ENABLE", Value: fmt.Sprintf("%v", log.Log.Logger.IsLevelEnabled(logrus.DebugLevel))},
{Name: "SUPERVISOR_DEBUG_ENABLE", Value: fmt.Sprintf("%v", log.Logger.IsLevelEnabled(logrus.DebugLevel))},
},
},
Type: wsmanapi.WorkspaceType_IMAGEBUILD,
Expand Down Expand Up @@ -476,8 +477,8 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil

err := resp.Send(update)
if err != nil {
log.WithError(err).Error("cannot forward build update - dropping listener")
return status.Errorf(codes.Unknown, "cannot send update: %v", err)
log.WithError(err).Info("cannot forward build update - dropping listener")
return handleFailedBuildStreamResponse(err, "cannot send update")
}

if update.Status == protocol.BuildStatus_done_failure || update.Status == protocol.BuildStatus_done_success {
Expand Down Expand Up @@ -555,8 +556,8 @@ func (o *Orchestrator) Logs(req *protocol.LogsRequest, resp protocol.ImageBuilde

err := resp.Send(update)
if err != nil {
log.WithError(err).Error("cannot forward log output - dropping listener")
return status.Errorf(codes.Unknown, "cannot send log output: %v", err)
log.WithError(err).Info("cannot forward log output - dropping listener")
return handleFailedBuildStreamResponse(err, "cannot send log output")
}
}

Expand Down Expand Up @@ -709,6 +710,33 @@ func (o *Orchestrator) getWorkspaceImageRef(ctx context.Context, baseref string)
return fmt.Sprintf("%s:%x", o.Config.WorkspaceImageRepository, dst), nil
}

func handleFailedBuildStreamResponse(err error, msg string) error {
if err == nil {
// OK is OK
return nil
}

// If the error is a context.DeadlineExceeded, we return nil (OK) as requested.
if errors.Is(err, context.DeadlineExceeded) {
// Return nil (OK) for DeadlineExceeded
return nil
}

// If it's already a gRPC status error, check for DeadlineExceeded
if st, ok := status.FromError(err); ok {
if st.Code() == codes.DeadlineExceeded {
// Return nil (OK) for DeadlineExceeded as requested
return nil
}

log.WithError(err).WithField("code", status.Code(err)).Error(fmt.Sprintf("unexpected error while sending build response: %s", msg))
return err
}

log.WithError(err).Error(fmt.Sprintf("unexpected error while sending build response: %s", msg))
return status.Errorf(codes.Unavailable, "%s: %v", msg, err)
}

// parentCantCancelContext is a bit of a hack. We have some operations which we want to keep alive even after clients
// disconnect. gRPC cancels the context once a client disconnects, thus we intercept the cancelation and act as if
// nothing had happened.
Expand Down
11 changes: 9 additions & 2 deletions components/ws-manager-mk2/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ import (
//+kubebuilder:scaffold:imports
)

// PROXIED_GRPC_SERVICE_PREFIX is the prefix used for proxied gRPC services.
// It is used to avoid conflicts with the original service names in metrics, and to filter out proxied services in SLIs etc.
const PROXIED_GRPC_SERVICE_PREFIX = "proxied"

var (
// ServiceName is the name we use for tracing/logging
ServiceName = "ws-manager-mk2"
Expand Down Expand Up @@ -258,9 +262,12 @@ func setupGRPCService(cfg *config.ServiceConfiguration, k8s client.Client, maint
grpcMetrics.EnableHandlingTimeHistogram()
metrics.Registry.MustRegister(grpcMetrics)

// Create interceptor for prefixing the proxied gRPC service names, to avoid conflicts with the original service names in metrics
grpcServicePrefixer := &imgproxy.ServiceNamePrefixerInterceptor{Prefix: PROXIED_GRPC_SERVICE_PREFIX}

grpcOpts := common_grpc.ServerOptionsWithInterceptors(
[]grpc.StreamServerInterceptor{grpcMetrics.StreamServerInterceptor()},
[]grpc.UnaryServerInterceptor{grpcMetrics.UnaryServerInterceptor(), ratelimits.UnaryInterceptor()},
[]grpc.StreamServerInterceptor{grpcServicePrefixer.StreamServerInterceptor(), grpcMetrics.StreamServerInterceptor()},
[]grpc.UnaryServerInterceptor{grpcServicePrefixer.UnaryServerInterceptor(), grpcMetrics.UnaryServerInterceptor(), ratelimits.UnaryInterceptor()},
)
if cfg.RPCServer.TLS.CA != "" && cfg.RPCServer.TLS.Certificate != "" && cfg.RPCServer.TLS.PrivateKey != "" {
tlsConfig, err := common_grpc.ClientAuthTLSConfig(
Expand Down
48 changes: 46 additions & 2 deletions components/ws-manager-mk2/pkg/proxy/imagebuilder.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,14 @@ package proxy

import (
"context"
"errors"
"io"

"github.com/gitpod-io/gitpod/common-go/log"
"github.com/gitpod-io/gitpod/image-builder/api"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/proto"
)

Expand Down Expand Up @@ -58,7 +64,7 @@ func forwardStream[R ProtoMessage](ctx context.Context, recv func() (R, error),
for {
resp, err := recv()
if err != nil {
return err
return handleProxyError(err)
}

// generic hack, can't compare R to nil because R's default value is unclear (not even sure this is nil)
Expand All @@ -69,9 +75,47 @@ func forwardStream[R ProtoMessage](ctx context.Context, recv func() (R, error),
}
err = send(resp)
if err != nil {
return err
return handleProxyError(err)
}
}

return nil
}

// handleProxyError ensures all errors have proper gRPC status codes
func handleProxyError(err error) error {
if err == nil {
return nil
}

// If it's already a gRPC status error, check for DeadlineExceeded
if st, ok := status.FromError(err); ok {
if st.Code() == codes.DeadlineExceeded {
// Return nil (OK) for DeadlineExceeded as requested
return nil
}

log.WithError(err).WithField("code", status.Code(err)).Error("unexpected error while sending stream response upstream")
return err
}

// Handle context errors
if errors.Is(err, context.DeadlineExceeded) {
// Return nil (OK) for DeadlineExceeded
return nil
}

if errors.Is(err, io.EOF) {
// Return nil (OK) for EOF, which is a normal when the client ends the stream
return nil
}

log.WithError(err).Error("unexpected error while sending stream response upstream")

if errors.Is(err, context.Canceled) {
return status.Error(codes.Canceled, err.Error())
}

// Wrap any other error as Internal
return status.Error(codes.Internal, err.Error())
}
Loading
Loading