logs: fix missing allocation logs after update to Nomad 1.5.4

.changelog/17087.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+logging: Fixed a bug where alloc logs would not be collected after an upgrade to 1.5.4
+```

api/tasks.go

@@ -639,16 +639,21 @@ func (g *TaskGroup) AddSpread(s *Spread) *TaskGroup {
 
 // LogConfig provides configuration for log rotation
 type LogConfig struct {
-	MaxFiles      *int  `mapstructure:"max_files" hcl:"max_files,optional"`
-	MaxFileSizeMB *int  `mapstructure:"max_file_size" hcl:"max_file_size,optional"`
-	Enabled       *bool `mapstructure:"enabled" hcl:"enabled,optional"`
+	MaxFiles      *int `mapstructure:"max_files" hcl:"max_files,optional"`
+	MaxFileSizeMB *int `mapstructure:"max_file_size" hcl:"max_file_size,optional"`
+
+	// COMPAT(1.6.0): Enabled had to be swapped for Disabled to fix a backwards
+	// compatibility bug when restoring pre-1.5.4 jobs. Remove in 1.6.0
+	Enabled *bool `mapstructure:"enabled" hcl:"enabled,optional"`
+
+	Disabled *bool `mapstructure:"disabled" hcl:"disabled,optional"`
 }
 
 func DefaultLogConfig() *LogConfig {
 	return &LogConfig{
 		MaxFiles:      pointerOf(10),
 		MaxFileSizeMB: pointerOf(10),
-		Enabled:       pointerOf(true),
+		Disabled:      pointerOf(false),
 	}
 }
 
@@ -659,8 +664,8 @@ func (l *LogConfig) Canonicalize() {
 	if l.MaxFileSizeMB == nil {
 		l.MaxFileSizeMB = pointerOf(10)
 	}
-	if l.Enabled == nil {
-		l.Enabled = pointerOf(true)
+	if l.Disabled == nil {
+		l.Disabled = pointerOf(false)
 	}
 }
 

client/allocrunner/taskrunner/logmon_hook.go

@@ -46,7 +46,7 @@ type logmonHook struct {
 
 type logmonHookConfig struct {
 	logDir     string
-	enabled    bool
+	disabled   bool
 	stdoutFifo string
 	stderrFifo string
 }
@@ -63,12 +63,12 @@ func newLogMonHook(tr *TaskRunner, logger hclog.Logger) *logmonHook {
 
 func newLogMonHookConfig(taskName string, logCfg *structs.LogConfig, logDir string) *logmonHookConfig {
 	cfg := &logmonHookConfig{
-		logDir:  logDir,
-		enabled: logCfg.Enabled,
+		logDir:   logDir,
+		disabled: logCfg.Disabled,
 	}
 
 	// If logging is disabled configure task's stdout/err to point to devnull
-	if !logCfg.Enabled {
+	if logCfg.Disabled {
 		cfg.stdoutFifo = os.DevNull
 		cfg.stderrFifo = os.DevNull
 		return cfg
@@ -116,7 +116,7 @@ func reattachConfigFromHookData(data map[string]string) (*plugin.ReattachConfig,
 
 func (h *logmonHook) Prestart(ctx context.Context,
 	req *interfaces.TaskPrestartRequest, resp *interfaces.TaskPrestartResponse) error {
-	if !h.isLoggingEnabled() {
+	if h.isLoggingDisabled() {
 		return nil
 	}
 
@@ -151,27 +151,27 @@ func (h *logmonHook) Prestart(ctx context.Context,
 	}
 }
 
-func (h *logmonHook) isLoggingEnabled() bool {
-	if !h.config.enabled {
+func (h *logmonHook) isLoggingDisabled() bool {
+	if h.config.disabled {
 		h.logger.Debug("log collection is disabled by task")
-		return false
+		return true
 	}
 
 	// internal plugins have access to a capability to disable logging and
 	// metrics via a private interface; this is an "experimental" interface and
 	// currently only the docker driver exposes this to users.
 	ic, ok := h.runner.driver.(drivers.InternalCapabilitiesDriver)
 	if !ok {
-		return true
+		return false
 	}
 
 	caps := ic.InternalCapabilities()
 	if caps.DisableLogCollection {
 		h.logger.Debug("log collection is disabled by driver")
-		return false
+		return true
 	}
 
-	return true
+	return false
 }
 
 func (h *logmonHook) prestartOneLoop(ctx context.Context, req *interfaces.TaskPrestartRequest) error {
@@ -219,7 +219,7 @@ func (h *logmonHook) prestartOneLoop(ctx context.Context, req *interfaces.TaskPr
 }
 
 func (h *logmonHook) Stop(_ context.Context, req *interfaces.TaskStopRequest, _ *interfaces.TaskStopResponse) error {
-	if !h.isLoggingEnabled() {
+	if h.isLoggingDisabled() {
 		return nil
 	}
 

client/allocrunner/taskrunner/logmon_hook_test.go

@@ -112,7 +112,7 @@ func TestTaskRunner_LogmonHook_Disabled(t *testing.T) {
 
 	alloc := mock.BatchAlloc()
 	task := alloc.Job.TaskGroups[0].Tasks[0]
-	task.LogConfig.Enabled = false
+	task.LogConfig.Disabled = true
 
 	dir := t.TempDir()
 

command/agent/job_endpoint.go

@@ -1242,11 +1242,7 @@ func ApiTaskToStructsTask(job *structs.Job, group *structs.TaskGroup,
 
 	structsTask.Resources = ApiResourcesToStructs(apiTask.Resources)
 
-	structsTask.LogConfig = &structs.LogConfig{
-		MaxFiles:      *apiTask.LogConfig.MaxFiles,
-		MaxFileSizeMB: *apiTask.LogConfig.MaxFileSizeMB,
-		Enabled:       *apiTask.LogConfig.Enabled,
-	}
+	structsTask.LogConfig = apiLogConfigToStructs(apiTask.LogConfig)
 
 	if len(apiTask.Artifacts) > 0 {
 		structsTask.Artifacts = []*structs.TaskArtifact{}
@@ -1809,13 +1805,21 @@ func apiLogConfigToStructs(in *api.LogConfig) *structs.LogConfig {
 	if in == nil {
 		return nil
 	}
+
 	return &structs.LogConfig{
-		Enabled:       *in.Enabled,
+		Disabled:      dereferenceBool(in.Disabled),
 		MaxFiles:      dereferenceInt(in.MaxFiles),
 		MaxFileSizeMB: dereferenceInt(in.MaxFileSizeMB),
 	}
 }
 
+func dereferenceBool(in *bool) bool {
+	if in == nil {
+		return false
+	}
+	return *in
+}
+
 func dereferenceInt(in *int) int {
 	if in == nil {
 		return 0

command/agent/job_endpoint_test.go

@@ -2767,7 +2767,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 						KillTimeout: pointer.Of(10 * time.Second),
 						KillSignal:  "SIGQUIT",
 						LogConfig: &api.LogConfig{
-							Enabled:       pointer.Of(true),
+							Disabled:      pointer.Of(true),
 							MaxFiles:      pointer.Of(10),
 							MaxFileSizeMB: pointer.Of(100),
 						},
@@ -3185,7 +3185,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 						KillTimeout: 10 * time.Second,
 						KillSignal:  "SIGQUIT",
 						LogConfig: &structs.LogConfig{
-							Enabled:       true,
+							Disabled:      true,
 							MaxFiles:      10,
 							MaxFileSizeMB: 100,
 						},
@@ -3342,7 +3342,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 						KillTimeout: pointer.Of(10 * time.Second),
 						KillSignal:  "SIGQUIT",
 						LogConfig: &api.LogConfig{
-							Enabled:       pointer.Of(true),
+							Disabled:      pointer.Of(true),
 							MaxFiles:      pointer.Of(10),
 							MaxFileSizeMB: pointer.Of(100),
 						},
@@ -3468,7 +3468,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 						KillTimeout: 10 * time.Second,
 						KillSignal:  "SIGQUIT",
 						LogConfig: &structs.LogConfig{
-							Enabled:       true,
+							Disabled:      true,
 							MaxFiles:      10,
 							MaxFileSizeMB: 100,
 						},
@@ -3639,16 +3639,39 @@ func TestConversion_dereferenceInt(t *testing.T) {
 
 func TestConversion_apiLogConfigToStructs(t *testing.T) {
 	ci.Parallel(t)
-	require.Nil(t, apiLogConfigToStructs(nil))
-	require.Equal(t, &structs.LogConfig{
-		Enabled:       true,
+	must.Nil(t, apiLogConfigToStructs(nil))
+	must.Eq(t, &structs.LogConfig{
+		Disabled:      true,
 		MaxFiles:      2,
 		MaxFileSizeMB: 8,
 	}, apiLogConfigToStructs(&api.LogConfig{
-		Enabled:       pointer.Of(true),
+		Disabled:      pointer.Of(true),
 		MaxFiles:      pointer.Of(2),
 		MaxFileSizeMB: pointer.Of(8),
 	}))
+
+	// COMPAT(1.6.0): verify backwards compatibility fixes
+	// Note: we're intentionally ignoring the Enabled: false case
+	must.Eq(t, &structs.LogConfig{Disabled: false},
+		apiLogConfigToStructs(&api.LogConfig{
+			Enabled: pointer.Of(false),
+		}))
+	must.Eq(t, &structs.LogConfig{Disabled: false},
+		apiLogConfigToStructs(&api.LogConfig{
+			Enabled: pointer.Of(true),
+		}))
+	must.Eq(t, &structs.LogConfig{Disabled: false},
+		apiLogConfigToStructs(&api.LogConfig{}))
+	must.Eq(t, &structs.LogConfig{Disabled: false},
+		apiLogConfigToStructs(&api.LogConfig{
+			Disabled: pointer.Of(false),
+		}))
+	must.Eq(t, &structs.LogConfig{Disabled: false},
+		apiLogConfigToStructs(&api.LogConfig{
+			Enabled:  pointer.Of(false),
+			Disabled: pointer.Of(false),
+		}))
+
 }
 
 func TestConversion_apiResourcesToStructs(t *testing.T) {
@@ -3743,7 +3766,7 @@ func TestConversion_apiConnectSidecarTaskToStructs(t *testing.T) {
 		Meta:        meta,
 		KillTimeout: &timeout,
 		LogConfig: &structs.LogConfig{
-			Enabled:       true,
+			Disabled:      true,
 			MaxFiles:      2,
 			MaxFileSizeMB: 8,
 		},
@@ -3762,7 +3785,7 @@ func TestConversion_apiConnectSidecarTaskToStructs(t *testing.T) {
 		Meta:        meta,
 		KillTimeout: &timeout,
 		LogConfig: &api.LogConfig{
-			Enabled:       pointer.Of(true),
+			Disabled:      pointer.Of(true),
 			MaxFiles:      pointer.Of(2),
 			MaxFileSizeMB: pointer.Of(8),
 		},

jobspec/parse_task.go

@@ -262,7 +262,8 @@ func parseTask(item *ast.ObjectItem, keys []string) (*api.Task, error) {
 		valid := []string{
 			"max_files",
 			"max_file_size",
-			"enabled",
+			"enabled", // COMPAT(1.6.0): remove in favor of disabled
+			"disabled",
 		}
 		if err := checkHCLKeys(logsBlock.Val, valid); err != nil {
 			return nil, multierror.Prefix(err, "logs ->")

jobspec/parse_test.go

@@ -350,7 +350,7 @@ func TestParse(t *testing.T) {
 								LogConfig: &api.LogConfig{
 									MaxFiles:      intToPtr(14),
 									MaxFileSizeMB: intToPtr(101),
-									Enabled:       boolToPtr(true),
+									Disabled:      boolToPtr(false),
 								},
 								Artifacts: []*api.TaskArtifact{
 									{

jobspec/test-fixtures/basic.hcl

@@ -194,7 +194,7 @@ job "binstore-storagelocker" {
       }
 
       logs {
-        enabled       = true
+        disabled      = false
         max_files     = 14
         max_file_size = 101
       }

nomad/structs/diff_test.go

@@ -4420,7 +4420,7 @@ func TestTaskDiff(t *testing.T) {
 				LogConfig: &LogConfig{
 					MaxFiles:      1,
 					MaxFileSizeMB: 10,
-					Enabled:       false,
+					Disabled:      true,
 				},
 			},
 			Expected: &TaskDiff{
@@ -4432,9 +4432,9 @@ func TestTaskDiff(t *testing.T) {
 						Fields: []*FieldDiff{
 							{
 								Type: DiffTypeAdded,
-								Name: "Enabled",
+								Name: "Disabled",
 								Old:  "",
-								New:  "false",
+								New:  "true",
 							},
 							{
 								Type: DiffTypeAdded,
@@ -4459,7 +4459,7 @@ func TestTaskDiff(t *testing.T) {
 				LogConfig: &LogConfig{
 					MaxFiles:      1,
 					MaxFileSizeMB: 10,
-					Enabled:       false,
+					Disabled:      true,
 				},
 			},
 			New: &Task{},
@@ -4472,8 +4472,8 @@ func TestTaskDiff(t *testing.T) {
 						Fields: []*FieldDiff{
 							{
 								Type: DiffTypeDeleted,
-								Name: "Enabled",
-								Old:  "false",
+								Name: "Disabled",
+								Old:  "true",
 								New:  "",
 							},
 							{
@@ -4499,14 +4499,14 @@ func TestTaskDiff(t *testing.T) {
 				LogConfig: &LogConfig{
 					MaxFiles:      1,
 					MaxFileSizeMB: 10,
-					Enabled:       false,
+					Disabled:      false,
 				},
 			},
 			New: &Task{
 				LogConfig: &LogConfig{
 					MaxFiles:      2,
 					MaxFileSizeMB: 20,
-					Enabled:       true,
+					Disabled:      true,
 				},
 			},
 			Expected: &TaskDiff{
@@ -4518,7 +4518,7 @@ func TestTaskDiff(t *testing.T) {
 						Fields: []*FieldDiff{
 							{
 								Type: DiffTypeEdited,
-								Name: "Enabled",
+								Name: "Disabled",
 								Old:  "false",
 								New:  "true",
 							},
@@ -4546,14 +4546,14 @@ func TestTaskDiff(t *testing.T) {
 				LogConfig: &LogConfig{
 					MaxFiles:      1,
 					MaxFileSizeMB: 10,
-					Enabled:       false,
+					Disabled:      false,
 				},
 			},
 			New: &Task{
 				LogConfig: &LogConfig{
 					MaxFiles:      1,
 					MaxFileSizeMB: 20,
-					Enabled:       true,
+					Disabled:      true,
 				},
 			},
 			Expected: &TaskDiff{
@@ -4565,7 +4565,7 @@ func TestTaskDiff(t *testing.T) {
 						Fields: []*FieldDiff{
 							{
 								Type: DiffTypeEdited,
-								Name: "Enabled",
+								Name: "Disabled",
 								Old:  "false",
 								New:  "true",
 							},