Skip to content

Catch or explicitly ignore unhandled errors #105

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

Merged
MarcPaquette merged 1 commit into from
Sep 30, 2024
Merged

Catch or explicitly ignore unhandled errors #105

Show file tree
Hide file tree
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
34 changes: 27 additions & 7 deletions depot/containerstore/credmanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ func (c *credManager) Runner(logger lager.Logger, containerInfoProvider Containe

creds := Credentials{InstanceIdentityCredential: idCred, C2CCredential: c2cCred}
for _, h := range c.handlers {
h.Close(creds, container)
return h.Close(creds, container)
}
return nil
}
Expand Down Expand Up @@ -296,14 +296,24 @@ func (c *credManager) generateInstanceIdentityCred(logger lager.Logger, containe
certificateSAN{IPAddress: ipForCert, OrganizationalUnits: container.CertificateProperties.OrganizationalUnit},
certGUID,
)
var metricErr error
duration := c.clock.Since(start)
if err != nil {
logger.Error("failed-to-generate-instance-identity-credentials", err)
c.metronClient.IncrementCounter(CredCreationFailedCount)
metricErr = c.metronClient.IncrementCounter(CredCreationFailedCount)
if metricErr != nil {
logger.Debug("failed-to-emit-cred-creation-failed-count-metric", lager.Data{"error": metricErr})
}
return Credential{}, err
}
c.metronClient.IncrementCounter(CredCreationSucceededCount)
c.metronClient.SendDuration(CredCreationSucceededDuration, duration)
metricErr = c.metronClient.IncrementCounter(CredCreationSucceededCount)
if metricErr != nil {
logger.Debug("failed-to-emit-cred-creation-succeeded-count-metric", lager.Data{"error": metricErr})
}
metricErr = c.metronClient.SendDuration(CredCreationSucceededDuration, duration)
if metricErr != nil {
logger.Debug("failed-to-emit-cred-creation-succeeded-duration-metric", lager.Data{"error": metricErr})
}

return idCred, nil
}
Expand All @@ -318,13 +328,23 @@ func (c *credManager) generateC2cCred(logger lager.Logger, container executor.Co
certGUID,
)
duration := c.clock.Since(start)
var metricErr error
if err != nil {
logger.Error("failed-to-generate-c2c-credentials", err)
c.metronClient.IncrementCounter(C2CCredCreationFailedCount)
metricErr = c.metronClient.IncrementCounter(C2CCredCreationFailedCount)
if metricErr != nil {
logger.Debug("failed-to-emit-c2c-cred-creation-failed-metric", lager.Data{"error": metricErr})
}
return Credential{}, err
}
c.metronClient.IncrementCounter(C2CCredCreationSucceededCount)
c.metronClient.SendDuration(C2CCredCreationSucceededDuration, duration)
metricErr = c.metronClient.IncrementCounter(C2CCredCreationSucceededCount)
if metricErr != nil {
logger.Debug("failed-to-emit-c2c-cred-creation-succeeded-count-metric", lager.Data{"error": metricErr})
}
metricErr = c.metronClient.SendDuration(C2CCredCreationSucceededDuration, duration)
if metricErr != nil {
logger.Debug("failed-to-emit-c2c-cred-creation-succeeded-duration-metric", lager.Data{"error": metricErr})
}

return c2cCred, nil
}
Expand Down
26 changes: 21 additions & 5 deletions depot/containerstore/dependencymanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,22 @@ func (bm *dependencyManager) downloadCachedDependency(logger lager.Logger, mount
if mount.LogSource != "" {
sourceName = mount.LogSource
}
var metricErr error
if mount.Name != "" {
metronClient.SendAppLog(fmt.Sprintf("Downloading %s...", mount.Name), sourceName, tags)
metricErr = metronClient.SendAppLog(fmt.Sprintf("Downloading %s...", mount.Name), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-appLog", lager.Data{"error": metricErr})
}
}

downloadURL, err := url.Parse(mount.From)
if err != nil {
logger.Error("failed-parsing-bind-mount-download-url", err, lager.Data{"download-url": mount.From, "cache-key": mount.CacheKey})
if mount.Name != "" {
metronClient.SendAppLog(fmt.Sprintf("Downloading %s failed", mount.Name), sourceName, tags)
metricErr = metronClient.SendAppLog(fmt.Sprintf("Downloading %s failed", mount.Name), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-appLog", lager.Data{"error": metricErr})
}
}
return nil, err
}
Expand All @@ -128,19 +135,28 @@ func (bm *dependencyManager) downloadCachedDependency(logger lager.Logger, mount
if err != nil {
logger.Error("failed-fetching-cache-dependency", err, lager.Data{"download-url": downloadURL.String(), "cache-key": mount.CacheKey})
if mount.Name != "" {
metronClient.SendAppLog(fmt.Sprintf("Downloading %s failed", mount.Name), sourceName, tags)
metricErr = metronClient.SendAppLog(fmt.Sprintf("Downloading %s failed", mount.Name), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-appLog", lager.Data{"error": metricErr})
}
}
return nil, err
}
logger.Debug("fetched-cache-dependency", lager.Data{"download-url": downloadURL.String(), "cache-key": mount.CacheKey, "size": downloadedSize})

if downloadedSize != 0 {
if mount.Name != "" {
metronClient.SendAppLog(fmt.Sprintf("Downloaded %s (%s)", mount.Name, bytefmt.ByteSize(uint64(downloadedSize))), sourceName, tags)
metricErr = metronClient.SendAppLog(fmt.Sprintf("Downloaded %s (%s)", mount.Name, bytefmt.ByteSize(uint64(downloadedSize))), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-appLog", lager.Data{"error": metricErr})
}
}
} else {
if mount.Name != "" {
metronClient.SendAppLog(fmt.Sprintf("Downloaded %s", mount.Name), sourceName, tags)
metricErr = metronClient.SendAppLog(fmt.Sprintf("Downloaded %s", mount.Name), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-appLog", lager.Data{"error": metricErr})
}
}
}
return newCachedBindMount(mount.CacheKey, newBindMount(dirPath, mount.To)), nil
Expand Down
40 changes: 32 additions & 8 deletions depot/containerstore/storenode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,14 +255,23 @@ func (n *storeNode) Create(logger lager.Logger, traceID string) error {
}

sourceName, tags := n.info.LogConfig.GetSourceNameAndTagsForLogging()
n.metronClient.SendAppLog(fmt.Sprintf("Cell %s creating container for instance %s", n.cellID, n.Info().Guid), sourceName, tags)
metricErr := n.metronClient.SendAppLog(fmt.Sprintf("Cell %s creating container for instance %s", n.cellID, n.Info().Guid), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-appLog", lager.Data{"error": metricErr})
}
gardenContainer, err := n.createGardenContainer(logger, traceID, &info)
if err != nil {
n.metronClient.SendAppErrorLog(fmt.Sprintf("Cell %s failed to create container for instance %s: %s", n.cellID, n.Info().Guid, err.Error()), sourceName, tags)
metricErr = n.metronClient.SendAppErrorLog(fmt.Sprintf("Cell %s failed to create container for instance %s: %s", n.cellID, n.Info().Guid, err.Error()), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-appErrorLog", lager.Data{"error": metricErr})
}
n.complete(logger, traceID, true, fmt.Sprintf("%s: %s", ContainerCreationFailedMessage, err.Error()), true)
return err
}
n.metronClient.SendAppLog(fmt.Sprintf("Cell %s successfully created container for instance %s", n.cellID, n.Info().Guid), sourceName, tags)
metricErr = n.metronClient.SendAppLog(fmt.Sprintf("Cell %s successfully created container for instance %s", n.cellID, n.Info().Guid), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-appLog", lager.Data{"error": metricErr})
}

n.infoLock.Lock()
n.gardenContainer = gardenContainer
Expand Down Expand Up @@ -566,7 +575,10 @@ func (n *storeNode) completeWithError(logger lager.Logger, traceID string, err e
if err != nil {
errorStr = err.Error()
if errwrap.ContainsType(err, new(steps.ExceededGracefulShutdownIntervalError)) || errwrap.ContainsType(err, new(steps.ExitTimeoutError)) {
n.metronClient.IncrementCounter(ContainerExitedOnTimeoutCount)
metricErr := n.metronClient.IncrementCounter(ContainerExitedOnTimeoutCount)
if metricErr != nil {
logger.Debug("failed-to-emit-container-exited-on-timeout-metric", lager.Data{"error": metricErr})
}
}
}

Expand Down Expand Up @@ -707,7 +719,10 @@ func (n *storeNode) stop(logger lager.Logger, traceID string) {
if n.process != nil {
if !stopped {
sourceName, tags := n.info.LogConfig.GetSourceNameAndTagsForLogging()
n.metronClient.SendAppLog(fmt.Sprintf("Cell %s stopping instance %s", n.cellID, n.Info().Guid), sourceName, tags)
metricErr := n.metronClient.SendAppLog(fmt.Sprintf("Cell %s stopping instance %s", n.cellID, n.Info().Guid), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-app-log", lager.Data{"error": metricErr})
}
}

n.process.Signal(os.Interrupt)
Expand Down Expand Up @@ -741,17 +756,26 @@ func (n *storeNode) Destroy(logger lager.Logger, traceID string) error {

sourceName, tags := n.info.LogConfig.GetSourceNameAndTagsForLogging()

n.metronClient.SendAppLog(fmt.Sprintf("Cell %s destroying container for instance %s", n.cellID, info.Guid), sourceName, tags)
metricErr := n.metronClient.SendAppLog(fmt.Sprintf("Cell %s destroying container for instance %s", n.cellID, info.Guid), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-app-log", lager.Data{"error": metricErr})
}
// ensure these directories are removed even if the container fails to destroy
defer n.removeCredsDir(logger, info)
defer n.umountVolumeMounts(logger, info)

err := n.destroyContainer(logger, traceID)
if err != nil {
n.metronClient.SendAppLog(fmt.Sprintf("Cell %s failed to destroy container for instance %s", n.cellID, info.Guid), sourceName, tags)
metricErr = n.metronClient.SendAppLog(fmt.Sprintf("Cell %s failed to destroy container for instance %s", n.cellID, info.Guid), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-app-log", lager.Data{"error": metricErr})
}
return err
}
n.metronClient.SendAppLog(fmt.Sprintf("Cell %s successfully destroyed container for instance %s", n.cellID, info.Guid), sourceName, tags)
metricErr = n.metronClient.SendAppLog(fmt.Sprintf("Cell %s successfully destroyed container for instance %s", n.cellID, info.Guid), sourceName, tags)
if metricErr != nil {
logger.Debug("failed-to-emit-app-log", lager.Data{"error": metricErr})
}

cacheKeys := n.bindMountCacheKeys

Expand Down
1 change: 1 addition & 0 deletions depot/log_streamer/log_rate_limiter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ func (r *logRateLimiter) emitMetrics() {
case <-t.C:
lastIntervalEmitted := atomic.SwapUint64(&r.bytesEmittedLastInterval, 0)
perSecondValue := float64(lastIntervalEmitted) / intervalDivider
// #nosec G104 - ignore error returned by sending metric here
r.metronClient.SendAppLogRate(perSecondValue, float64(r.maxLogBytesPerSecond), r.tags)
case <-r.ctx.Done():
return
Expand Down
30 changes: 24 additions & 6 deletions depot/steps/emit_progress_step.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,30 @@ func NewEmitProgress(

func (step *emitProgressStep) Run(signals <-chan os.Signal, ready chan<- struct{}) error {
if step.startMessage != "" {
step.streamer.Stdout().Write([]byte(step.startMessage + "\n"))
_, writeErr := step.streamer.Stdout().Write([]byte(step.startMessage + "\n"))
if writeErr != nil {
step.logger.Debug("failed-writing-to-stdout", lager.Data{"error": writeErr})
}
}

err := step.substep.Run(signals, ready)

if err != nil {
if step.failureMessage != "" {
step.streamer.Stderr().Write([]byte(step.failureMessage))
_, writeErr := step.streamer.Stderr().Write([]byte(step.failureMessage))
if writeErr != nil {
step.logger.Debug("failed-writing-to-stderr", lager.Data{"error": writeErr})
}

if emittableError, ok := err.(*EmittableError); ok {
step.streamer.Stderr().Write([]byte(": "))
step.streamer.Stderr().Write([]byte(emittableError.Error()))
_, writeErr := step.streamer.Stderr().Write([]byte(": "))
if writeErr != nil {
step.logger.Debug("failed-writing-to-stderr", lager.Data{"error": writeErr})
}
_, writeErr = step.streamer.Stderr().Write([]byte(emittableError.Error()))
if writeErr != nil {
step.logger.Debug("failed-writing-to-stderr", lager.Data{"error": writeErr})
}

wrappedMessage := ""
if emittableError.WrappedError() != nil {
Expand All @@ -63,11 +75,17 @@ func (step *emitProgressStep) Run(signals <-chan os.Signal, ready chan<- struct{
})
}

step.streamer.Stderr().Write([]byte("\n"))
_, writeErr = step.streamer.Stderr().Write([]byte("\n"))
if writeErr != nil {
step.logger.Debug("failed-writing-to-stderr", lager.Data{"error": writeErr})
}
}
} else {
if step.successMessage != "" {
step.streamer.Stdout().Write([]byte(step.successMessage + "\n"))
_, writeErr := step.streamer.Stdout().Write([]byte(step.successMessage + "\n"))
if writeErr != nil {
step.logger.Debug("failed-writing-to-stdout", lager.Data{"error": writeErr})
}
}
}

Expand Down
5 changes: 4 additions & 1 deletion depot/steps/run_step.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,10 @@ func (step *runStep) networkingEnvVars() []string {

func (step *runStep) writeToStdOut(message string) {
if !step.model.SuppressLogOutput {
step.streamer.Stdout().Write([]byte(message))
_, err := step.streamer.Stdout().Write([]byte(message))
if err != nil {
step.logger.Debug("failed-writing-to-stdout", lager.Data{"error": err})
}
step.streamer.Flush()
}
}
10 changes: 8 additions & 2 deletions depot/steps/upload_step.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,14 @@ func (step *uploadStep) Run(signals <-chan os.Signal, ready chan<- struct{}) (er
}
finalFileLocation := tempFile.Name()
defer func() {
tempFile.Close()
os.Remove(finalFileLocation)
deferErr := tempFile.Close()
if deferErr != nil {
step.logger.Debug("failed-to-close-file", lager.Data{"error": deferErr, "file": finalFileLocation})
}
deferErr = os.Remove(finalFileLocation)
if deferErr != nil {
step.logger.Debug("failed-to-remove-file", lager.Data{"error": deferErr, "file": finalFileLocation})
}
}()

// #nosec - G110 - We're fine with unbounded file decompression here as we have container filesystem quotas that will prevent this from eating up the entire diego cell disk space
Expand Down
5 changes: 4 additions & 1 deletion gardenhealth/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ func (r *Runner) Run(signals <-chan os.Signal, ready chan<- struct{}) error {
case <-healthcheckTimeout.C():
r.setUnhealthy(logger)
r.checker.Cancel(logger)
r.metronClient.SendMetric(CellUnhealthyMetric, 1)
err := r.metronClient.SendMetric(CellUnhealthyMetric, 1)
if err != nil {
logger.Debug("failed-to-emit-cell-unhealth-metric", lager.Data{"error": err})
}

case <-emitInterval.C():
r.emitUnhealthyCellMetric(logger)
Expand Down
5 changes: 4 additions & 1 deletion initializer/initializer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,8 +583,11 @@ func closeHub(logger lager.Logger, hub event.Hub) ifrit.Runner {
return ifrit.RunFunc(func(signals <-chan os.Signal, ready chan<- struct{}) error {
close(ready)
signal := <-signals
hub.Close()
err := hub.Close()
hubLogger := logger.Session("close-hub")
if err != nil {
logger.Error("failed-to-close-hub", err)
}
hubLogger.Info("signalled", lager.Data{"signal": signal.String()})
return nil
})
Expand Down