Skip to content
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.

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

Vault 20270 docker test raft wal #24463

Merged
merged 4 commits into from
Jan 17, 2024
Merged

Conversation

hghaf099
Copy link
Contributor

@hghaf099 hghaf099 commented Dec 8, 2023

Addresses https://hashicorp.atlassian.net/browse/VAULT-20270

Adding tests to satisfy the following:

* raft-wal doesn’t lose data

* There is no migration procedure for individual nodes. The correct procedure is destroying existing raft-boltdb nodes and starting brand new nodes that use raft-wal (and vice-versa)

* Having a cluster of mixed nodes, some using raft-boltdb and some using raft-wal, is not a problem.

* Nodes running raft-wal can restore snapshots taken while raft-boltdb

adding a migration test using snapshot restore
@hghaf099 hghaf099 requested review from a team as code owners December 8, 2023 23:24
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Dec 8, 2023
Copy link

github-actions bot commented Dec 8, 2023

CI Results:
Failures:

Test Type Package Test Logs
vault/external_tests/sealmigration TestSealMigration_ShamirToTransit_Post14 view test results
vault/external_tests/sealmigration TestSealMigration_ShamirToTransit_Post14/raft view test results
vault/external_tests/sealmigration TestSealMigration_TransitToShamir_Post14 view test results
vault/external_tests/sealmigration TestSealMigration_TransitToShamir_Post14/raft view test results
vault/external_tests/sealmigration TestSealMigration_TransitToTransit view test results
vault/external_tests/sealmigration TestSealMigration_TransitToTransit/raft view test results
race vault/external_tests/sealmigration TestSealMigration_ShamirToTransit_Post14 view test results
race vault/external_tests/sealmigration TestSealMigration_ShamirToTransit_Post14/raft view test results
race vault/external_tests/sealmigration TestSealMigration_TransitToShamir_Post14 view test results
race vault/external_tests/sealmigration TestSealMigration_TransitToShamir_Post14/raft view test results

and 12 other tests

Copy link

github-actions bot commented Dec 8, 2023

Build Results:
All builds succeeded! ✅

@schavis
Copy link
Contributor

schavis commented Dec 13, 2023

Removing vault-education-approvers from the PR as there don't seem to be any documentation updates. But feel free to re-add if need.

@schavis schavis removed the request for review from a team December 13, 2023 18:22
Copy link
Contributor

@raskchanky raskchanky left a comment

Choose a reason for hiding this comment

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

vault/external_tests/raft/raft_binary/raft_test.go Outdated Show resolved Hide resolved
vault/external_tests/raft/raft_binary/raft_test.go Outdated Show resolved Hide resolved
vault/external_tests/raft/raft_binary/raft_test.go Outdated Show resolved Hide resolved
vault/external_tests/raft/raft_binary/raft_test.go Outdated Show resolved Hide resolved
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Nice. These test looks great!

@hghaf099 hghaf099 requested a review from raskchanky January 16, 2024 18:17
// caching the old cluster's barrier keys
oldBarrierKeys := cluster.GetBarrierKeys()
// clean up the old cluster as there is no further use to it
cluster.Cleanup()
Copy link
Contributor

Choose a reason for hiding this comment

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

This call has already been deferred from the beginning of the function. This line means Cleanup() gets called twice on the original cluster. Is that problematic or no big deal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it won't cause any issue. It just tries to clean up whatever is left, and if there is nothing left or even had been cleaned up before, it just moves on.

Copy link
Contributor

@raskchanky raskchanky left a comment

Choose a reason for hiding this comment

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

I had one small question but otherwise this looks great to me!

@hghaf099 hghaf099 merged commit 1bb29b3 into raft-wal Jan 17, 2024
96 of 101 checks passed
@hghaf099 hghaf099 deleted the vault-20270-docker-test-raft-wal branch January 17, 2024 00:27
raskchanky added a commit that referenced this pull request Jan 25, 2024
* Implement raft-wal

* go mod tidy

* add metrics, fix a panic

* fix the panic for real this time

* PR feedback

* refactor tests to use a helper and reduce duplication

* add a test to verify we don't use raft-wal if raft.db exists

* add config to enable the verifier

* add tests for parsing verification intervals

* run the verifier in the background

* wire up the verifier

* go mod tidy

* refactor config parsing

* remove unused function

* trying to get the verifier working

* wire up some more verifier bits

* sorted out an error, added a new test, lots of debug logging that needs to come out

* fix a bug and remove all the debugging statements

* make sure we close raft-wal stablestore too

* run verifier tests for both boltdb and raft-wal

* PR feedback

* Vault 20270 docker test raft wal (#24463)

* adding a migration test from boltdb to raftwal and back
adding a migration test using snapshot restore

* feedback

* Update physical/raft/raft.go

Co-authored-by: Paul Banks <[email protected]>

* PR feedback

* change verifier function

* make this shorter

* add changelog

* Fix Close behavior

* make supporting empty logs more explicit

* add some godocs

---------

Co-authored-by: hamid ghaf <[email protected]>
Co-authored-by: Hamid Ghaf <[email protected]>
Co-authored-by: Paul Banks <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants