client: statically assert hook interfaces in build (#25472)

While working on #25373, I noticed that the CSI hook's `Destroy` method doesn't
match the interface, which means it never gets called. Because this method only
cancels any in-flight CSI requests, the only impact of this bug is that any CSI
RPCs that are in-flight when an alloc is GC'd on the client or a dev agent is
shut down won't be interrupted gracefully.

Fix the interface, but also make static assertions for all the allocrunner hooks
in the production code, so that you can make changes to interfaces and have
compile-time assistance in avoiding mistakes.

Ref: https://github.com/hashicorp/nomad/pull/25373

.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
+```

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"
 }

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.
 //

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)

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
 }

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"
 }

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)
 

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
 }

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)
 }

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
 }

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)
 

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)

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
 }

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) {

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"
 }

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

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"
 }

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)
 

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"
 }

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"
 }

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

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"
 }