Skip to content

Make action optional when upgrade details are provided #5609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Make action optional when upgrade details are provided #5609

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f8a4a2c
Make action optional when upgrade details are provided
michalpristas Oct 2, 2025
6331a01
Merge branch 'main' of github.com:elastic/fleet-server into fix/migra…
michalpristas Oct 2, 2025
e455769
removed not needed test
michalpristas Oct 2, 2025
e5e9f68
Merge branch 'main' into fix/migrate-watching
michalpristas Oct 7, 2025
8a895c0
Merge branch 'main' of github.com:elastic/fleet-server into fix/migra…
michalpristas Oct 7, 2025
80d23ca
Merge branch 'fix/migrate-watching' of github.com:michalpristas/fleet…
michalpristas Oct 7, 2025
bd39320
Merge branch 'main' into fix/migrate-watching
michalpristas Oct 7, 2025
0862864
Merge branch 'main' into fix/migrate-watching
michalpristas Oct 24, 2025
7809dfb
Merge branch 'main' into fix/migrate-watching
michalpristas Oct 27, 2025
45376cb
Merge branch 'main' into fix/migrate-watching
michalpristas Nov 24, 2025
0ad1868
added changelog fragment
michalpristas Nov 25, 2025
72ee7a5
added changelog fragment
michalpristas Nov 25, 2025
3d7aa6a
revert code cleanup
michalpristas Nov 26, 2025
42f4781
Merge branch 'main' of github.com:elastic/fleet-server into fix/migra…
michalpristas Nov 26, 2025
0e8cefe
Merge branch 'main' into fix/migrate-watching
michalpristas Dec 1, 2025
378d270
Merge branch 'main' into fix/migrate-watching
michalpristas Dec 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions internal/pkg/api/handleAck.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,22 +612,18 @@ func (ack *AckT) handleUpgrade(ctx context.Context, zlog zerolog.Logger, agent *
zlog.Info().Msg("marking agent upgrade as failed, agent logs contain failure message")
doc = bulk.UpdateFields{
dl.FieldUpgradeStartedAt: nil,
dl.FieldUpgradeStatus: "failed",
}
} else if event.Payload.Retry {
zlog.Info().Int("retry_attempt", event.Payload.RetryAttempt).Msg("marking agent upgrade as retrying")
doc[dl.FieldUpgradeStatus] = "retrying" // Keep FieldUpgradeStatedAt abd FieldUpgradeded at to original values
} else {
zlog.Info().Int("retry_attempt", event.Payload.RetryAttempt).Msg("marking agent upgrade as failed, agent logs contain failure message")
doc = bulk.UpdateFields{
dl.FieldUpgradeStartedAt: nil,
dl.FieldUpgradeStatus: "failed",
}
}
} else {
doc = bulk.UpdateFields{
dl.FieldUpgradeStartedAt: nil,
dl.FieldUpgradeStatus: nil,
dl.FieldUpgradedAt: now,
}
if agent.UpgradeDetails == nil {
Expand Down
61 changes: 0 additions & 61 deletions internal/pkg/api/handleAck_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,67 +719,6 @@ func TestAckHandleUpgrade(t *testing.T) {
ESDocument: model.ESDocument{Id: "ab12dcd8-bde0-4045-92dc-c4b27668d735"},
Agent: &model.AgentMetadata{Version: "8.0.0"},
},
}, {
name: "retry signaled",
Copy link
Contributor

@michel-laterman michel-laterman Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to remove these tests in this PR?

event: UpgradeEvent{
Error: ptr("upgrade error"),
Payload: &struct {
Retry bool `json:"retry"`
RetryAttempt int `json:"retry_attempt"`
}{
Retry: true,
RetryAttempt: 1,
},
},
bulker: func(t *testing.T) *ftesting.MockBulk {
m := ftesting.NewMockBulk()
m.On("Update", mock.Anything, mock.Anything, mock.Anything, mock.MatchedBy(func(p []byte) bool {
var body struct {
Doc struct {
Status string `json:"upgrade_status"`
} `json:"doc"`
}
if err := json.Unmarshal(p, &body); err != nil {
t.Fatal(err)
}
return body.Doc.Status == "retrying"
}), mock.Anything).Return(nil).Once()
return m
},
agent: &model.Agent{
ESDocument: model.ESDocument{Id: "ab12dcd8-bde0-4045-92dc-c4b27668d735"},
Agent: &model.AgentMetadata{Version: "8.0.0"},
},
}, {
name: "no more retries",
event: UpgradeEvent{
Error: ptr("upgrade error"),
Payload: &struct {
Retry bool `json:"retry"`
RetryAttempt int `json:"retry_attempt"`
}{
Retry: false,
},
},
bulker: func(t *testing.T) *ftesting.MockBulk {
m := ftesting.NewMockBulk()
m.On("Update", mock.Anything, mock.Anything, mock.Anything, mock.MatchedBy(func(p []byte) bool {
var body struct {
Doc struct {
Status string `json:"upgrade_status"`
} `json:"doc"`
}
if err := json.Unmarshal(p, &body); err != nil {
t.Fatal(err)
}
return body.Doc.Status == "failed"
}), mock.Anything).Return(nil).Once()
return m
},
agent: &model.Agent{
ESDocument: model.ESDocument{Id: "ab12dcd8-bde0-4045-92dc-c4b27668d735"},
Agent: &model.AgentMetadata{Version: "8.0.0"},
},
}, {
name: "keep upgrade_attempts if upgrade_details is not nil",
event: UpgradeEvent{},
Expand Down
19 changes: 9 additions & 10 deletions internal/pkg/api/handleCheckin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/elastic/fleet-server/v7/internal/pkg/checkin"
"github.com/elastic/fleet-server/v7/internal/pkg/config"
"github.com/elastic/fleet-server/v7/internal/pkg/dl"
"github.com/elastic/fleet-server/v7/internal/pkg/es"
"github.com/elastic/fleet-server/v7/internal/pkg/logger/ecs"
"github.com/elastic/fleet-server/v7/internal/pkg/model"
"github.com/elastic/fleet-server/v7/internal/pkg/monitor"
Expand Down Expand Up @@ -454,22 +455,21 @@ func (ct *CheckinT) processUpgradeDetails(ctx context.Context, agent *model.Agen
return nil
}
// update docs with in progress details

vSpan, vCtx := apm.StartSpan(ctx, "Check update action", "validate")
action, err := ct.verifyActionExists(ctx, vSpan, agent, details)
if err != nil {

var actionTraceparent string
Copy link
Contributor Author

@michalpristas michalpristas Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blakerouse normally when no actin is present we stop here
but we make loading optional and if action is not present we just set traceparent (only property used here in the code) to ""

then later we check if we have here when setting parent span and continue until we reach a point where we update details on line 543

if action, err := ct.verifyActionExists(ctx, vSpan, agent, details); err == nil && action != nil {
Copy link
Member

@cmacknz cmacknz Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Kibana tolerate having an action ID it doesn't recognize as well?

I think passing through the upgrade details unmodified is correct, but we need to avoid Kibana now or in the future trying to do this same validation or making assumption about this field.

Copy link
Member

@cmacknz cmacknz Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think passing through the upgrade details unmodified is correct

Well a possibility here would be removing the action_id if it doesn't exist so kibana can't depend on it, or putting some marker in the details about whether the action exists or not (though kibana can also just check this).

Copy link
Member

@cmacknz cmacknz Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliaElastic any opinions on if sending upgrade_details to Kibana for action_ids that don't exist or are from another cluster is a problem?

actionTraceparent = action.Traceparent
} else if !errors.Is(err, es.ErrNotFound) {
return err
}
if action == nil {
return nil
}

// link action with APM spans
var links []apm.SpanLink
if ct.bulker.HasTracer() && action.Traceparent != "" {
traceCtx, err := apmhttp.ParseTraceparentHeader(action.Traceparent)
if ct.bulker.HasTracer() && actionTraceparent != "" {
traceCtx, err := apmhttp.ParseTraceparentHeader(actionTraceparent)
if err != nil {
zerolog.Ctx(vCtx).Trace().Err(err).Msgf("Error parsing traceparent: %s %s", action.Traceparent, err)
zerolog.Ctx(vCtx).Trace().Err(err).Msgf("Error parsing traceparent: %s %s", actionTraceparent, err)
} else {
links = []apm.SpanLink{
{
Expand Down Expand Up @@ -565,7 +565,6 @@ func (ct *CheckinT) markUpgradeComplete(ctx context.Context, agent *model.Agent)
doc := bulk.UpdateFields{
dl.FieldUpgradeDetails: nil,
dl.FieldUpgradeStartedAt: nil,
dl.FieldUpgradeStatus: nil,
dl.FieldUpgradedAt: time.Now().UTC().Format(time.RFC3339),
}
body, err := doc.Marshal()
Expand Down
22 changes: 19 additions & 3 deletions internal/pkg/api/handleCheckin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ func TestProcessUpgradeDetails(t *testing.T) {
t.Logf("bulk match unmarshal error: %v", err)
return false
}
return doc.Doc[dl.FieldUpgradeDetails] == nil && doc.Doc[dl.FieldUpgradeStartedAt] == nil && doc.Doc[dl.FieldUpgradeStatus] == nil && doc.Doc[dl.FieldUpgradedAt] != ""
return doc.Doc[dl.FieldUpgradeDetails] == nil && doc.Doc[dl.FieldUpgradeStartedAt] == nil && doc.Doc[dl.FieldUpgradedAt] != ""
}), mock.Anything, mock.Anything).Return(nil)
return mBulk
},
Expand Down Expand Up @@ -467,20 +467,36 @@ func TestProcessUpgradeDetails(t *testing.T) {
},
err: nil,
}, {
name: "upgrade requested action invalid",
name: "upgrade requested action not found",
agent: &model.Agent{ESDocument: esd, Agent: &model.AgentMetadata{ID: "test-agent"}},
details: &UpgradeDetails{ActionId: "test-action", State: UpgradeDetailsStateUPGREQUESTED},
bulk: func() *ftesting.MockBulk {
mBulk := ftesting.NewMockBulk()
mBulk.On("Search", mock.Anything, dl.FleetActions, mock.Anything, mock.Anything).Return(&es.ResultT{}, es.ErrNotFound)
mBulk.On("Update", mock.Anything, dl.FleetAgents, "doc-ID", mock.Anything, mock.Anything, mock.Anything).Return(nil)
return mBulk
},
cache: func() *testcache.MockCache {
mCache := testcache.NewMockCache()
mCache.On("GetAction", "test-action").Return(model.Action{}, false)
return mCache
},
err: nil,
}, {
name: "upgrade requested action failed to fetch",
agent: &model.Agent{ESDocument: esd, Agent: &model.AgentMetadata{ID: "test-agent"}},
details: &UpgradeDetails{ActionId: "test-action", State: UpgradeDetailsStateUPGREQUESTED},
bulk: func() *ftesting.MockBulk {
mBulk := ftesting.NewMockBulk()
mBulk.On("Search", mock.Anything, dl.FleetActions, mock.Anything, mock.Anything).Return(&es.ResultT{}, es.ErrTimeout)
return mBulk
},
cache: func() *testcache.MockCache {
mCache := testcache.NewMockCache()
mCache.On("GetAction", "test-action").Return(model.Action{}, false)
return mCache
},
err: es.ErrNotFound,
err: es.ErrTimeout,
}, {
name: "upgrade scheduled action in cache",
agent: &model.Agent{ESDocument: esd, Agent: &model.AgentMetadata{ID: "test-agent"}},
Expand Down
1 change: 0 additions & 1 deletion internal/pkg/dl/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ const (
FieldUnenrolledAt = "unenrolled_at"
FieldUpgradedAt = "upgraded_at"
FieldUpgradeStartedAt = "upgrade_started_at"
FieldUpgradeStatus = "upgrade_status"
FieldUpgradeDetails = "upgrade_details"
FieldUpgradeAttempts = "upgrade_attempts"

Expand Down