Fix and refactor the state store #4441

AndersonQ · 2024-03-20T06:10:49Z

Proposed commit message:

Title: refactor the state store to use JSON

Body:
Fix action store -> state store migration tests (#4235)
* use golden file for store migration test
* remove withFile function
* migrate take in a storage.Store instead of the storage's path. It's needed so we can set the encrypted store vault's path

refactor state store (#4253)
It modifies the state store API to match the current needs.

update action model to match fleet-server schema (#4240)
* simplify fleetapi.Actions.UnmarshalJSON
* add test to ensure the state store is correctly loaded from disk
* skip state store migration tests, they will be fixes on a follow-up PR as part of #3912

add migrations for action and state stores (#4305)

What does this PR do?

Fixes the agent's state store. This PR is the following PRs combined and a few needed changes to fix the conflicts with main.

As explained here, the integration test for this will be done on another issue.

Why is it important?

Currently, the agent loses data when loading actions from the state store. With the fix, it won't loose data anymore and now the action schema and serialisation are the same for the state store and the fleetapi

Checklist

My code follows the style guidelines of this project
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
~~[ ] I have added an integration test or an E2E test~~

How to test this PR locally

reproduce the issue decribed on #3912. Now it should not fail.

Related issues

Questions to ask yourself

How are we going to support this in production?
How are we going to measure its adoption?
How are we going to debug this?
What are the metrics I should take care of?
...

* use golden file for store migration test * remove withFile function * migrate take in a storage.Store instead of the storage's path. It's needed so we can set the encrypted store vault's path

* simplify fleetapi.Actions.UnmarshalJSON * add test to ensure the state store is correctly loaded from disk * skip state store migration tests, they will be fixes on a follow-up PR as part of #3912

It modifies the state store API to match the current needs.

elasticmachine · 2024-03-20T06:10:51Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

mergify · 2024-03-20T06:11:32Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b bugfix/3912-borken-stat-store upstream/bugfix/3912-borken-stat-store
git merge upstream/main
git push upstream bugfix/3912-borken-stat-store

mergify · 2024-03-20T06:11:33Z

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

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

- take bugfix/3912-borken-stat-store as the correct one for any store change - the code ill likely be broken after this merge

internal/pkg/agent/protection/action.go

…astic-agent into bugfix/3912-borken-stat-store

AndersonQ

There is still one thing I need to check, I put a panic so the tests will fail with a TODO message for myself

AndersonQ · 2024-06-20T12:39:53Z

internal/pkg/agent/application/actions/handlers/handler_action_diagnostics.go

 	// if on-disk creation fails an in-memory buffer is used.
-	f, s, err := h.diagFile(aDiag, uDiag, cDiag, action.ExcludeEventsLog)
+	f, s, err := h.diagFile(aDiag, uDiag, cDiag, action.Data.ExcludeEventsLog)


ok, so just to confirm, #4384 is already the ticket for this test, right?

AndersonQ · 2024-06-20T12:44:30Z

internal/pkg/agent/storage/store/internal/migrations/migrations.go

+			err2 = fmt.Errorf("migration storeLoad failed to close reader: %w", err2)
+		}
+		if err != nil {
+			err = errors.Join(err, err2)


The migrated stores are deleted afterwards, once the migration succeeds. Therefore not closing the file here might create problems to delete it after.

I could try to manually test it, I'd guess it could be a problem on windows

AndersonQ · 2024-06-20T12:58:26Z

internal/pkg/agent/storage/store/migrations.go

+		return nil
+	}
+
+	if action.Type != fleetapi.ActionTypePolicyChange {


Or is this only handling actions that were persisted by previous versions of the agent which are known to only write this action type?

yes, those are migrations only

AndersonQ · 2024-06-20T14:44:51Z

internal/pkg/agent/storage/store/migrations.go

+		return nil
+	}
+
+	if action.Type != fleetapi.ActionTypePolicyChange {


the thing about the unenroll is that an unenrolled agent cannot be upgraded. I can add it to migrate unenroll as well for completeness.

AndersonQ · 2024-06-20T14:52:13Z

internal/pkg/agent/storage/store/migrations.go

+			Data: fleetapi.ActionPolicyChangeData{
+				Policy: conv.YAMLMapToJSONMap(yamlStore.Action.Policy)},
+		}
+	case fleetapi.ActionTypeUnenroll:


it's a migration, no action will be added on an older version. Besides this is the whole problem of the current implementation, support reading actions from YAML with different schema. Using JSON it's already handled on fleetapi and the store uses the same approach and reuses the fleetapi code

AndersonQ · 2024-06-20T16:09:26Z

internal/pkg/agent/storage/store/state_store.go

 	s.mx.Lock()
 	defer s.mx.Unlock()

 	switch v := a.(type) {
 	case *fleetapi.ActionPolicyChange, *fleetapi.ActionUnenroll:
 		// Only persist the action if the action is different.
-		if s.state.action != nil && s.state.action.ID() == v.ID() {
+		if s.state.ActionSerializer.Action != nil &&
+			s.state.ActionSerializer.Action.ID() == v.ID() {


exactly. Now I fixed the old migration tests to run and use a golden file, looking at golden file for the action store, it has only one action whereas the state store, the evolution of the action, store has the queue.

AndersonQ · 2024-06-20T17:06:39Z

internal/pkg/agent/storage/store/state_store.go

 	}

-	reader, err := yamlToReader(&serialize)
+	reader, err := jsonToReader(&s.state)


AndersonQ · 2024-06-20T17:16:32Z

internal/pkg/agent/storage/store/state_store_test.go

+		require.Equal(t, ackToken, store.AckToken())
+	})
+
+	t.Run("errors when saving invalid action type", func(t *testing.T) {


internal/pkg/fleetapi/action.go

AndersonQ · 2024-06-20T17:20:10Z

internal/pkg/fleetapi/action.go

-	return nil
+// UnmarshalYAML prevents to decode actions from .
+func (a *Actions) UnmarshalYAML(_ func(interface{}) error) error {
+	// TODO(AndersonQ): we need this to migrate the store from YAML to JSON


cmacknz · 2024-06-20T17:52:20Z

Are there any implications for being able to downgrade the agent when a migration has to occur?

This is something Fleet does not allow today, but we want to make it possible: elastic/kibana#172745

AndersonQ · 2024-06-21T09:45:32Z

Are there any implications for being able to downgrade the agent when a migration has to occur?

This is something Fleet does not allow today, but we want to make it possible: elastic/kibana#172745

I don't believe so. The same is valid for the other migrations, the store isn't designed with a rollback in mind, the old files are deleted and the store does not support a migration rollback.

However, it isn't a problem in itself. Considering an agent rollback is intended to replace a broken agent, I'd assume it'll install the older version overriding the current installation. It'd recreate the store from scratch, thus no problem. Also trying to revert the store migration might even be worse, if the issue is on the store migration, it could corrupt the store of the rollback agent. Without extensive thought I see as the best having the agent to recreate its store from scratch.

What we might need is to keep the fleet.enc file/state for the rollback agent, avoiding to perform a new enroll during the rollback.

…astic-agent into bugfix/3912-borken-stat-store

pchila · 2024-06-21T11:51:38Z

internal/pkg/agent/storage/store/internal/migrations/migrations.go

+		if err2 != nil {
+			err2 = fmt.Errorf("migration storeLoad failed to close reader: %w", err2)
+		}
+		if err != nil {
+			err = errors.Join(err, err2)
+		}


You can probably join the errors whether err is nil or not

Suggested change

if err2 != nil {

err2 = fmt.Errorf("migration storeLoad failed to close reader: %w", err2)

}

if err != nil {

err = errors.Join(err, err2)

}

if err2 != nil {

err = errors.Join(err, fmt.Errorf("migration storeLoad failed to close reader: %w", err2))

}

AndersonQ · 2024-06-21T12:18:47Z

internal/pkg/agent/storage/store/internal/migrations/migrations.go

+	defer func() {
+		errClose := reader.Close()
+		if errClose != nil {
+			errClose = fmt.Errorf(
+				"migration storeLoad failed to close reader: %w", errClose)
+		}
+		err = errors.Join(err, errClose)
+	}()


@cmacknz, answering your question:

I tested on windows and as I expected it breaks if it fails to close the reader.
Also the current implementation only returns the error if there is already an error. But I changed it now, better to return it anyway.

here is what happens if the reader isn't closed:

--- FAIL: TestStoreMigrations (1.65s) --- FAIL: TestStoreMigrations/action_store_to_YAML_state_store (1.65s) --- FAIL: TestStoreMigrations/action_store_to_YAML_state_store/unenroll (1.65s) migrations_test.go:147: Error Trace: C:/vagrant/internal/pkg/agent/storage/store/migrations_test.go:147 Error: Received unexpected error: migration storeLoad failed to close reader: fake reader.Close() error Test: TestStoreMigrations/action_store_to_YAML_state_store/unenroll Messages: migration action store -> state store failed testing.go:1225: TempDir RemoveAll cleanup: remove C:\Users\vagrant\AppData\Local\Temp\1\TestStoreMigrationsaction_store_to_YAML_state_storeunenroll2169855176\001\action_store.yml: The process cannot access the file because it is being used by another process. FAIL FAIL github.com/elastic/elastic-agent/internal/pkg/a

to emulate this error, I just removed the code closing the reader

…astic-agent into bugfix/3912-borken-stat-store

pchila

A couple of typos and I'm not entirely comfortable with silently making SetAction a no-op if the action type is unexpected...

pchila · 2024-06-21T13:16:29Z

internal/pkg/agent/storage/store/migrations.go

+	for _, a := range yamlStore.ActionQueue {
+		if a.Type != fleetapi.ActionTypeUpgrade {
+			log.Warnf(
+				"loaded a unsupported %s action from the deprecated YAML state store action queue, ignoring it",


Suggested change

"loaded a unsupported %s action from the deprecated YAML state store action queue, ignoring it",

"loaded a unsupported %s action from the deprecated YAML state store action queue, it will be dropped",

pchila · 2024-06-21T13:40:30Z

internal/pkg/agent/storage/store/state_store.go

-		}
+	if st.Version != Version {
+		return nil, fmt.Errorf(
+			"invalid state store version, got %q isntead of %s",


Suggested change

"invalid state store version, got %q isntead of %s",

"invalid state store version, expected %q, got %q ",

pchila · 2024-06-21T13:45:11Z

internal/pkg/agent/storage/store/state_store.go

+// SetAction sets the current action. It accepts ActionPolicyChange or
+// ActionUnenroll. Any other type will be silently discarded.


Could you add a default case in the switch statement with a debug log with action id and type being discarded because it's not supported ?
Maybe our future selves will be grateful to have it 😅
Furthermore, we check the Action type both here and in Save()... is there a difference between thse two checks ?

pchila · 2024-06-21T14:11:11Z

internal/pkg/agent/storage/store/state_store_test.go

+		store.dirty = true
+		store.state.ActionSerializer.Action = fleetapi.NewAction(fleetapi.ActionTypeUnknown)
+		err = store.Save()
+		require.Error(t, err, "expected and error when saving sore with invalid state")


Suggested change

require.Error(t, err, "expected and error when saving sore with invalid state")

require.Error(t, err, "expected an error when saving store with invalid state")

AndersonQ · 2024-06-21T15:25:33Z

A couple of typos and I'm not entirely comfortable with silently making SetAction a no-op if the action type is unexpected...

this is the current behaviour. I understand your concern and actually agree with you. I just don't think this PR is the right place to change the store's behaviour

elastic-sonarqube · 2024-06-21T16:00:58Z

Quality Gate passed

Issues
3 New issues
5 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

AndersonQ added 4 commits February 15, 2024 14:22

Fix action store -> state store migration tests (#4235)

db6c7a6

* use golden file for store migration test * remove withFile function * migrate take in a storage.Store instead of the storage's path. It's needed so we can set the encrypted store vault's path

update action model to match fleet-server schema (#4240)

19c8fd0

* simplify fleetapi.Actions.UnmarshalJSON * add test to ensure the state store is correctly loaded from disk * skip state store migration tests, they will be fixes on a follow-up PR as part of #3912

refactor state store (#4253)

a7b7bca

It modifies the state store API to match the current needs.

add migrations for action and state stores (#4305)

9f7ccc2

AndersonQ added bug Something isn't working Team:Elastic-Agent Label for the Agent team labels Mar 20, 2024

AndersonQ self-assigned this Mar 20, 2024

AndersonQ requested a review from a team as a code owner March 20, 2024 06:10

AndersonQ requested review from michalpristas and pchila March 20, 2024 06:10

mergify bot added the backport-skip label Mar 20, 2024

AndersonQ mentioned this pull request Mar 26, 2024

Scheduled upgrade fails with error parsing version "" if agent restarts before the upgrade starts #3912

Closed

7 tasks

AndersonQ added 3 commits March 27, 2024 16:29

merge 'upstream/main' into bugfix/3912-borken-stat-store

1af2c59

- take bugfix/3912-borken-stat-store as the correct one for any store change - the code ill likely be broken after this merge

fix stores conflicts

1c3dfe0

adjust actions in protection package and diagnostics action

f61bbdf

AndersonQ force-pushed the bugfix/3912-borken-stat-store branch from fe0f59c to f61bbdf Compare April 2, 2024 12:05

AndersonQ added 4 commits April 2, 2024 14:06

Merge branch 'main' into bugfix/3912-borken-stat-store

ac8591e

fix action diagnostics handler

9589a5d

it should not be there

4032005

use .json instead of .yml/.yaml

bd1f715

AndersonQ commented Apr 2, 2024

View reviewed changes

internal/pkg/agent/protection/action.go Show resolved Hide resolved

AndersonQ requested review from blakerouse and removed request for michalpristas April 2, 2024 12:37

AndersonQ added 4 commits April 2, 2024 14:58

fix more tests

27920c3

split actionDiagnosticsData into its own type and fix another test

cc4d6b4

add changelog

85324a5

fix signed action data schema

27a235e

AndersonQ added 7 commits June 20, 2024 14:26

Merge branch 'main' into bugfix/3912-borken-stat-store

a9832f8

improve docs/comments

a305fd4

fix store dirty state being cleaned on failed save and add tests

e949aa1

fix docs and remove TODO

057fc26

add tests to load store

100375f

WIP

785372b

Merge branch 'bugfix/3912-borken-stat-store' of github.com:elastic/el…

bb44a11

…astic-agent into bugfix/3912-borken-stat-store

AndersonQ commented Jun 20, 2024

View reviewed changes

migration: add tests for empty and unknown actions

a1ae941

AndersonQ added 3 commits June 21, 2024 11:47

Merge branch 'main' into bugfix/3912-borken-stat-store

113bf22

always return the reader.Close error

eba70e6

Merge branch 'bugfix/3912-borken-stat-store' of github.com:elastic/el…

f450435

…astic-agent into bugfix/3912-borken-stat-store

pchila reviewed Jun 21, 2024

View reviewed changes

Merge branch 'main' into bugfix/3912-borken-stat-store

d2b9fda

AndersonQ commented Jun 21, 2024

View reviewed changes

AndersonQ added 2 commits June 21, 2024 15:05

do not log errors

987c9f6

Merge branch 'bugfix/3912-borken-stat-store' of github.com:elastic/el…

57dfe49

…astic-agent into bugfix/3912-borken-stat-store

pchila reviewed Jun 21, 2024

View reviewed changes

AndersonQ added 2 commits June 21, 2024 17:33

add debug log when dropping actions on SetAction

e61cf6d

fix typos/better logs

57dec1e

AndersonQ requested review from pchila and cmacknz June 21, 2024 15:33

AndersonQ enabled auto-merge (squash) June 21, 2024 15:40

pchila approved these changes Jun 27, 2024

View reviewed changes

AndersonQ merged commit b4877f5 into main Jun 27, 2024
13 checks passed

AndersonQ deleted the bugfix/3912-borken-stat-store branch June 27, 2024 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and refactor the state store #4441

Fix and refactor the state store #4441

AndersonQ commented Mar 20, 2024 •

edited

Loading

elasticmachine commented Mar 20, 2024

mergify bot commented Mar 20, 2024

mergify bot commented Mar 20, 2024

AndersonQ left a comment

AndersonQ Jun 20, 2024

AndersonQ Jun 20, 2024

AndersonQ Jun 20, 2024

AndersonQ Jun 20, 2024

AndersonQ Jun 20, 2024

AndersonQ Jun 20, 2024

AndersonQ Jun 20, 2024

AndersonQ Jun 20, 2024

AndersonQ Jun 20, 2024

cmacknz commented Jun 20, 2024

AndersonQ commented Jun 21, 2024 •

edited

Loading

pchila Jun 21, 2024

AndersonQ Jun 21, 2024

pchila left a comment

pchila Jun 21, 2024

pchila Jun 21, 2024

pchila Jun 21, 2024

pchila Jun 21, 2024

AndersonQ commented Jun 21, 2024

elastic-sonarqube bot commented Jun 21, 2024

	"loaded a unsupported %s action from the deprecated YAML state store action queue, ignoring it",
	"loaded a unsupported %s action from the deprecated YAML state store action queue, it will be dropped",

	"invalid state store version, got %q isntead of %s",
	"invalid state store version, expected %q, got %q ",

		// SetAction sets the current action. It accepts ActionPolicyChange or
		// ActionUnenroll. Any other type will be silently discarded.

	require.Error(t, err, "expected and error when saving sore with invalid state")
	require.Error(t, err, "expected an error when saving store with invalid state")

Fix and refactor the state store #4441

Fix and refactor the state store #4441

Conversation

AndersonQ commented Mar 20, 2024 • edited Loading

Proposed commit message:

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

Questions to ask yourself

elasticmachine commented Mar 20, 2024

mergify bot commented Mar 20, 2024

mergify bot commented Mar 20, 2024

AndersonQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmacknz commented Jun 20, 2024

AndersonQ commented Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersonQ commented Jun 21, 2024

elastic-sonarqube bot commented Jun 21, 2024

Quality Gate passed

AndersonQ commented Mar 20, 2024 •

edited

Loading

AndersonQ commented Jun 21, 2024 •

edited

Loading