Clear agent.upgrade_attempts on upgrade complete

[jillguyonnet]

What is the problem this PR solves?

elastic/kibana#212744 adds retry logic to the task that automatically ugprades agents. Agents that were upgraded through this task have their new upgrade_attempts property populated. It is missing a way to clear this property when the upgrade completes successfully.

How does this PR solve the problem?

The change in this PR clears upgrade_attempts when the upgrade details of the agent get into UPG_WATCHING state and are processed in handleCheckin.

How to test this PR locally

This should be tested alongside elastic/kibana#212744 (or after it is merged - this is fine, since automatic upgrades are currently behind the enableAutomaticAgentUpgrades feature flag). With this change, agents upgraded through the automatic upgrade task should have their upgrade_attempts property set to null when the upgrade is successful.

Testing should also validate that upgrade_attempts stays set if the upgrade failed, e.g. after requesting an upgrade to an invalid version.

Design Checklist

• I have ensured my design is stateless and will work when multiple fleet-server instances are behind a load balancer.
• I have or intend to scale test my changes, ensuring it will work reliably with 100K+ agents connected.
• I have included fail safe mechanisms to limit the load on fleet-server: rate limiting, circuit breakers, caching, load shedding, etc.

Checklist

• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in ./changelog/fragments using the changelog tool

Related issues

Relates https://github.com/elastic/ingest-dev/issues/4720

[mergify]

This pull request does not have a backport label. Could you fix it @jillguyonnet? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit
• backport-active-all is the label that automatically backports to all active branches.
• backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
• backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

[pchila]

Code looks sensible, holding off the approval until we have a green CI run (it seems we are getting some errors from ECH when creating a stack and 503s when trying to clean it up (not sure if it's related to the failed creation)

[michel-laterman]

lgtm, merge when CI is green

[cmacknz]

There is a test you should update to check that this field is properly reset:

fleet-server/internal/pkg/api/handleCheckin_test.go

Lines 334 to 354 in ba68a24

	name: "agent has details checkin details are nil",
	agent: &model.Agent{ESDocument: esd, Agent: &model.AgentMetadata{ID: "test-agent"}, UpgradeDetails: &model.UpgradeDetails{}},
	details: nil,
	bulk: func() *ftesting.MockBulk {
	mBulk := ftesting.NewMockBulk()
	mBulk.On("Update", mock.Anything, dl.FleetAgents, "doc-ID", mock.MatchedBy(func(p []byte) bool {
	doc := struct {
	Doc map[string]interface{} `json:"doc"`
	}{}
	if err := json.Unmarshal(p, &doc); err != nil {
	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] != ""
	}), mock.Anything, mock.Anything).Return(nil)
	return mBulk
	},
	cache: func() *testcache.MockCache {
	return testcache.NewMockCache()
	},
	err: nil,

[jillguyonnet]

Upon more testing with elastic/kibana#212744, this is not behaving as expected as upgrade_attempts is cleared after failed upgrades. 👀

[jillguyonnet]

OK, my original approach didn't do what is expected, i.e. only reset upgrade_attempts if the upgrade is complete and successful. This is because in handleAck the upgrade is considered complete when done retrying, even if the outcome was failure. I was also under the impression that upgrade_details were updated as part of it, but I see that's actually handled in handleCheckin, which makes sense.

I have changed the approach to reset upgrade_attempts when the agent reports an upgrade_details.state value of UPG_WATCHING in handleCheckin. I could be missing some edge cases, but it seems to meet the primary expectation. My testing so far looks good.

@michel-laterman Would you please be able to review this approach? If it looks OK, I will update tests.

Edit: it seems we need to support agents with no upgrade details as well, so this won't be sufficient nevermind

[michel-laterman]

lgtm

[elastic-sonarqube]

Quality Gate passed

Issues
1 New issue
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[juliaElastic]

@michel-laterman could you approve again? had to fix the sonarqube failure

[blakerouse]

Looks good.