Conversation
…struct, remove nolint, deterministic tests Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com>
Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com>
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
There was a problem hiding this comment.
Pull request overview
Adds operational tooling and code to support migrating legacy Raft storage (BoltDB logs.dat/stable.dat) to the newer Pebble-backed raft.db, and provides a rolling update script that can detect/handle legacy storage during container restarts.
Changes:
- Add
internal/raftstorelegacy BoltDB → Pebble migration implementation + unit test. - Add
cmd/raft-migrateoffline migration CLI and document migration steps inREADME.md. - Add
scripts/rolling-update.shand an example env file to perform safe rolling container updates with leadership transfer and legacy-storage cleanup.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/rolling-update.sh | New rolling update script that pulls image, transfers Raft leadership when needed, restarts container, and handles legacy BoltDB detection/cleanup. |
| scripts/rolling-update.env.example | Example configuration for the rolling update script. |
| internal/raftstore/migrate.go | Implements legacy BoltDB (logs/conf buckets) migration into the existing Pebble raft store. |
| internal/raftstore/migrate_test.go | Unit test covering migration correctness for logs and stable keys. |
| cmd/raft-migrate/main.go | Adds a small CLI wrapper around MigrateLegacyBoltDB. |
| README.md | Documents when/how to run the offline migrator and required post-move steps. |
| go.mod | Adds go.etcd.io/bbolt dependency for reading legacy BoltDB files. |
| go.sum | Records checksums for the added bbolt dependency. |
scripts/rolling-update.sh
Outdated
| sudo mkdir -p "$backup_dir" | ||
| for name in logs.dat stable.dat; do | ||
| if sudo test -e "$dir/$name"; then | ||
| sudo mv "$dir/$name" "$backup_dir/$name" | ||
| moved=1 |
There was a problem hiding this comment.
The remote script uses sudo without non-interactive mode (e.g., sudo mkdir -p ...). Over SSH this can block indefinitely if passwordless sudo isn't configured. Consider using sudo -n (fail fast) and emitting a clear error instructing the operator to configure passwordless sudo (or make sudo usage configurable/optional).
| tempDir := destDir + legacyMigrationSuffix | ||
| if err := requireDestinationAbsent(tempDir); err != nil { | ||
| return "", err | ||
| } | ||
| return tempDir, nil |
There was a problem hiding this comment.
prepareMigrationPaths builds the temp dir via string concatenation (destDir + ".migrating") without normalizing destDir. If the caller passes a destination with a trailing slash (e.g. /path/raft.db/), the temp dir becomes nested under destDir (e.g. /path/raft.db/.migrating) and os.Rename(tempDir, destDir) will fail after creating partial directories. Consider normalizing destDir with filepath.Clean (and/or explicitly rejecting a trailing path separator) before computing tempDir.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com>
fix: sudo -n, SSH env passthrough, filepath.Clean for destDir
No description provided.