From 97449ee0eed2709436ade3cf84e39bbb9b9b8136 Mon Sep 17 00:00:00 2001 From: ma-04 <120931948+ma-04@users.noreply.github.com> Date: Sun, 27 Sep 2026 13:52:13 +0600 Subject: [PATCH] fix(core): detect user/container and middleware label changes in job hash BareJob.Hash was the only Hash implementation. Every job type embeds BareJob, so the promoted method only hashed Schedule, Name and Command, and dockerLabelsUpdate never replaced a job when labels such as user, container, tty or environment changed. The job kept running with stale values until Ofelia was restarted. Give each job type and each job config its own Hash so both job fields and middleware settings (no-overlap, save-*, slack-*, mail-*) are covered, and exclude the Docker client from RunJob/RunServiceJob hashes. Refs flywp/flywp-internal-tasks#82, mcuadros/ofelia#471 --- cli/config.go | 29 +++++++++++++ cli/docker_handler_test.go | 62 ++++++++++++++++++++++++++- core/execjob.go | 6 +++ core/job.go | 4 ++ core/jobhash_test.go | 85 ++++++++++++++++++++++++++++++++++++++ core/localjob.go | 6 +++ core/runjob.go | 8 +++- core/runservice.go | 8 +++- 8 files changed, 204 insertions(+), 4 deletions(-) create mode 100644 core/jobhash_test.go diff --git a/cli/config.go b/cli/config.go index 0a570f1aec..99082ba43b 100644 --- a/cli/config.go +++ b/cli/config.go @@ -7,6 +7,7 @@ import ( "github.com/mcuadros/ofelia/core" "github.com/mcuadros/ofelia/middlewares" + "github.com/gohugoio/hashstructure" defaults "github.com/mcuadros/go-defaults" gcfg "gopkg.in/gcfg.v1" ) @@ -266,6 +267,13 @@ type ExecJobConfig struct { middlewares.MailConfig `mapstructure:",squash"` } +// Hash covers the job and its middleware settings, so a label change to +// either of them (e.g. user or no-overlap) replaces the scheduled job. +func (c *ExecJobConfig) Hash() uint64 { + hash, _ := hashstructure.Hash(c, nil) + return hash +} + func (c *ExecJobConfig) buildMiddlewares() { c.ExecJob.Use(middlewares.NewOverlap(&c.OverlapConfig)) c.ExecJob.Use(middlewares.NewSlack(&c.SlackConfig)) @@ -290,6 +298,13 @@ type RunJobConfig struct { middlewares.MailConfig `mapstructure:",squash"` } +// Hash covers the job and its middleware settings, so a label change to +// either of them (e.g. user or no-overlap) replaces the scheduled job. +func (c *RunJobConfig) Hash() uint64 { + hash, _ := hashstructure.Hash(c, nil) + return hash +} + func (c *RunJobConfig) buildMiddlewares() { c.RunJob.Use(middlewares.NewOverlap(&c.OverlapConfig)) c.RunJob.Use(middlewares.NewSlack(&c.SlackConfig)) @@ -306,6 +321,13 @@ type LocalJobConfig struct { middlewares.MailConfig `mapstructure:",squash"` } +// Hash covers the job and its middleware settings, so a label change to +// either of them (e.g. user or no-overlap) replaces the scheduled job. +func (c *LocalJobConfig) Hash() uint64 { + hash, _ := hashstructure.Hash(c, nil) + return hash +} + func (c *LocalJobConfig) buildMiddlewares() { c.LocalJob.Use(middlewares.NewOverlap(&c.OverlapConfig)) c.LocalJob.Use(middlewares.NewSlack(&c.SlackConfig)) @@ -313,6 +335,13 @@ func (c *LocalJobConfig) buildMiddlewares() { c.LocalJob.Use(middlewares.NewMail(&c.MailConfig)) } +// Hash covers the job and its middleware settings, so a label change to +// either of them (e.g. user or no-overlap) replaces the scheduled job. +func (c *RunServiceConfig) Hash() uint64 { + hash, _ := hashstructure.Hash(c, nil) + return hash +} + func (c *RunServiceConfig) buildMiddlewares() { c.RunServiceJob.Use(middlewares.NewOverlap(&c.OverlapConfig)) c.RunServiceJob.Use(middlewares.NewSlack(&c.SlackConfig)) diff --git a/cli/docker_handler_test.go b/cli/docker_handler_test.go index 40798a47d6..a5a4b57607 100644 --- a/cli/docker_handler_test.go +++ b/cli/docker_handler_test.go @@ -363,7 +363,7 @@ func (s *TestDockerSuit) TestConsumeEventsTriggersLabelUpdate(c *check.C) { { Names: []string{"/myapp"}, Labels: map[string]string{ - requiredLabel: "true", + requiredLabel: "true", labelPrefix + "." + jobExec + ".job1.schedule": "* * * * *", labelPrefix + "." + jobExec + ".job1.command": "echo hello", }, @@ -417,7 +417,7 @@ func (s *TestDockerSuit) TestConsumeEventsDebounces(c *check.C) { { Names: []string{"/myapp"}, Labels: map[string]string{ - requiredLabel: "true", + requiredLabel: "true", labelPrefix + "." + jobExec + ".job1.schedule": "* * * * *", labelPrefix + "." + jobExec + ".job1.command": "echo hello", }, @@ -612,3 +612,61 @@ func withDockerEnv(envs map[string]string) func() { } } } + +func (s *TestDockerSuit) TestDockerLabelsUpdateDetectsJobChanges(c *check.C) { + execPrefix := labelPrefix + "." + jobExec + ".wpcron." + runPrefix := labelPrefix + "." + jobRun + ".backup." + labels := func(extra map[string]string) map[string]string { + l := map[string]string{ + requiredLabel: "true", + serviceLabel: "true", + execPrefix + "schedule": "@every 10m", + execPrefix + "command": "wp cron event run --due-now", + runPrefix + "schedule": "@hourly", + runPrefix + "image": "alpine", + runPrefix + "command": "true", + execPrefix + "environment": `["A=1","B=2","C=3"]`, + runPrefix + "volume": `["/a:/a","/b:/b","/c:/c"]`, + } + for k, v := range extra { + l[k] = v + } + return l + } + update := func(conf *Config, extra map[string]string) { + conf.dockerLabelsUpdate(map[string]map[string]string{"site-php-1": labels(extra)}) + } + + mock := &mockCLIDockerClient{ + containers: []container.Summary{ + {Names: []string{"/site-php-1"}, Labels: labels(map[string]string{execPrefix + "user": "www-data"})}, + }, + } + mockLogger := &TestLogger{} + conf := NewConfig(mockLogger) + conf.sh = core.NewScheduler(mockLogger) + conf.dockerHandler = newTestDockerHandler(mock, nil) + c.Assert(conf.InitializeApp(), check.IsNil) + + execJob, runJob := conf.ExecJobs["wpcron"], conf.RunJobs["backup"] + c.Assert(execJob.User, check.Equals, "www-data") + c.Assert(execJob.Hash(), check.Not(check.Equals), uint64(0)) + c.Assert(runJob.Hash(), check.Not(check.Equals), uint64(0)) + + // Unchanged labels must keep the scheduled jobs untouched + for i := 0; i < 20; i++ { + update(conf, map[string]string{execPrefix + "user": "www-data"}) + c.Assert(conf.ExecJobs["wpcron"], check.Equals, execJob) + c.Assert(conf.RunJobs["backup"], check.Equals, runJob) + } + + // Removing the user label must switch the job to the container's user + update(conf, nil) + c.Assert(conf.ExecJobs["wpcron"].User, check.Equals, "") + c.Assert(conf.RunJobs["backup"], check.Equals, runJob) + + // Middleware labels are part of the job config as well + update(conf, map[string]string{execPrefix + "no-overlap": "true", runPrefix + "user": "nobody"}) + c.Assert(conf.ExecJobs["wpcron"].NoOverlap, check.Equals, true) + c.Assert(conf.RunJobs["backup"].User, check.Equals, "nobody") +} diff --git a/core/execjob.go b/core/execjob.go index 9eb85df7cd..a6d00eb098 100644 --- a/core/execjob.go +++ b/core/execjob.go @@ -98,3 +98,9 @@ func (j *ExecJob) inspectExec(ctx *Context) (client.ExecInspectResult, error) { return i, nil } + +// Hash overrides the promoted BareJob.Hash, which only covers the BareJob +// fields, so changes to Container, User, TTY, etc. are detected too. +func (j *ExecJob) Hash() uint64 { + return hashJob(j) +} diff --git a/core/job.go b/core/job.go index 3b9563227e..7bcd95eac0 100644 --- a/core/job.go +++ b/core/job.go @@ -54,6 +54,10 @@ func (j *BareJob) NotifyStop() { // Returns a hash of all the job attributes. Used to detect changes // unexported struct fields are ignored - https://pkg.go.dev/github.com/gohugoio/hashstructure#Hash func (j *BareJob) Hash() uint64 { + return hashJob(j) +} + +func hashJob(j any) uint64 { hash, _ := hashstructure.Hash(j, nil) return hash } diff --git a/core/jobhash_test.go b/core/jobhash_test.go new file mode 100644 index 0000000000..5c7d198d44 --- /dev/null +++ b/core/jobhash_test.go @@ -0,0 +1,85 @@ +package core + +import ( + . "gopkg.in/check.v1" +) + +type SuiteJobHash struct{} + +var _ = Suite(&SuiteJobHash{}) + +func newHashExecJob(container, user string) *ExecJob { + return &ExecJob{ + BareJob: BareJob{Name: "wpcron", Schedule: "@every 10m", Command: "wp cron event run --due-now"}, + Container: container, + User: user, + } +} + +func (s *SuiteJobHash) TestExecJobHashDetectsFieldChanges(c *C) { + base := newHashExecJob("site-php-1", "www-data") + + user := newHashExecJob("site-php-1", "") + c.Assert(user.Hash(), Not(Equals), base.Hash()) + + container := newHashExecJob("other-php-1", "www-data") + c.Assert(container.Hash(), Not(Equals), base.Hash()) + + tty := newHashExecJob("site-php-1", "www-data") + tty.TTY = true + c.Assert(tty.Hash(), Not(Equals), base.Hash()) + + env := newHashExecJob("site-php-1", "www-data") + env.Environment = []string{"FOO=bar"} + c.Assert(env.Hash(), Not(Equals), base.Hash()) +} + +func (s *SuiteJobHash) TestExecJobHashIgnoresClientAndRuntimeState(c *C) { + a := newHashExecJob("site-php-1", "1000:1000") + b := newHashExecJob("site-php-1", "1000:1000") + b.Client = &mockDockerClient{} + b.cronID = 42 + b.execID = "abc" + b.history = append(b.history, &Execution{}) + c.Assert(b.Hash(), Equals, a.Hash()) +} + +func (s *SuiteJobHash) TestRunJobHashDetectsFieldChanges(c *C) { + newJob := func() *RunJob { + return &RunJob{BareJob: BareJob{Name: "r", Schedule: "@hourly", Command: "true"}, Image: "alpine", User: "nobody"} + } + base := newJob() + + user := newJob() + user.User = "1000" + c.Assert(user.Hash(), Not(Equals), base.Hash()) + + image := newJob() + image.Image = "busybox" + c.Assert(image.Hash(), Not(Equals), base.Hash()) + + withClient := newJob() + withClient.Client = &mockDockerClient{} + c.Assert(withClient.Hash(), Equals, base.Hash()) +} + +func (s *SuiteJobHash) TestRunServiceJobHashDetectsFieldChanges(c *C) { + newJob := func() *RunServiceJob { + return &RunServiceJob{BareJob: BareJob{Name: "s", Schedule: "@hourly", Command: "true"}, Image: "alpine"} + } + base := newJob() + + user := newJob() + user.User = "1000" + c.Assert(user.Hash(), Not(Equals), base.Hash()) + + withClient := newJob() + withClient.Client = &mockDockerClient{} + c.Assert(withClient.Hash(), Equals, base.Hash()) +} + +func (s *SuiteJobHash) TestLocalJobHashDetectsFieldChanges(c *C) { + base := &LocalJob{BareJob: BareJob{Name: "l", Schedule: "@hourly", Command: "true"}} + dir := &LocalJob{BareJob: BareJob{Name: "l", Schedule: "@hourly", Command: "true"}, Dir: "/tmp"} + c.Assert(dir.Hash(), Not(Equals), base.Hash()) +} diff --git a/core/localjob.go b/core/localjob.go index f2acb14e5d..7a2c9803f4 100644 --- a/core/localjob.go +++ b/core/localjob.go @@ -44,3 +44,9 @@ func (j *LocalJob) buildCommand(ctx *Context) (*exec.Cmd, error) { Dir: j.Dir, }, nil } + +// Hash overrides the promoted BareJob.Hash, which only covers the BareJob +// fields, so changes to Dir and Environment are detected too. +func (j *LocalJob) Hash() uint64 { + return hashJob(j) +} diff --git a/core/runjob.go b/core/runjob.go index 11b09063ee..f6afc2039f 100644 --- a/core/runjob.go +++ b/core/runjob.go @@ -16,7 +16,7 @@ import ( type RunJob struct { BareJob `mapstructure:",squash"` - Client DockerClient `json:"-"` + Client DockerClient `json:"-" hash:"-"` User string `default:""` TTY bool `default:"false"` @@ -264,3 +264,9 @@ func (j *RunJob) deleteContainer(ctx *Context) error { _, err := j.Client.ContainerRemove(ctx.Context(), j.containerID, client.ContainerRemoveOptions{}) return err } + +// Hash overrides the promoted BareJob.Hash, which only covers the BareJob +// fields, so changes to Image, User, Volume, etc. are detected too. +func (j *RunJob) Hash() uint64 { + return hashJob(j) +} diff --git a/core/runservice.go b/core/runservice.go index a00a7a87bd..c289d3cf12 100644 --- a/core/runservice.go +++ b/core/runservice.go @@ -14,7 +14,7 @@ import ( type RunServiceJob struct { BareJob `mapstructure:",squash"` - Client DockerClient `json:"-"` + Client DockerClient `json:"-" hash:"-"` User string `default:""` TTY bool `default:"false"` // do not use bool values with "default:true" because if @@ -195,3 +195,9 @@ func (j *RunServiceJob) deleteService(ctx *Context, svcID string) error { return err } + +// Hash overrides the promoted BareJob.Hash, which only covers the BareJob +// fields, so changes to Image, User, Network, etc. are detected too. +func (j *RunServiceJob) Hash() uint64 { + return hashJob(j) +}