Add action and state stores migrations #4305

AndersonQ · 2024-02-21T14:18:53Z

What does this PR do?

Add migration for the action/state stores and the necessary tests.

Now it migrates:

from the action store
from the deprecated YAML state store

It also removes all action store unused code and creates the internal/pkg/agent/storage/store/internal/migrations/ package. This package should contain all necessary and deprecated code to read the old stores data into current models. internal/pkg/agent/storage/store/migrations.go orchestrates the migrations.

Why is it important?

The new State Store uses a different schema and is serialised in JSON instead of YAML

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

Testing action store -> new JSON state Store:

install a 7.17 agent
perform a policy update
ensure the action is persisted in the action store.
upgrade to this version
ensure the action store file is deleted and a new store.enc is present
decrypt the encrypted store and check the same action is present.

Testing current state store -> new JSON state Store:

same as above, but start with the latest agent release
you might use a scheduled upgrade action to ensure there is an action in the action queue
the store will be encrypted by default, so:
decrypt it
check the content
upgrade the agent
check the store content again. The information should be the same, but in JSON

Related issues

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

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?
...

elasticmachine · 2024-02-22T16:48:03Z

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

mergify · 2024-02-23T06:05:19Z

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 3912-state-store-migrations upstream/3912-state-store-migrations
git merge upstream/bugfix/3912-borken-stat-store
git push upstream 3912-state-store-migrations

blakerouse

I think this is missing unit tests for the migrations.

AndersonQ · 2024-03-05T16:15:16Z

I think this is missing unit tests for the migrations.

@blakerouse could you be more specific? I didn't add explicit test for the function on store/migrations.go and store/internal/migrations/migrations.go because they're tested together with the whole store.

It's odd, GoLand and SonarQube show different code coverage.

Anyway, I can spot a few cases the test did not cover, there are tests for the migrations

pchila · 2024-03-07T12:26:23Z

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

+
+// Action is a struct to read an Action Policy Change from its old
+// YAML format.
+type Action struct {


Action has quite a broad meaning in the rest of the agent.
This looks like a very specific action store: it represents a Policy Action.
What about calling this PolicyActionStore or something more specific?

There is also a private type actionQueue []fleetapi.ScheduledAction in state_store.go that makes following this even more confusion.

Why isn't this the general fleetapi.Action? Or one of the concrete Fleet action API implementations?

This package is only for action and state store migrations. It cannot be imported outside the strore package. That's why I thought it would not be confusing, at this level there is only one meaning for Action.

Why isn't this the general fleetapi.Action? Or one of the concrete Fleet action API implementations?

because it's a code for migration, the fleetapi models do not work, they changed. There was a whole PR just to fix the schema: #4240

Also it was copied from the old store code. I kept it as it was, even thought it has properties to read different actions, as far as I can tell, there should only be 'Policy Change' actions saved

pchila · 2024-03-07T12:30:52Z

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

+}
+
+// ActionQueue is a struct to read the action queue from its old YAML format.
+type ActionQueue struct {


This queue is quite peculiar: it's the same as Action above with ExpirationTime and Version attributes added.
I assume that the structure follows the old structure we had in the store, but maybe we can add some comments about why it's defined this way 😅

pchila · 2024-03-07T12:37:54Z

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

+// LoadActionStore loads an action store from loader.
+func LoadActionStore(loader interface{ Load() (io.ReadCloser, error) }) (*Action, error) {
+	return LoadStore[Action](loader)
+}
+
+// LoadYAMLStateStore loads the old YAML state store from loader.
+func LoadYAMLStateStore(loader interface{ Load() (io.ReadCloser, error) }) (*StateStore, error) {
+	return LoadStore[StateStore](loader)
+}
+
+// LoadStore loads a YAML file.
+func LoadStore[Store any](loader interface{ Load() (io.ReadCloser, error) }) (store *Store, err error) {


Is there a specific reason to use an anonymous interface type inline here? Since it already appears 3 times in these few lines we may define something like:

Suggested change

// LoadActionStore loads an action store from loader.

func LoadActionStore(loader interface{ Load() (io.ReadCloser, error) }) (*Action, error) {

return LoadStore[Action](loader)

}

// LoadYAMLStateStore loads the old YAML state store from loader.

func LoadYAMLStateStore(loader interface{ Load() (io.ReadCloser, error) }) (*StateStore, error) {

return LoadStore[StateStore](loader)

}

// LoadStore loads a YAML file.

func LoadStore[Store any](loader interface{ Load() (io.ReadCloser, error) }) (store *Store, err error) {

type loader interface {

Load() (io.ReadCloser, error)

}

// LoadActionStore loads an action store from loader.

func LoadActionStore(loader loader) (*Action, error) {

return LoadStore[Action](loader)

}

// LoadYAMLStateStore loads the old YAML state store from loader.

func LoadYAMLStateStore(loader loader) (*StateStore, error) {

return LoadStore[StateStore](loader)

}

// LoadStore loads a YAML file.

func LoadStore[Store any](loader loader) (store *Store, err error) {

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

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

pchila · 2024-03-07T13:13:25Z

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

+		return fmt.Errorf("store state migrated from YAML failed: %w", err)
+	}
+
+	err = store.Save(jsonReader)


Maybe we should make a copy of the old store before rewriting the contents?
I am mostly thinking of having a way to restore the old contents in case of botched migration or rollback of an upgrade...

I don't know if we need it. We don't do it when saving the store i the normal, non-migration flow, why would we need it here we well? Another thing, it uses a encrypted store to save, which saves by saving a .tmp file and renaming it.

My thinking was more about having a backup in case of problems with the migration... but in that case we would also need to save it in the diagnostic bundle. It's just a nice to have, not blocking

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

pchila · 2024-03-07T14:16:40Z

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

+		}
+
+		t.Run("action store file does not exists", func(t *testing.T) {
+			ctx := context.Background()


We could use a context that expires after a while, just in case the test gets stuck...

I could do that, but those tests do not wait on anything, there isn't any expectation they might get stuck.

elastic-sonarqube · 2024-03-12T07:27:36Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
68.4% 68.4% Coverage on New Code
0.0% 0.0% Duplication on New Code

See analysis details on SonarQube

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)

AndersonQ added Team:Elastic-Agent Label for the Agent team backport-skip skip-changelog labels Feb 21, 2024

AndersonQ self-assigned this Feb 21, 2024

AndersonQ changed the base branch from main to bugfix/3912-borken-stat-store February 21, 2024 14:19

AndersonQ force-pushed the 3912-state-store-migrations branch from 8acc5da to 6edaacf Compare February 21, 2024 14:22

AndersonQ changed the title ~~[WIP] state store migrations~~ Add action and state stores migrations Feb 22, 2024

AndersonQ marked this pull request as ready for review February 22, 2024 16:48

AndersonQ requested a review from a team as a code owner February 22, 2024 16:48

AndersonQ requested review from michalpristas and blakerouse and removed request for a team February 22, 2024 16:48

AndersonQ requested review from pchila and removed request for michalpristas February 22, 2024 16:49

AndersonQ force-pushed the 3912-state-store-migrations branch from d7c4e40 to dae2b26 Compare February 23, 2024 06:16

AndersonQ mentioned this pull request Mar 5, 2024

Refactor state store #4253

Merged

3 tasks

AndersonQ force-pushed the 3912-state-store-migrations branch from 88f1d98 to de84eab Compare March 5, 2024 14:48

blakerouse reviewed Mar 5, 2024

View reviewed changes

AndersonQ added 9 commits March 7, 2024 11:07

delete action store but keep its migration

2584db6

add new migration golden files

3e8c546

add migrations

dc43804

integrate migrations on NewStateStoreWithMigration

5558612

add tests

4bb54c7

rename stuff

c768a55

extend encrypted disk store test

42441b9

close reader and log errors

9b9001a

add file header

73eaf64

AndersonQ added 8 commits March 7, 2024 11:07

instantiate and replace the named return

af354cc

skip all migration tests on darwin

92dc2b4

for sure close the reader

41bfe12

close asap and better docs

6d8b347

fix typos

a06bacc

remove debug test

5f458fe

fix linter issues

f73bd81

add more tests and minor fixes

7424ddb

AndersonQ force-pushed the 3912-state-store-migrations branch from 53405cf to 7424ddb Compare March 7, 2024 10:08

pchila reviewed Mar 7, 2024

View reviewed changes

PR changes

fdb5aa2

AndersonQ requested review from cmacknz, pchila and blakerouse March 12, 2024 06:58

pchila approved these changes Mar 19, 2024

View reviewed changes

AndersonQ merged commit 9f7ccc2 into elastic:bugfix/3912-borken-stat-store Mar 20, 2024
9 checks passed

AndersonQ deleted the 3912-state-store-migrations branch March 20, 2024 06:09

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 mentioned this pull request Apr 2, 2024

Fix and refactor the state store #4441

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add action and state stores migrations #4305

Add action and state stores migrations #4305

AndersonQ commented Feb 21, 2024 •

edited

Loading

elasticmachine commented Feb 22, 2024

mergify bot commented Feb 23, 2024

blakerouse left a comment

AndersonQ commented Mar 5, 2024

pchila Mar 7, 2024

cmacknz Mar 8, 2024

AndersonQ Mar 12, 2024

AndersonQ Mar 12, 2024 •

edited

Loading

pchila Mar 7, 2024

pchila Mar 7, 2024

pchila Mar 7, 2024

AndersonQ Mar 12, 2024

pchila Mar 19, 2024 •

edited

Loading

pchila Mar 7, 2024

AndersonQ Mar 12, 2024

elastic-sonarqube bot commented Mar 12, 2024

Add action and state stores migrations #4305

Add action and state stores migrations #4305

Conversation

AndersonQ commented Feb 21, 2024 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

Questions to ask yourself

elasticmachine commented Feb 22, 2024

mergify bot commented Feb 23, 2024

blakerouse left a comment

Choose a reason for hiding this comment

AndersonQ commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersonQ Mar 12, 2024 • edited Loading

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

pchila Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented Mar 12, 2024

Quality Gate passed

AndersonQ commented Feb 21, 2024 •

edited

Loading

AndersonQ Mar 12, 2024 •

edited

Loading

pchila Mar 19, 2024 •

edited

Loading