Lint fixes part 1

Author: NikhilSinha1

internal/checkpointer/checkpointer.go

@@ -31,14 +31,14 @@ const (
 	checkpointSubdirName = "checkpoint"
 )
 
-var errNoCheckpointDir = errors.New("Could not find checkpoint directory environment variable")
+var errNoCheckpointDir = errors.New("could not find checkpoint directory environment variable")
 
-type FatalCheckpointErr struct {
+type FatalCheckpointError struct {
 	err error
 }
 
-func (e *FatalCheckpointErr) Error() string {
-	return e.Error()
+func (e *FatalCheckpointError) Error() string {
+	return e.err.Error()
 }
 
 type Checkpointer interface {
@@ -113,7 +113,7 @@ func (c *checkpointer) Checkpoint(ctx context.Context, cogletCmd *exec.Cmd) erro
 		return errNoCheckpointDir
 	}
 
-	err := os.MkdirAll(filepath.Join(c.checkpointDir, checkpointSubdirName), 0o666)
+	err := os.MkdirAll(filepath.Join(c.checkpointDir, checkpointSubdirName), 0o666) //nolint:gosec // coglet needs to write here
 	if err != nil {
 		return err
 	}
@@ -137,7 +137,7 @@ func (c *checkpointer) Checkpoint(ctx context.Context, cogletCmd *exec.Cmd) erro
 	cudaCmd := strings.TrimSpace(string(data))
 
 	// Write said command to a file for later
-	err = os.WriteFile(filepath.Join(c.checkpointDir, cudaCmdFileName), []byte(cudaCmd), 0o666)
+	err = os.WriteFile(filepath.Join(c.checkpointDir, cudaCmdFileName), []byte(cudaCmd), 0o644) //nolint:gosec
 	if err != nil {
 		return err
 	}
@@ -157,7 +157,7 @@ func (c *checkpointer) Checkpoint(ctx context.Context, cogletCmd *exec.Cmd) erro
 		cmd = exec.CommandContext(ctx, cudaCheckpointPath, "--toggle", "--pid", string(cudaPID))
 		if cudaErr := cmd.Run(); cudaErr != nil {
 			// Return a fatal error so upstream knows we cannot continue in the current state
-			return &FatalCheckpointErr{
+			return &FatalCheckpointError{
 				err: cudaErr,
 			}
 		}
@@ -171,7 +171,7 @@ func (c *checkpointer) Checkpoint(ctx context.Context, cogletCmd *exec.Cmd) erro
 	cmd = exec.CommandContext(ctx, cudaCheckpointPath, "--toggle", "--pid", string(cudaPID))
 	if err := cmd.Run(); err != nil {
 		// Return a fatal error so upstream knows we cannot continue in the current state
-		return &FatalCheckpointErr{
+		return &FatalCheckpointError{
 			err: err,
 		}
 	}
@@ -200,7 +200,7 @@ func (c *checkpointer) Restore(ctx context.Context) (*exec.Cmd, func(context.Con
 		if err != nil {
 			// If this command failed, we want to best effort try to kill the started process,
 			// since we'll start a new one
-			restoreCmd.Process.Kill()
+			restoreCmd.Process.Kill() //nolint:errcheck
 
 			return err
 		}
@@ -210,7 +210,7 @@ func (c *checkpointer) Restore(ctx context.Context) (*exec.Cmd, func(context.Con
 		if err := cmd.Run(); err != nil {
 			// If this command failed, we want to best effort try to kill the started process,
 			// since we'll start a new one
-			restoreCmd.Process.Kill()
+			restoreCmd.Process.Kill() //nolint:errcheck
 
 			return err
 		}
@@ -219,7 +219,7 @@ func (c *checkpointer) Restore(ctx context.Context) (*exec.Cmd, func(context.Con
 		if err != nil {
 			// If this command failed, we want to best effort try to kill the started process,
 			// since we'll start a new one
-			restoreCmd.Process.Kill()
+			restoreCmd.Process.Kill() //nolint:errcheck
 
 			return err
 		}
@@ -246,6 +246,5 @@ func downloadCUDACheckpointBinaries(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("failed to download and untar CRIU: %w", err)
 	}
-	updateEnvVar("LD_LIBRARY_PATH", filepath.Join(dir, "criu-lib"))
-	return nil
+	return updateEnvVar("LD_LIBRARY_PATH", filepath.Join(dir, "criu-lib"))
 }

internal/checkpointer/utils.go

@@ -12,24 +12,26 @@ import (
 	"time"
 )
 
-var errTimedOutPolling = errors.New("Timed out while polling for file")
+var errTimedOutPolling = errors.New("timed out while polling for file")
 
 // updateEnvVar updates an environment variable in-place, adding an item to it
 // if it exists or creating it if it doesn't exist
-func updateEnvVar(envVarName, newItem string) {
+func updateEnvVar(envVarName, newItem string) error {
 	old := os.Getenv(envVarName)
 	if old == "" {
-		os.Setenv(envVarName, newItem)
-		return
+		return os.Setenv(envVarName, newItem)
 	}
 	path := newItem + string(os.PathListSeparator) + os.Getenv(envVarName)
-	os.Setenv(envVarName, path)
+	return os.Setenv(envVarName, path)
 }
 
 // downloadFile downloads a file from the URL provided to the path provided
 func downloadFile(url, path string) error {
 	filename := filepath.Base(path)
-	os.MkdirAll(filepath.Dir(path), 0o755)
+	err := os.MkdirAll(filepath.Dir(path), 0o600)
+	if err != nil {
+		return err
+	}
 
 	resp, err := http.Get(url)
 	if err != nil {
@@ -41,13 +43,13 @@ func downloadFile(url, path string) error {
 		return fmt.Errorf("failed to download %s: %w", filename, err)
 	}
 
-	binary, err := os.Create(path)
+	file, err := os.Create(path)
 	if err != nil {
 		return fmt.Errorf("failed to touch file: %w", err)
 	}
-	defer binary.Close()
+	defer file.Close() //nolint: errcheck
 
-	_, err = io.Copy(binary, resp.Body)
+	_, err = io.Copy(file, resp.Body)
 	if err != nil {
 		return fmt.Errorf("failed to save %s: %w", filename, err)
 	}
@@ -63,7 +65,7 @@ func downloadAndChmod(url, path string) error {
 		return err
 	}
 
-	if err := os.Chmod(path, 0o755); err != nil {
+	if err := os.Chmod(path, 0o600); err != nil {
 		return fmt.Errorf("failed to chmod file: %w", err)
 	}
 	return nil
@@ -116,37 +118,16 @@ func pollForFileDeletion(target string, timeout, pollInterval time.Duration) err
 	}
 }
 
-// pollForFileExistance waits for a file to exist, up until a timeout. It returns an error if the
-// timeout is hit
-func pollForFileExistance(target string, timeout, pollInterval time.Duration) error {
-	deadline := time.After(timeout)
-
-	for {
-		// Check if the file exists, if it doesn't keep looping
-		if _, err := os.Stat(target); err == nil {
-			return nil
-		}
-
-		// Check for timeout before sleeping for the polling interval
-		select {
-		case <-deadline:
-			return errTimedOutPolling
-		default:
-			time.Sleep(pollInterval)
-		}
-	}
-}
-
 // https://stackoverflow.com/a/30708914/30548878
 func isDirEmpty(name string) (bool, error) {
 	f, err := os.Open(name)
 	if err != nil {
 		return false, err
 	}
-	defer f.Close()
+	defer f.Close() //nolint:errcheck
 
 	_, err = f.Readdirnames(1)
-	if err == io.EOF {
+	if errors.Is(err, io.EOF) {
 		return true, nil
 	}
 	return false, err
@@ -155,7 +136,7 @@ func isDirEmpty(name string) (bool, error) {
 // Touch a file if it doesn't exist, otherwise wipes the contents of the file
 func touchFile(name string) error {
 	// Ensure upstream directory exists for file
-	err := os.MkdirAll(filepath.Dir(name), 0o755)
+	err := os.MkdirAll(filepath.Dir(name), 0o644)
 	if err != nil {
 		return err
 	}
@@ -164,8 +145,7 @@ func touchFile(name string) error {
 	if err != nil {
 		return err
 	}
-	f.Close()
-	return nil
+	return f.Close()
 }
 
 // setStatusReady ensures the ready files exist

internal/runner/manager.go

@@ -356,6 +356,7 @@ func (m *Manager) createDefaultRunner(ctx context.Context) (*Runner, error) {
 	// Derive the runtime context from the manager's context
 	runtimeContext, runtimeCancel := context.WithCancel(ctx)
 
+commandSetup:
 	cmd := exec.CommandContext(runtimeContext, pythonPath, args...) //nolint:gosec // expected subprocess launched with variable
 	runner, err := m.setupRunner(runtimeContext, runtimeCancel, cmd, env, runnerCtx, cogYaml.Concurrency.Max)
 
@@ -374,12 +375,18 @@ func (m *Manager) createDefaultRunner(ctx context.Context) (*Runner, error) {
 
 	if !cp.HasCheckpoint() {
 		err := cp.Checkpoint(ctx, cmd)
-		var fatalCheckpointErr *checkpointer.FatalCheckpointErr
-		if errors.As(err, &fatalCheckpointErr) {
-			return nil, fmt.Errorf("fatal error while trying to checkpoint: %w", err)
+		var FatalCheckpointError *checkpointer.FatalCheckpointError
+		// If we saw an error that would leave the runner unusable, turn off the
+		// checkpointer and recreate the command and runner
+		if errors.As(err, &FatalCheckpointError) {
+			// TODO: Is this bad? Should we just return the error back up?
+			// The main concern is what `runner.Config` does leaving artifacts
+			// between runs, although I think that should be fine?
+			cp.Disable()
+			goto commandSetup
 		}
 		// If the error is not fatal, we failed to create a checkpoint but are still
-		// running the original cog process, so we can just continue as if we did
+		// running the cog process successfully, so we can just continue as if we did
 		// nothing
 	}
 

python/coglet/__main__.py

@@ -54,7 +54,7 @@ def main() -> int:
     parser = argparse.ArgumentParser()
     parser.add_argument('--name', metavar='NAME', required=True, help='name')
     group = parser.add_mutually_exclusive_group()
-    group.add_argument('--ipc-url', metavar='URL', required=True, help='IPC URL')
+    group.add_argument('--ipc-url', metavar='URL', help='IPC URL')
     group.add_argument('--signal_mode', action='store_true')
     parser.add_argument(
         '--working-dir', metavar='DIR', required=True, help='working directory'