diff --git a/.changelog/25472.txt b/.changelog/25472.txt new file mode 100644 index 000000000..dc3bd4b31 --- /dev/null +++ b/.changelog/25472.txt @@ -0,0 +1,3 @@ +```release-note:bug +csi: Fixed a bug where in-flight CSI RPCs would not be cancelled on client GC or dev agent shutdown +``` diff --git a/client/allocrunner/allocdir_hook.go b/client/allocrunner/allocdir_hook.go index f575be44a..e49b74ac4 100644 --- a/client/allocrunner/allocdir_hook.go +++ b/client/allocrunner/allocdir_hook.go @@ -6,6 +6,7 @@ package allocrunner import ( "github.com/hashicorp/go-hclog" "github.com/hashicorp/nomad/client/allocdir" + "github.com/hashicorp/nomad/client/allocrunner/interfaces" ) // allocDirHook creates and destroys the root directory and shared directories @@ -23,6 +24,12 @@ func newAllocDirHook(logger hclog.Logger, allocDir allocdir.Interface) *allocDir return ad } +// statically assert that the hook meets the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*allocDirHook)(nil) + _ interfaces.RunnerDestroyHook = (*allocDirHook)(nil) +) + func (h *allocDirHook) Name() string { return "alloc_dir" } diff --git a/client/allocrunner/checks_hook.go b/client/allocrunner/checks_hook.go index 39da47832..bb6cea16f 100644 --- a/client/allocrunner/checks_hook.go +++ b/client/allocrunner/checks_hook.go @@ -113,6 +113,13 @@ func newChecksHook( return h } +// statically assert that the hook meets the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*checksHook)(nil) + _ interfaces.RunnerUpdateHook = (*checksHook)(nil) + _ interfaces.RunnerPreKillHook = (*checksHook)(nil) +) + // initialize the dynamic fields of checksHook, which is to say setup all the // observers and query context things associated with the alloc. // diff --git a/client/allocrunner/checks_hook_test.go b/client/allocrunner/checks_hook_test.go index 948e6e287..bb7355f67 100644 --- a/client/allocrunner/checks_hook_test.go +++ b/client/allocrunner/checks_hook_test.go @@ -25,12 +25,6 @@ import ( "github.com/shoenig/test/must" ) -var ( - _ interfaces.RunnerPrerunHook = (*checksHook)(nil) - _ interfaces.RunnerUpdateHook = (*checksHook)(nil) - _ interfaces.RunnerPreKillHook = (*checksHook)(nil) -) - func makeCheckStore(logger hclog.Logger) checkstore.Shim { db := state.NewMemDB(logger) checkStore := checkstore.NewStore(logger, db) diff --git a/client/allocrunner/consul_grpc_sock_hook.go b/client/allocrunner/consul_grpc_sock_hook.go index 99132a1ca..50dc3f1bd 100644 --- a/client/allocrunner/consul_grpc_sock_hook.go +++ b/client/allocrunner/consul_grpc_sock_hook.go @@ -100,6 +100,13 @@ func newConsulGRPCSocketHook( } } +// statically assert that the hook meets the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*consulGRPCSocketHook)(nil) + _ interfaces.RunnerUpdateHook = (*consulGRPCSocketHook)(nil) + _ interfaces.RunnerPostrunHook = (*consulGRPCSocketHook)(nil) +) + func (*consulGRPCSocketHook) Name() string { return consulGRPCSockHookName } diff --git a/client/allocrunner/consul_hook.go b/client/allocrunner/consul_hook.go index b4e087e06..f47b9f992 100644 --- a/client/allocrunner/consul_hook.go +++ b/client/allocrunner/consul_hook.go @@ -11,6 +11,7 @@ import ( log "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-multierror" "github.com/hashicorp/nomad/client/allocdir" + "github.com/hashicorp/nomad/client/allocrunner/interfaces" "github.com/hashicorp/nomad/client/consul" cstructs "github.com/hashicorp/nomad/client/structs" "github.com/hashicorp/nomad/client/taskenv" @@ -70,6 +71,14 @@ func newConsulHook(cfg consulHookConfig) *consulHook { return h } +// statically assert the hook implements the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*consulHook)(nil) + _ interfaces.RunnerPostrunHook = (*consulHook)(nil) + _ interfaces.RunnerDestroyHook = (*consulHook)(nil) + _ interfaces.ShutdownHook = (*consulHook)(nil) +) + func (*consulHook) Name() string { return "consul" } diff --git a/client/allocrunner/consul_hook_test.go b/client/allocrunner/consul_hook_test.go index 68e0c166e..c24b915e4 100644 --- a/client/allocrunner/consul_hook_test.go +++ b/client/allocrunner/consul_hook_test.go @@ -11,7 +11,6 @@ import ( consulapi "github.com/hashicorp/consul/api" "github.com/hashicorp/nomad/ci" - "github.com/hashicorp/nomad/client/allocrunner/interfaces" "github.com/hashicorp/nomad/client/consul" cstate "github.com/hashicorp/nomad/client/state" cstructs "github.com/hashicorp/nomad/client/structs" @@ -24,9 +23,6 @@ import ( "github.com/shoenig/test/must" ) -// statically assert consul hook implements the expected interfaces -var _ interfaces.RunnerPrerunHook = (*consulHook)(nil) - func consulHookTestHarness(t *testing.T) *consulHook { logger := testlog.HCLogger(t) diff --git a/client/allocrunner/consul_http_sock_hook.go b/client/allocrunner/consul_http_sock_hook.go index b817e7aa8..86278b768 100644 --- a/client/allocrunner/consul_http_sock_hook.go +++ b/client/allocrunner/consul_http_sock_hook.go @@ -74,6 +74,13 @@ func newConsulHTTPSocketHook( } } +// statically assert the hook implements the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*consulHTTPSockHook)(nil) + _ interfaces.RunnerPostrunHook = (*consulHTTPSockHook)(nil) + _ interfaces.RunnerUpdateHook = (*consulHTTPSockHook)(nil) +) + func (*consulHTTPSockHook) Name() string { return consulHTTPSocketHookName } diff --git a/client/allocrunner/cpuparts_hook.go b/client/allocrunner/cpuparts_hook.go index 7cac0f0f4..b9e545b03 100644 --- a/client/allocrunner/cpuparts_hook.go +++ b/client/allocrunner/cpuparts_hook.go @@ -5,6 +5,7 @@ package allocrunner import ( "github.com/hashicorp/go-hclog" + "github.com/hashicorp/nomad/client/allocrunner/interfaces" "github.com/hashicorp/nomad/client/lib/cgroupslib" "github.com/hashicorp/nomad/client/lib/idset" "github.com/hashicorp/nomad/client/lib/numalib/hw" @@ -46,6 +47,12 @@ func (h *cpuPartsHook) Name() string { return cpuPartsHookName } +// statically assert the hook implements the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*cpuPartsHook)(nil) + _ interfaces.RunnerPostrunHook = (*cpuPartsHook)(nil) +) + func (h *cpuPartsHook) Prerun() error { return h.partitions.Reserve(h.reservations) } diff --git a/client/allocrunner/csi_hook.go b/client/allocrunner/csi_hook.go index 775a06912..1f0fe1810 100644 --- a/client/allocrunner/csi_hook.go +++ b/client/allocrunner/csi_hook.go @@ -12,6 +12,7 @@ import ( hclog "github.com/hashicorp/go-hclog" multierror "github.com/hashicorp/go-multierror" + "github.com/hashicorp/nomad/client/allocrunner/interfaces" "github.com/hashicorp/nomad/client/allocrunner/state" "github.com/hashicorp/nomad/client/config" "github.com/hashicorp/nomad/client/dynamicplugins" @@ -76,6 +77,14 @@ func newCSIHook(alloc *structs.Allocation, logger hclog.Logger, csi csimanager.M } } +// statically assert the hook implements the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*csiHook)(nil) + _ interfaces.RunnerPostrunHook = (*csiHook)(nil) + _ interfaces.RunnerDestroyHook = (*csiHook)(nil) + _ interfaces.ShutdownHook = (*csiHook)(nil) +) + func (c *csiHook) Name() string { return "csi_hook" } @@ -541,7 +550,6 @@ func (c *csiHook) unmountImpl(result *volumePublishResult) error { // stopping. Cancel our shutdown context so that we don't block client // shutdown while in the CSI RPC retry loop. func (c *csiHook) Shutdown() { - c.logger.Trace("shutting down hook") c.shutdownCancelFn() } @@ -549,7 +557,7 @@ func (c *csiHook) Shutdown() { // or when a -dev mode client is stopped. Cancel our shutdown context // so that we don't block client shutdown while in the CSI RPC retry // loop. -func (c *csiHook) Destroy() { - c.logger.Trace("destroying hook") +func (c *csiHook) Destroy() error { c.shutdownCancelFn() + return nil } diff --git a/client/allocrunner/csi_hook_test.go b/client/allocrunner/csi_hook_test.go index 457ad1020..68f0d306f 100644 --- a/client/allocrunner/csi_hook_test.go +++ b/client/allocrunner/csi_hook_test.go @@ -11,7 +11,6 @@ import ( "time" "github.com/hashicorp/nomad/ci" - "github.com/hashicorp/nomad/client/allocrunner/interfaces" "github.com/hashicorp/nomad/client/allocrunner/state" "github.com/hashicorp/nomad/client/pluginmanager/csimanager" cstructs "github.com/hashicorp/nomad/client/structs" @@ -24,9 +23,6 @@ import ( "github.com/shoenig/test/must" ) -var _ interfaces.RunnerPrerunHook = (*csiHook)(nil) -var _ interfaces.RunnerPostrunHook = (*csiHook)(nil) - func TestCSIHook(t *testing.T) { ci.Parallel(t) diff --git a/client/allocrunner/fail_hook.go b/client/allocrunner/fail_hook.go index 3891a6193..0c03d6184 100644 --- a/client/allocrunner/fail_hook.go +++ b/client/allocrunner/fail_hook.go @@ -56,7 +56,16 @@ func (h *FailHook) LoadConfig(path string) *FailHook { return h } -var _ interfaces.RunnerPrerunHook = &FailHook{} +// statically assert the hook implements the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*FailHook)(nil) + _ interfaces.RunnerPreKillHook = (*FailHook)(nil) + _ interfaces.RunnerPostrunHook = (*FailHook)(nil) + _ interfaces.RunnerDestroyHook = (*FailHook)(nil) + _ interfaces.RunnerUpdateHook = (*FailHook)(nil) + _ interfaces.RunnerTaskRestartHook = (*FailHook)(nil) + _ interfaces.ShutdownHook = (*FailHook)(nil) +) func (h *FailHook) Prerun() error { if h.Fail.Prerun { @@ -65,16 +74,12 @@ func (h *FailHook) Prerun() error { return nil } -var _ interfaces.RunnerPreKillHook = &FailHook{} - func (h *FailHook) PreKill() { if h.Fail.PreKill { h.logger.Error("prekill", "error", ErrFailHookError) } } -var _ interfaces.RunnerPostrunHook = &FailHook{} - func (h *FailHook) Postrun() error { if h.Fail.Postrun { return fmt.Errorf("postrun %w", ErrFailHookError) @@ -82,8 +87,6 @@ func (h *FailHook) Postrun() error { return nil } -var _ interfaces.RunnerDestroyHook = &FailHook{} - func (h *FailHook) Destroy() error { if h.Fail.Destroy { return fmt.Errorf("destroy %w", ErrFailHookError) @@ -91,8 +94,6 @@ func (h *FailHook) Destroy() error { return nil } -var _ interfaces.RunnerUpdateHook = &FailHook{} - func (h *FailHook) Update(request *interfaces.RunnerUpdateRequest) error { if h.Fail.Update { return fmt.Errorf("update %w", ErrFailHookError) @@ -100,8 +101,6 @@ func (h *FailHook) Update(request *interfaces.RunnerUpdateRequest) error { return nil } -var _ interfaces.RunnerTaskRestartHook = &FailHook{} - func (h *FailHook) PreTaskRestart() error { if h.Fail.PreTaskRestart { return fmt.Errorf("destroy %w", ErrFailHookError) @@ -109,8 +108,6 @@ func (h *FailHook) PreTaskRestart() error { return nil } -var _ interfaces.ShutdownHook = &FailHook{} - func (h *FailHook) Shutdown() { if h.Fail.Shutdown { h.logger.Error("shutdown", "error", ErrFailHookError) diff --git a/client/allocrunner/group_service_hook.go b/client/allocrunner/group_service_hook.go index 879a56108..289d7d361 100644 --- a/client/allocrunner/group_service_hook.go +++ b/client/allocrunner/group_service_hook.go @@ -118,6 +118,15 @@ func newGroupServiceHook(cfg groupServiceHookConfig) *groupServiceHook { return h } +// statically assert the hook implements the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*groupServiceHook)(nil) + _ interfaces.RunnerPreKillHook = (*groupServiceHook)(nil) + _ interfaces.RunnerPostrunHook = (*groupServiceHook)(nil) + _ interfaces.RunnerUpdateHook = (*groupServiceHook)(nil) + _ interfaces.RunnerTaskRestartHook = (*groupServiceHook)(nil) +) + func (*groupServiceHook) Name() string { return groupServiceHookName } diff --git a/client/allocrunner/group_service_hook_test.go b/client/allocrunner/group_service_hook_test.go index bc2134119..90866fde6 100644 --- a/client/allocrunner/group_service_hook_test.go +++ b/client/allocrunner/group_service_hook_test.go @@ -22,12 +22,6 @@ import ( "github.com/shoenig/test/must" ) -var _ interfaces.RunnerPrerunHook = (*groupServiceHook)(nil) -var _ interfaces.RunnerUpdateHook = (*groupServiceHook)(nil) -var _ interfaces.RunnerPostrunHook = (*groupServiceHook)(nil) -var _ interfaces.RunnerPreKillHook = (*groupServiceHook)(nil) -var _ interfaces.RunnerTaskRestartHook = (*groupServiceHook)(nil) - // TestGroupServiceHook_NoGroupServices asserts calling group service hooks // without group services does not error. func TestGroupServiceHook_NoGroupServices(t *testing.T) { diff --git a/client/allocrunner/health_hook.go b/client/allocrunner/health_hook.go index 8ea7ffb6e..9ac3653f5 100644 --- a/client/allocrunner/health_hook.go +++ b/client/allocrunner/health_hook.go @@ -118,6 +118,14 @@ func newAllocHealthWatcherHook( return h } +// statically assert the hook implements the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*allocHealthWatcherHook)(nil) + _ interfaces.RunnerPostrunHook = (*allocHealthWatcherHook)(nil) + _ interfaces.RunnerUpdateHook = (*allocHealthWatcherHook)(nil) + _ interfaces.ShutdownHook = (*allocHealthWatcherHook)(nil) +) + func (h *allocHealthWatcherHook) Name() string { return "alloc_health_watcher" } diff --git a/client/allocrunner/health_hook_test.go b/client/allocrunner/health_hook_test.go index 81fcf950b..7b1bb09b4 100644 --- a/client/allocrunner/health_hook_test.go +++ b/client/allocrunner/health_hook_test.go @@ -23,12 +23,6 @@ import ( "github.com/stretchr/testify/require" ) -// statically assert health hook implements the expected interfaces -var _ interfaces.RunnerPrerunHook = (*allocHealthWatcherHook)(nil) -var _ interfaces.RunnerUpdateHook = (*allocHealthWatcherHook)(nil) -var _ interfaces.RunnerPostrunHook = (*allocHealthWatcherHook)(nil) -var _ interfaces.ShutdownHook = (*allocHealthWatcherHook)(nil) - // allocHealth is emitted to a chan whenever SetHealth is called type allocHealth struct { healthy bool diff --git a/client/allocrunner/identity_hook.go b/client/allocrunner/identity_hook.go index 043840351..ea34b8ee8 100644 --- a/client/allocrunner/identity_hook.go +++ b/client/allocrunner/identity_hook.go @@ -5,6 +5,7 @@ package allocrunner import ( log "github.com/hashicorp/go-hclog" + "github.com/hashicorp/nomad/client/allocrunner/interfaces" "github.com/hashicorp/nomad/client/widmgr" ) @@ -21,6 +22,14 @@ func newIdentityHook(logger log.Logger, widmgr widmgr.IdentityManager) *identity return h } +// statically assert the hook implements the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*identityHook)(nil) + _ interfaces.RunnerPreKillHook = (*identityHook)(nil) + _ interfaces.RunnerDestroyHook = (*identityHook)(nil) + _ interfaces.ShutdownHook = (*identityHook)(nil) +) + func (*identityHook) Name() string { return "identity" } diff --git a/client/allocrunner/identity_hook_test.go b/client/allocrunner/identity_hook_test.go index bdf0ca278..0d49087e5 100644 --- a/client/allocrunner/identity_hook_test.go +++ b/client/allocrunner/identity_hook_test.go @@ -8,7 +8,6 @@ import ( "time" "github.com/hashicorp/nomad/ci" - "github.com/hashicorp/nomad/client/allocrunner/interfaces" cstate "github.com/hashicorp/nomad/client/state" "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/client/widmgr" @@ -18,12 +17,6 @@ import ( "github.com/shoenig/test/must" ) -// statically assert network hook implements the expected interfaces -var _ interfaces.RunnerPrerunHook = (*identityHook)(nil) -var _ interfaces.ShutdownHook = (*identityHook)(nil) -var _ interfaces.RunnerPreKillHook = (*identityHook)(nil) -var _ interfaces.RunnerDestroyHook = (*identityHook)(nil) - func TestIdentityHook_Prerun(t *testing.T) { ci.Parallel(t) diff --git a/client/allocrunner/migrate_hook.go b/client/allocrunner/migrate_hook.go index 872ec8679..113b9c9da 100644 --- a/client/allocrunner/migrate_hook.go +++ b/client/allocrunner/migrate_hook.go @@ -9,6 +9,7 @@ import ( log "github.com/hashicorp/go-hclog" "github.com/hashicorp/nomad/client/allocdir" + "github.com/hashicorp/nomad/client/allocrunner/interfaces" "github.com/hashicorp/nomad/client/config" ) @@ -33,6 +34,9 @@ func newDiskMigrationHook( return h } +// statically assert the hook implements the expected interfaces +var _ interfaces.RunnerPrerunHook = (*diskMigrationHook)(nil) + func (h *diskMigrationHook) Name() string { return "migrate_disk" } diff --git a/client/allocrunner/network_hook.go b/client/allocrunner/network_hook.go index bddcf1715..13384dd9e 100644 --- a/client/allocrunner/network_hook.go +++ b/client/allocrunner/network_hook.go @@ -9,6 +9,7 @@ import ( "fmt" hclog "github.com/hashicorp/go-hclog" + "github.com/hashicorp/nomad/client/allocrunner/interfaces" "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/plugins/drivers" @@ -103,6 +104,12 @@ func newNetworkHook(logger hclog.Logger, } } +// statically assert the hook implements the expected interfaces +var ( + _ interfaces.RunnerPrerunHook = (*networkHook)(nil) + _ interfaces.RunnerPostrunHook = (*networkHook)(nil) +) + func (h *networkHook) Name() string { return "network" } diff --git a/client/allocrunner/network_hook_test.go b/client/allocrunner/network_hook_test.go index 9e4be2da8..a8ec11fa7 100644 --- a/client/allocrunner/network_hook_test.go +++ b/client/allocrunner/network_hook_test.go @@ -8,7 +8,6 @@ import ( "testing" "github.com/hashicorp/nomad/ci" - "github.com/hashicorp/nomad/client/allocrunner/interfaces" "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/helper/testlog" "github.com/hashicorp/nomad/nomad/mock" @@ -20,10 +19,6 @@ import ( "github.com/shoenig/test/must" ) -// statically assert network hook implements the expected interfaces -var _ interfaces.RunnerPrerunHook = (*networkHook)(nil) -var _ interfaces.RunnerPostrunHook = (*networkHook)(nil) - type mockNetworkIsolationSetter struct { t *testing.T expectedSpec *drivers.NetworkIsolationSpec diff --git a/client/allocrunner/upstream_allocs_hook.go b/client/allocrunner/upstream_allocs_hook.go index 8fff954fc..d990ed82b 100644 --- a/client/allocrunner/upstream_allocs_hook.go +++ b/client/allocrunner/upstream_allocs_hook.go @@ -7,6 +7,7 @@ import ( "context" log "github.com/hashicorp/go-hclog" + "github.com/hashicorp/nomad/client/allocrunner/interfaces" "github.com/hashicorp/nomad/client/config" ) @@ -25,6 +26,9 @@ func newUpstreamAllocsHook(logger log.Logger, allocWatcher config.PrevAllocWatch return h } +// statically assert the hook implements the expected interfaces +var _ interfaces.RunnerPrerunHook = (*upstreamAllocsHook)(nil) + func (h *upstreamAllocsHook) Name() string { return "await_previous_allocations" }