Files
nomad/client/allocrunner/network_hook_test.go
Tim Gross e168548341 provide allocrunner hooks with prebuilt taskenv and fix mutation bugs (#25373)
Some of our allocrunner hooks require a task environment for interpolating values based on the node or allocation. But several of the hooks accept an already-built environment or builder and then keep that in memory. Both of these retain a copy of all the node attributes and allocation metadata, which balloons memory usage until the allocation is GC'd.

While we'd like to look into ways to avoid keeping the allocrunner around entirely (see #25372), for now we can significantly reduce memory usage by creating the task environment on-demand when calling allocrunner methods, rather than persisting it in the allocrunner hooks.

In doing so, we uncover two other bugs:
* The WID manager, the group service hook, and the checks hook have to interpolate services for specific tasks. They mutated a taskenv builder to do so, but each time they mutate the builder, they write to the same environment map. When a group has multiple tasks, it's possible for one task to set an environment variable that would then be interpolated in the service definition for another task if that task did not have that environment variable. Only the service definition interpolation is impacted. This does not leak env vars across running tasks, as each taskrunner has its own builder.

  To fix this, we move the `UpdateTask` method off the builder and onto the taskenv as the `WithTask` method. This makes a shallow copy of the taskenv with a deep clone of the environment map used for interpolation, and then overwrites the environment from the task.

* The checks hook interpolates Nomad native service checks only on `Prerun` and not on `Update`. This could cause unexpected deregistration and registration of checks during in-place updates. To fix this, we make sure we interpolate in the `Update` method.

I also bumped into an incorrectly implemented interface in the CSI hook. I've pulled that and some better guardrails out to https://github.com/hashicorp/nomad/pull/25472.

Fixes: https://github.com/hashicorp/nomad/issues/25269
Fixes: https://hashicorp.atlassian.net/browse/NET-12310
Ref: https://github.com/hashicorp/nomad/issues/25372
2025-03-24 12:05:04 -04:00

280 lines
8.5 KiB
Go

// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1
package allocrunner
import (
"fmt"
"testing"
"github.com/hashicorp/nomad/ci"
"github.com/hashicorp/nomad/client/taskenv"
"github.com/hashicorp/nomad/helper/testlog"
"github.com/hashicorp/nomad/nomad/mock"
"github.com/hashicorp/nomad/nomad/structs"
"github.com/hashicorp/nomad/plugins/drivers"
"github.com/hashicorp/nomad/plugins/drivers/testutils"
"github.com/hashicorp/nomad/testutil"
"github.com/shoenig/test"
"github.com/shoenig/test/must"
)
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 mockNetworkStatusSetter struct {
t *testing.T
expectedStatus *structs.AllocNetworkStatus
called bool
}
func (m *mockNetworkStatusSetter) SetNetworkStatus(status *structs.AllocNetworkStatus) {
m.called = true
test.Eq(m.t, m.expectedStatus, status)
}
// 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
},
DestroyNetworkF: func(allocID string, netSpec *drivers.NetworkIsolationSpec) error {
destroyCalled = true
test.Eq(t, alloc.ID, allocID)
test.Eq(t, spec, netSpec)
return nil
},
},
}
setter := &mockNetworkIsolationSetter{
t: t,
expectedSpec: spec,
}
statusSetter := &mockNetworkStatusSetter{
t: t,
expectedStatus: nil,
}
logger := testlog.HCLogger(t)
hook := newNetworkHook(logger, setter, alloc, nm, &hostNetworkConfigurator{}, statusSetter)
env := taskenv.NewBuilder(mock.Node(), alloc, nil, alloc.Job.Region).Build()
must.NoError(t, hook.Prerun(env))
must.True(t, setter.called)
must.False(t, destroyCalled)
must.NoError(t, hook.Postrun())
must.True(t, destroyCalled)
}
// Test that prerun and postrun hooks do not expect a NetworkIsolationSpec
func TestNetworkHook_Prerun_Postrun_host(t *testing.T) {
ci.Parallel(t)
alloc := mock.Alloc()
alloc.Job.TaskGroups[0].Networks = []*structs.NetworkResource{
{Mode: "host"},
}
destroyCalled := false
nm := &testutils.MockDriver{
MockNetworkManager: testutils.MockNetworkManager{
CreateNetworkF: func(allocID string, req *drivers.NetworkCreateRequest) (*drivers.NetworkIsolationSpec, bool, error) {
test.Unreachable(t, test.Sprintf("should not call CreateNetwork for host network"))
return nil, false, nil
},
DestroyNetworkF: func(allocID string, netSpec *drivers.NetworkIsolationSpec) error {
destroyCalled = true
test.Nil(t, netSpec)
return nil
},
},
}
setter := &mockNetworkIsolationSetter{
t: t,
expectedSpec: nil,
}
statusSetter := &mockNetworkStatusSetter{
t: t,
expectedStatus: nil,
}
logger := testlog.HCLogger(t)
hook := newNetworkHook(logger, setter, alloc, nm, &hostNetworkConfigurator{}, statusSetter)
env := taskenv.NewBuilder(mock.Node(), alloc, nil, alloc.Job.Region).Build()
must.NoError(t, hook.Prerun(env))
must.False(t, setter.called)
must.False(t, destroyCalled)
must.NoError(t, hook.Postrun())
must.True(t, destroyCalled)
}
// TestNetworkHook_Prerun_Postrun_ExistingNetNS tests that the prerun and
// postrun hooks call the Setup and Destroy with the expected behaviors when the
// network namespace already exists (typical of agent restarts and host reboots)
func TestNetworkHook_Prerun_Postrun_ExistingNetNS(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"},
}
isolationSetter := &mockNetworkIsolationSetter{t: t, expectedSpec: spec}
statusSetter := &mockNetworkStatusSetter{t: t, expectedStatus: nil}
callCounts := testutil.NewCallCounter()
nm := &testutils.MockDriver{
MockNetworkManager: testutils.MockNetworkManager{
CreateNetworkF: func(allocID string, req *drivers.NetworkCreateRequest) (*drivers.NetworkIsolationSpec, bool, error) {
test.Eq(t, alloc.ID, allocID)
callCounts.Inc("CreateNetwork")
return spec, false, nil
},
DestroyNetworkF: func(allocID string, netSpec *drivers.NetworkIsolationSpec) error {
test.Eq(t, alloc.ID, allocID)
test.Eq(t, spec, netSpec)
callCounts.Inc("DestroyNetwork")
return nil
},
},
}
fakePlugin := newMockCNIPlugin()
configurator := &cniNetworkConfigurator{
nodeAttrs: map[string]string{
"plugins.cni.version.bridge": "1.6.1",
},
nodeMeta: map[string]string{},
logger: testlog.HCLogger(t),
cni: fakePlugin,
nsOpts: &nsOpts{},
}
env := taskenv.NewBuilder(mock.Node(), alloc, nil, alloc.Job.Region).Build()
testCases := []struct {
name string
cniVersion string
checkErrs []error
setupErrs []string
expectPrerunCreateNetworkCalls int
expectPrerunDestroyNetworkCalls int
expectCheckCalls int
expectSetupCalls int
expectPostrunDestroyNetworkCalls int
expectPrerunError string
}{
{
name: "good check",
cniVersion: "1.6.1",
expectPrerunCreateNetworkCalls: 1,
expectPrerunDestroyNetworkCalls: 0,
expectCheckCalls: 1,
expectSetupCalls: 0,
expectPostrunDestroyNetworkCalls: 1,
},
{
name: "initial check fails",
cniVersion: "1.6.1",
checkErrs: []error{fmt.Errorf("whatever")},
expectPrerunCreateNetworkCalls: 2,
expectPrerunDestroyNetworkCalls: 1,
expectCheckCalls: 2,
expectSetupCalls: 0,
expectPostrunDestroyNetworkCalls: 2,
},
{
name: "check fails twice",
cniVersion: "1.6.1",
checkErrs: []error{
fmt.Errorf("whatever"),
fmt.Errorf("whatever"),
},
expectPrerunCreateNetworkCalls: 2,
expectPrerunDestroyNetworkCalls: 1,
expectCheckCalls: 2,
expectSetupCalls: 0,
expectPostrunDestroyNetworkCalls: 2,
expectPrerunError: "failed to configure networking for alloc: network namespace already exists but was misconfigured: whatever",
},
{
name: "old CNI version skips check",
cniVersion: "1.2.0",
expectPrerunCreateNetworkCalls: 1,
expectPrerunDestroyNetworkCalls: 0,
expectCheckCalls: 0,
expectSetupCalls: 0,
expectPostrunDestroyNetworkCalls: 1,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
callCounts.Reset()
fakePlugin.counter.Reset()
fakePlugin.checkErrors = tc.checkErrs
configurator.nodeAttrs["plugins.cni.version.bridge"] = tc.cniVersion
hook := newNetworkHook(testlog.HCLogger(t), isolationSetter,
alloc, nm, configurator, statusSetter)
err := hook.Prerun(env)
if tc.expectPrerunError == "" {
must.NoError(t, err)
} else {
must.EqError(t, err, tc.expectPrerunError)
}
test.Eq(t, tc.expectPrerunDestroyNetworkCalls,
callCounts.Get()["DestroyNetwork"], test.Sprint("DestroyNetwork calls after prerun"))
test.Eq(t, tc.expectPrerunCreateNetworkCalls,
callCounts.Get()["CreateNetwork"], test.Sprint("CreateNetwork calls after prerun"))
test.Eq(t, tc.expectCheckCalls, fakePlugin.counter.Get()["Check"], test.Sprint("Check calls"))
test.Eq(t, tc.expectSetupCalls, fakePlugin.counter.Get()["Setup"], test.Sprint("Setup calls"))
must.NoError(t, hook.Postrun())
test.Eq(t, tc.expectPostrunDestroyNetworkCalls,
callCounts.Get()["DestroyNetwork"], test.Sprint("DestroyNetwork calls after postrun"))
})
}
}