ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/nomad
File: client/allocrunner/health_hook.go (Go)

Code before:
	// cancelFn stops the health watching/setting goroutine. Wait on
	// watchLock to block until the watcher exits.
	cancelFn context.CancelFunc

	// alloc set by new func or Update. Must hold hookLock to access.
	alloc *structs.Allocation

	// isDeploy is true if monitoring a deployment. Set in init(). Must
	// hold hookLock to access.
	isDeploy bool

	logger hclog.Logger
}

func newAllocHealthWatcherHook(
	logger hclog.Logger,
	alloc *structs.Allocation,
	hs healthSetter,
	listener *cstructs.AllocListener,
	consul serviceregistration.Handler,
	checkStore checkstore.Shim,
) interfaces.RunnerHook {

	// Neither deployments nor migrations care about the health of
	// non-service jobs so never watch their health
	if !slices.Contains([]string{structs.JobTypeService, structs.JobTypeSystem}, alloc.Job.Type) {
		return noopAllocHealthWatcherHook{}
	}

	// Initialize watchDone with a closed chan in case Update runs before Prerun
	closedDone := make(chan struct{})
	close(closedDone)

	h := &allocHealthWatcherHook{
		alloc:        alloc,
		cancelFn:     func() {}, // initialize to prevent nil func panics
		watchDone:    closedDone,
		consul:       consul,
		checkStore:   checkStore,
		healthSetter: hs,
		listener:     listener,
	}

	h.logger = logger.Named(h.Name())
	return h
}

// statically assert the hook implements the expected interfaces
var (
	_ interfaces.RunnerPrerunHook  = (*allocHealthWatcherHook)(nil)
	_ interfaces.RunnerPostrunHook = (*allocHealthWatcherHook)(nil)

Review comment (suggestion):
You can do 

```go
switch alloc.JobType {
	case structs.JobTypeService, structs.JobTypeSystem:
		// ok
	default:
		return noopAllocHealthWatcherHook{}
}
```

And then you don't have to allocate a slice and iterate it.

Code after:
	// watchLock to block until the watcher exits.
	cancelFn context.CancelFunc

	// alloc set by new func or Update. Must hold hookLock to access.
	alloc *structs.Allocation

	// isDeploy is true if monitoring a deployment. Set in init(). Must
	// hold hookLock to access.
	isDeploy bool

	logger hclog.Logger
}

func newAllocHealthWatcherHook(
	logger hclog.Logger,
	alloc *structs.Allocation,
	hs healthSetter,
	listener *cstructs.AllocListener,
	consul serviceregistration.Handler,
	checkStore checkstore.Shim,
) interfaces.RunnerHook {

	// Neither deployments nor migrations care about the health of
	// non-service jobs so never watch their health
	switch alloc.Job.Type {
	case structs.JobTypeService, structs.JobTypeSystem:
	default:
		return noopAllocHealthWatcherHook{}
	}

	// Initialize watchDone with a closed chan in case Update runs before Prerun
	closedDone := make(chan struct{})
	close(closedDone)

	h := &allocHealthWatcherHook{
		alloc:        alloc,
		cancelFn:     func() {}, // initialize to prevent nil func panics
		watchDone:    closedDone,
		consul:       consul,
		checkStore:   checkStore,
		healthSetter: hs,
		listener:     listener,
	}

	h.logger = logger.Named(h.Name())
	return h
}

// statically assert the hook implements the expected interfaces
var (
	_ interfaces.RunnerPrerunHook  = (*allocHealthWatcherHook)(nil)
Repository: hashicorp/nomad
File: client/allocrunner/taskrunner/getter/util.go (Go)

Code before:
		}

		// Check that entry is still within sandbox
		if !isPathWithin(rootDir, toCheck) {
			return ErrSandboxEscape
		}

		return nil
	}
	return walkFn, nil
}

// isPathWithin checks if the toCheckPath is within the rootPath. It
// uses the os.SameFile function to perform the path check so paths
// are compared appropriately based on the filesystem.
func isPathWithin(rootPath, toCheckPath string) bool {
	rootPath = filepath.Clean(rootPath)
	toCheckPath = filepath.Clean(toCheckPath)

	if len(rootPath) > len(toCheckPath) {
		return false
	}

	rootStat, err := os.Stat(rootPath)
	if err != nil {
		return false
	}

	checkStat, err := os.Stat(toCheckPath[0:len(rootPath)])
	if err != nil {
		return false
	}

	return os.SameFile(rootStat, checkStat)
}


Review comment (bug):
there being multiple ways to get `false`, but no way to tell which route it took, can make it difficult to debug later.

what if this method returned an `error`, and line 270 could wrap/join it with `ErrSandboxEscape`? is there some reason we would want to conceal this information from users?

Code after:
		}

		toCheck, err := filepath.Abs(resolved)
		if err != nil {
			return err
		}

		// Check that entry is still within sandbox
		isWithin, err := isPathWithin(rootDir, toCheck)
		if err != nil {
			return err
		}

		if !isWithin {
			return ErrSandboxEscape
		}

		return nil
	}
	return walkFn, nil
}

// isPathWithin checks if the toCheckPath is within the rootPath. It
// uses the os.SameFile function to perform the path check so paths
// are compared appropriately based on the filesystem.
func isPathWithin(rootPath, toCheckPath string) (bool, error) {
	rootPath = filepath.Clean(rootPath)
	toCheckPath = filepath.Clean(toCheckPath)

	if len(rootPath) > len(toCheckPath) {
		return false, nil
	}

	rootStat, err := os.Stat(rootPath)
	if err != nil {
		return false, err
	}

	checkStat, err := os.Stat(toCheckPath[0:len(rootPath)])
	if err != nil {
		return false, err
	}

	return os.SameFile(rootStat, checkStat), nil
}
Repository: hashicorp/nomad
File: client/allocrunner/taskrunner/getter/util_test.go (Go)

Code before:

func TestUtil_isPathWithin(t *testing.T) {
	tdir := t.TempDir()
	t.Run("when path not within root", func(t *testing.T) {
		root := mktmpdir(t, tdir)
		check := mktmpdir(t, tdir)

		must.False(t, isPathWithin(root, check))
	})

	t.Run("when path within root", func(t *testing.T) {
		root := mktmpdir(t, tdir)
		check := mktmpdir(t, root)

		must.True(t, isPathWithin(root, check))
	})

	t.Run("when root within path", func(t *testing.T) {
		check := mktmpdir(t, tdir)
		root := mktmpdir(t, check)

		must.False(t, isPathWithin(root, check))
	})
}

func mktmpdir(t *testing.T, parent string) string {
	dir, err := os.MkdirTemp(parent, "path-within")
	must.NoError(t, err, must.Sprint("failed to create temporary directory"))
	return dir
}


Review comment (suggestion):
I believe you've reinvented [`t.TempDir`](https://pkg.go.dev/testing#T.TempDir) -- there are some other `os.MkdirTemp`s around, too, which could use `t.TempDir` instead.

Code after:
}

func TestUtil_isPathWithin(t *testing.T) {
	tdir := t.TempDir()
	pathFn := func(parent string) string {
		dir, err := os.MkdirTemp(parent, "testing-path")
		must.NoError(t, err, must.Sprint("failed to create temporary directory"))
		return dir
	}

	t.Run("when path not within root", func(t *testing.T) {
		root := pathFn(tdir)
		check := pathFn(tdir)
		result, err := isPathWithin(root, check)

		must.NoError(t, err)
		must.False(t, result)
	})

	t.Run("when path within root", func(t *testing.T) {
		root := pathFn(tdir)
		check := pathFn(root)
		result, err := isPathWithin(root, check)

		must.NoError(t, err)
		must.True(t, result)
	})

	t.Run("when root within path", func(t *testing.T) {
		check := pathFn(tdir)
		root := pathFn(check)
		result, err := isPathWithin(root, check)

		must.NoError(t, err)
		must.False(t, result)
	})

	t.Run("when path does not exist", func(t *testing.T) {
		root := filepath.Join(tdir, "missing")
		check := filepath.Join(root, "unknown")
		result, err := isPathWithin(root, check)

		must.ErrorContains(t, err, "no such file or directory")
		must.False(t, result)
	})
}
Repository: hashicorp/nomad
File: scheduler/reconciler/reconcile_node.go (Go)

Code before:
				Alloc:     alloc,
			})
			continue
		}

		// If the definition is updated we need to update
		if job.JobModifyIndex != alloc.Job.JobModifyIndex {
			if canariesPerTG[tg.Name] > 0 {
				isCanarying[tg.Name] = true
				if canaryNode[tg.Name] {
					result.Update = append(result.Update, AllocTuple{
						Name:      name,
						TaskGroup: tg,
						Alloc:     alloc,
						Canary:    true,
					})
					desiredCanaries[tg.Name] += 1
					desiredTotal[tg.Name] += 1
				}
			} else {
				result.Update = append(result.Update, AllocTuple{
					Name:      name,
					TaskGroup: tg,
					Alloc:     alloc,
				})
				desiredTotal[tg.Name] += 1
			}
			continue
		}

		// Everything is up-to-date
	IGNORE:
		result.Ignore = append(result.Ignore, AllocTuple{
			Name:      name,
			TaskGroup: tg,
			Alloc:     alloc,
		})
	}

	// as we iterate over require groups, we'll keep track of whether the deployment
	// is complete or not
	deploymentComplete := false

	// Scan the required groups
	for name, tg := range required {

		// populate deployment state for this task group
		var dstate = new(structs.DeploymentState)
		var existingDeployment bool
		if nr.DeploymentCurrent != nil {
			dstate, existingDeployment = nr.DeploymentCurrent.TaskGroups[tg.Name]

Review comment (suggestion):
`DesiredTotal` should be the _end goal_ of the deployment, but we're setting it based on the number of updates.

For example, if I have 10 nodes, 2 with already-running allocs, and max_parallel=2, I should see `DesiredTotal=10` and `PlacedAllocs=4` by the time I'm done. But I'm seeing `DesiredTotal=8` and `PlacedAllocs=2` instead.

Code after:
			})
			continue
		}

		// If the definition is updated we need to update
		if job.JobModifyIndex != alloc.Job.JobModifyIndex {
			if canariesPerTG[tg.Name] > 0 {
				isCanarying[tg.Name] = true
				if canaryNode[tg.Name] {
					result.Update = append(result.Update, AllocTuple{
						Name:      name,
						TaskGroup: tg,
						Alloc:     alloc,
						Canary:    true,
					})
					desiredCanaries[tg.Name] += 1
				}
			} else {
				result.Update = append(result.Update, AllocTuple{
					Name:      name,
					TaskGroup: tg,
					Alloc:     alloc,
				})
			}
			continue
		}

		// Everything is up-to-date
	IGNORE:
		result.Ignore = append(result.Ignore, AllocTuple{
			Name:      name,
			TaskGroup: tg,
			Alloc:     alloc,
		})
	}

	// as we iterate over require groups, we'll keep track of whether the deployment
	// is complete or not
	deploymentComplete := false

	// Scan the required groups
	for name, tg := range required {

		// populate deployment state for this task group
		var dstate = new(structs.DeploymentState)
		var existingDeployment bool
		if nr.DeploymentCurrent != nil {
			dstate, existingDeployment = nr.DeploymentCurrent.TaskGroups[tg.Name]
		}

		if !existingDeployment {
Repository: hashicorp/nomad
File: command/agent/csi_endpoint.go (Go)

Code before:
	}

	return out.Volume, nil
}

func (s *HTTPServer) csiVolumeRegister(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
	switch req.Method {
	case http.MethodPost, http.MethodPut:
	default:
		return nil, CodedError(405, ErrInvalidMethod)
	}

	args := structs.CSIVolumeRegisterRequest{}
	if err := decodeBody(req, &args); err != nil {
		return err, CodedError(400, err.Error())
	}
	s.parseWriteRequest(req, &args.WriteRequest)

	var out structs.CSIVolumeRegisterResponse
	if err := s.agent.RPC("CSIVolume.Register", &args, &out); err != nil {
		return nil, err
	}

	setMeta(resp, &out.QueryMeta)

	return resp, nil
}

func (s *HTTPServer) csiVolumeCreate(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
	switch req.Method {
	case http.MethodPost, http.MethodPut:
	default:
		return nil, CodedError(405, ErrInvalidMethod)
	}

	args := structs.CSIVolumeCreateRequest{}
	if err := decodeBody(req, &args); err != nil {
		return err, CodedError(400, err.Error())
	}
	s.parseWriteRequest(req, &args.WriteRequest)

	var out structs.CSIVolumeCreateResponse
	if err := s.agent.RPC("CSIVolume.Create", &args, &out); err != nil {
		return nil, err
	}

	setMeta(resp, &out.QueryMeta)

	return out, nil
}


Review comment (suggestion):
`resp` is non-`nil`, but it's quite odd to return the `http.ResponseWriter` like this.

this has returned `nil` for ages -- I wonder why it just recently started causing issues...

Code after:
	}

	return out.Volume, nil
}

func (s *HTTPServer) csiVolumeRegister(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
	switch req.Method {
	case http.MethodPost, http.MethodPut:
	default:
		return nil, CodedError(405, ErrInvalidMethod)
	}

	args := structs.CSIVolumeRegisterRequest{}
	if err := decodeBody(req, &args); err != nil {
		return err, CodedError(400, err.Error())
	}
	s.parseWriteRequest(req, &args.WriteRequest)

	var out structs.CSIVolumeRegisterResponse
	if err := s.agent.RPC("CSIVolume.Register", &args, &out); err != nil {
		return nil, err
	}

	setMeta(resp, &out.QueryMeta)

	return out, nil
}

func (s *HTTPServer) csiVolumeCreate(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
	switch req.Method {
	case http.MethodPost, http.MethodPut:
	default:
		return nil, CodedError(405, ErrInvalidMethod)
	}

	args := structs.CSIVolumeCreateRequest{}
	if err := decodeBody(req, &args); err != nil {
		return err, CodedError(400, err.Error())
	}
	s.parseWriteRequest(req, &args.WriteRequest)

	var out structs.CSIVolumeCreateResponse
	if err := s.agent.RPC("CSIVolume.Create", &args, &out); err != nil {
		return nil, err
	}

	setMeta(resp, &out.QueryMeta)

	return out, nil
}
Repository: hashicorp/nomad
File: client/allocrunner/alloc_runner.go (Go)

Code before:
	return ar.allocDir
}

// Restore state from database. Must be called after NewAllocRunner but before
// Run.
func (ar *allocRunner) Restore() error {
	// Retrieve deployment status to avoid reseting it across agent
	// restarts. Once a deployment status is set Nomad no longer monitors
	// alloc health, so we must persist deployment state across restarts.
	ds, err := ar.stateDB.GetDeploymentStatus(ar.id)
	if err != nil {
		return err
	}

	ns, err := ar.stateDB.GetNetworkStatus(ar.id)
	if err != nil {
		return err
	}

	ar.stateLock.Lock()
	ar.state.DeploymentStatus = ds
	ar.stateLock.Unlock()

	// sets the network status in alloc state along with setting
	// the status for each taskrunner
	ar.SetNetworkStatus(ns)

	states := make(map[string]*structs.TaskState)

	// Restore task runners
	for _, tr := range ar.tasks {
		if err := tr.Restore(); err != nil {
			return err
		}
		states[tr.Task().Name] = tr.TaskState()

		// restore process wrangler for task
		ar.wranglers.Setup(proclib.Task{AllocID: tr.Alloc().ID, Task: tr.Task().Name})

		// restore cpuset partition state
		ar.restoreCores(tr.Alloc().AllocatedResources)
	}

	ar.taskCoordinator.Restore(states)

	return nil
}

// restoreCores will restore the cpuset partitions with the reserved core
// data for each task in the alloc
func (ar *allocRunner) restoreCores(res *structs.AllocatedResources) {

Review comment (suggestion):
Testing this is proving challenging because you need to check the taskRunner's envBuilder, which is a private field.

Code after:
	return ar.allocDir
}

// Restore state from database. Must be called after NewAllocRunner but before
// Run.
func (ar *allocRunner) Restore() error {
	// Retrieve deployment status to avoid reseting it across agent
	// restarts. Once a deployment status is set Nomad no longer monitors
	// alloc health, so we must persist deployment state across restarts.
	ds, err := ar.stateDB.GetDeploymentStatus(ar.id)
	if err != nil {
		return err
	}

	ns, err := ar.stateDB.GetNetworkStatus(ar.id)
	if err != nil {
		return err
	}

	ar.stateLock.Lock()
	ar.state.DeploymentStatus = ds
	ar.state.NetworkStatus = ns
	ar.stateLock.Unlock()

	states := make(map[string]*structs.TaskState)

	// Restore task runners
	for _, tr := range ar.tasks {
		if err := tr.Restore(); err != nil {
			return err
		}
		states[tr.Task().Name] = tr.TaskState()

		// restore process wrangler for task
		ar.wranglers.Setup(proclib.Task{AllocID: tr.Alloc().ID, Task: tr.Task().Name})

		// restore cpuset partition state
		ar.restoreCores(tr.Alloc().AllocatedResources)
	}

	ar.taskCoordinator.Restore(states)

	return nil
}

// restoreCores will restore the cpuset partitions with the reserved core
// data for each task in the alloc
func (ar *allocRunner) restoreCores(res *structs.AllocatedResources) {
	for _, taskRes := range res.Tasks {
		s := idset.From[hw.CoreID](taskRes.Cpu.ReservedCores)
		ar.partitions.Restore(s)
Repository: hashicorp/nomad
File: client/allocrunner/network_hook_test.go (Go)

Code before:
)

type mockNetworkIsolationSetter struct {
	t            *testing.T
	expectedSpec *drivers.NetworkIsolationSpec
	called       bool
}

func (m *mockNetworkIsolationSetter) SetNetworkIsolation(spec *drivers.NetworkIsolationSpec) {
	m.called = true
	test.Eq(m.t, m.expectedSpec, spec)
}

type mockNetworkStatus struct {
	t              *testing.T
	expectedStatus *structs.AllocNetworkStatus
	calls          int
}

func (m *mockNetworkStatus) SetNetworkStatus(status *structs.AllocNetworkStatus) {
	m.calls++
	test.Eq(m.t, m.expectedStatus, status)
}

func (m *mockNetworkStatus) NetworkStatus() *structs.AllocNetworkStatus {
	m.calls++
	return m.expectedStatus
}

// Test that the prerun and postrun hooks call the setter with the expected
// NetworkIsolationSpec for group bridge network.
func TestNetworkHook_Prerun_Postrun_group(t *testing.T) {
	ci.Parallel(t)

	alloc := mock.Alloc()
	alloc.Job.TaskGroups[0].Networks = []*structs.NetworkResource{
		{Mode: "bridge"},
	}

	spec := &drivers.NetworkIsolationSpec{
		Mode:   drivers.NetIsolationModeGroup,
		Path:   "test",
		Labels: map[string]string{"abc": "123"},
	}

	destroyCalled := false
	nm := &testutils.MockDriver{
		MockNetworkManager: testutils.MockNetworkManager{
			CreateNetworkF: func(allocID string, req *drivers.NetworkCreateRequest) (*drivers.NetworkIsolationSpec, bool, error) {
				test.Eq(t, alloc.ID, allocID)
				return spec, true, nil

Review comment (suggestion):
in general, I like to distinguish between the different methods when counting, to remove ambiguity in test assertions.  e.g. `getCalls` and `setCalls`

Code after:
)

type mockNetworkIsolationSetter struct {
	t            *testing.T
	expectedSpec *drivers.NetworkIsolationSpec
	called       bool
}

func (m *mockNetworkIsolationSetter) SetNetworkIsolation(spec *drivers.NetworkIsolationSpec) {
	m.called = true
	test.Eq(m.t, m.expectedSpec, spec)
}

type mockNetworkStatus struct {
	t              *testing.T
	expectedStatus *structs.AllocNetworkStatus
	getCalls       int
	setCalls       int
}

func (m *mockNetworkStatus) SetNetworkStatus(status *structs.AllocNetworkStatus) {
	m.setCalls++
	test.Eq(m.t, m.expectedStatus, status)
}

func (m *mockNetworkStatus) NetworkStatus() *structs.AllocNetworkStatus {
	m.getCalls++
	return m.expectedStatus
}

// Test that the prerun and postrun hooks call the setter with the expected
// NetworkIsolationSpec for group bridge network.
func TestNetworkHook_Prerun_Postrun_group(t *testing.T) {
	ci.Parallel(t)

	alloc := mock.Alloc()
	alloc.Job.TaskGroups[0].Networks = []*structs.NetworkResource{
		{Mode: "bridge"},
	}

	spec := &drivers.NetworkIsolationSpec{
		Mode:   drivers.NetIsolationModeGroup,
		Path:   "test",
		Labels: map[string]string{"abc": "123"},
	}

	destroyCalled := false
	nm := &testutils.MockDriver{
		MockNetworkManager: testutils.MockNetworkManager{
			CreateNetworkF: func(allocID string, req *drivers.NetworkCreateRequest) (*drivers.NetworkIsolationSpec, bool, error) {
				test.Eq(t, alloc.ID, allocID)
Repository: hashicorp/nomad
File: command/agent/node_identity_endpoint_test.go (Go)

Code before:
				http.MethodConnect,
				http.MethodDelete,
				http.MethodHead,
				http.MethodOptions,
				http.MethodPatch,
				http.MethodPost,
				http.MethodPut,
				http.MethodTrace,
			}

			for _, method := range badMethods {
				req, err := http.NewRequest(method, "/v1/client/identity", nil)
				must.NoError(t, err)

				_, err = s.Server.NodeIdentityGetRequest(respW, req)
				must.ErrorContains(t, err, "Invalid method")

				codedErr, ok := err.(HTTPCodedError)
				must.True(t, ok)
				must.Eq(t, http.StatusMethodNotAllowed, codedErr.Code())
				must.Eq(t, ErrInvalidMethod, codedErr.Error())
			}
		})
	})

	t.Run("400 no node query param", func(t *testing.T) {
		httpTest(t, nil, func(s *TestAgent) {

			respW := httptest.NewRecorder()

			req, err := http.NewRequest(
				http.MethodGet,
				"/v1/client/identity?node_id="+uuid.Generate(),
				nil,
			)
			must.NoError(t, err)

			_, err = s.Server.NodeIdentityGetRequest(respW, req)
			must.ErrorContains(t, err, "Unknown node")
		})
	})

	t.Run("200 ok query param", func(t *testing.T) {

		// Enable the client, so we have something to renew.
		configFn := func(c *Config) { c.Client.Enabled = true }

		httpTest(t, configFn, func(s *TestAgent) {

			respW := httptest.NewRecorder()


Review comment (suggestion):
There is a node query parameter, it's just not one that exists. The order of the wording here makes it ambiguous.

```suggestion
	t.Run("400 query param with unknown node", func(t *testing.T) {
```

Code after:
				http.MethodConnect,
				http.MethodDelete,
				http.MethodHead,
				http.MethodOptions,
				http.MethodPatch,
				http.MethodPost,
				http.MethodPut,
				http.MethodTrace,
			}

			for _, method := range badMethods {
				req, err := http.NewRequest(method, "/v1/client/identity", nil)
				must.NoError(t, err)

				_, err = s.Server.NodeIdentityGetRequest(respW, req)
				must.ErrorContains(t, err, "Invalid method")

				codedErr, ok := err.(HTTPCodedError)
				must.True(t, ok)
				must.Eq(t, http.StatusMethodNotAllowed, codedErr.Code())
				must.Eq(t, ErrInvalidMethod, codedErr.Error())
			}
		})
	})

	t.Run("400 query param with unknown node", func(t *testing.T) {
		httpTest(t, nil, func(s *TestAgent) {

			respW := httptest.NewRecorder()

			req, err := http.NewRequest(
				http.MethodGet,
				"/v1/client/identity?node_id="+uuid.Generate(),
				nil,
			)
			must.NoError(t, err)

			_, err = s.Server.NodeIdentityGetRequest(respW, req)
			must.ErrorContains(t, err, "Unknown node")
		})
	})

	t.Run("200 ok query param", func(t *testing.T) {

		// Enable the client, so we have something to renew.
		configFn := func(c *Config) { c.Client.Enabled = true }

		httpTest(t, configFn, func(s *TestAgent) {

			respW := httptest.NewRecorder()