Skip to content

Snapshots feature refactor #292

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.

Sign up for GitHub

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

Open
AnnaPaolaMusio wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Snapshots feature refactor #292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5f89b3e
feat: snapshots feature refactor for C++ part
AnnaPaolaMusio May 25, 2026
ce1feab
feat: added component requirement tests
AnnaPaolaMusio May 25, 2026
cb1bb1e
feat: fixed component requirement tests c++
AnnaPaolaMusio May 26, 2026
429daff
feat: fixed component requirement tests c++
AnnaPaolaMusio May 26, 2026
90f647a
feat: fixed component requirement tests c++
AnnaPaolaMusio May 27, 2026
c6ed728
feat: fixed component requirement tests c++
AnnaPaolaMusio May 27, 2026
eef40a2
feat: fixed comment
AnnaPaolaMusio May 27, 2026
9c9d22a
fixed format
AnnaPaolaMusio May 29, 2026
b772d3a
delete snapshot_rotate comp.req.
AnnaPaolaMusio Jun 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions score/kvs/docs/requirements/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,17 @@ Component Requirements

The component shall create a snapshot each time data is stored.

.. comp_req:: Snapshot Explicit Creation
:id: comp_req__kvs__snapshot_explicit_creation
:reqtype: Functional
:security: NO
:safety: ASIL_B
:satisfies: feat_req__persistency__snapshot_create
:status: valid
:belongs_to: comp__persistency_kvs

The component shall create a snapshot in the first available slot when the snapshot_create function is explicitly called.

.. comp_req:: Snapshot Maximum Number
:id: comp_req__kvs__snapshot_max_num
:reqtype: Functional
Expand All @@ -324,17 +335,6 @@ Component Requirements

The component shall assign the ID 1 to the newest snapshot and shall increment the IDs of older snapshots accordingly.

.. comp_req:: Snapshot Rotation
:id: comp_req__kvs__snapshot_rotate
:reqtype: Functional
:security: NO
:safety: ASIL_B
:satisfies: feat_req__persistency__snapshot_remove, feat_req__persistency__snapshot_restore
:status: valid
:belongs_to: comp__persistency_kvs

The component shall rotate and delete the oldest snapshot when the maximum number is reached.

.. comp_req:: Snapshot Restore
:id: comp_req__kvs__snapshot_restore
:reqtype: Functional
Expand Down
229 changes: 146 additions & 83 deletions src/cpp/src/kvs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,119 +593,141 @@ score::ResultBlank Kvs::flush()
result = score::MakeUnexpected(ErrorCode::JsonGeneratorError);
}
else

@arkjedrz arkjedrz Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use clang-format with config available in the repo for C++ files.

{
/* Rotate Snapshots */
auto rotate_result = snapshot_rotate();
if (!rotate_result)
{
result = rotate_result;
}
else
{
/* Write JSON Data */
std::string buf = std::move(buf_res.value());
result = write_json_data(buf);
}
{
/* Write JSON Data */

@arkjedrz arkjedrz Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a comment here that it writes to a file with snapshot ID 0.
Add this information to flush method docs.

std::string buf = std::move(buf_res.value());
result = write_json_data(buf);

}
}

return result;
}

/* Retrieve the snapshot count*/
score::Result<size_t> Kvs::snapshot_count() const
score::Result<std::size_t> Kvs::snapshot_count() const
{
score::Result<size_t> result = score::MakeUnexpected(ErrorCode::UnmappedError);
size_t count = 0;
bool error = false;
for (size_t idx = 0; idx < KVS_MAX_SNAPSHOTS; ++idx)
for (size_t idx = 1; idx <= KVS_MAX_SNAPSHOTS; ++idx)

@arkjedrz arkjedrz Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, so snapshot ID==0 is not considered snapshot? This might be fine, but consider documenting the approach to snapshot ID in score/kvs/docs/requirements/index.rst. It should be written somewhere that 0 is reserved for current storage, and snapshots are in <1; max snapshot> range.

{
const score::filesystem::Path fname = filename_prefix.Native() + "_" + to_string(idx) + ".json";
const auto fname_exists_res = filesystem->standard->Exists(fname);
if (fname_exists_res)
if (!fname_exists_res)
{
if (false == fname_exists_res.value())
{
break;
}
/* Filesystem error: cannot determine whether the slot exists */
return score::MakeUnexpected(ErrorCode::PhysicalStorageFailure);
}
else
if (fname_exists_res.value())
{
error = true;
break;
count++;
}
count = idx + 1;
/* Slot is empty: do nothing and continue to the next slot */
}
if (error)
return count;
}

score::Result<std::size_t> Kvs::snapshot_create()

@arkjedrz arkjedrz Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please rework the function to return quickly on error. This will reduce if-else pyramids occurring in the code, with easier tracking on what's the method result.

{
score::Result<std::size_t> result = score::MakeUnexpected(ErrorCode::UnmappedError);
std::unique_lock<std::mutex> lock(kvs_mutex, std::try_to_lock);
if (lock.owns_lock())
{
result = score::MakeUnexpected(ErrorCode::PhysicalStorageFailure);
auto snapshot_count_res = snapshot_count();
if (!snapshot_count_res)
{
result = score::MakeUnexpected(static_cast<ErrorCode>(*snapshot_count_res.error()));
}
else
{
if (snapshot_count_res.value() >= snapshot_max_count())
{
result = score::MakeUnexpected(ErrorCode::QuotaExceeded);
}
else
{
auto first_free_slot_res = first_free_slot();
if (!first_free_slot_res)
{
result = score::MakeUnexpected(static_cast<ErrorCode>(*first_free_slot_res.error()));
}
else
{
const size_t new_snapshot_id = first_free_slot_res.value();
const score::filesystem::Path src_json{filename_prefix.Native() + "_0.json"};
const score::filesystem::Path src_hash{filename_prefix.Native() + "_0.hash"};
const score::filesystem::Path dst_json{filename_prefix.Native() + "_" + to_string(new_snapshot_id) + ".json"};
const score::filesystem::Path dst_hash{filename_prefix.Native() + "_" + to_string(new_snapshot_id) + ".hash"};

const auto copy_json_res = filesystem->standard->CopyFile(src_json, dst_json);

@arkjedrz arkjedrz Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This approach stores new snapshot based on snapshot 0 file. This can lead to surprising behavior if flush was not done prior to snapshot creation.
I'd consider storing current memory state as less surprising variant, but nonetheless this should be clearly stated.

if (!copy_json_res)
{
logger->LogError() << "Failed to copy snapshot JSON file to '" << dst_json << "'";
result = score::MakeUnexpected(ErrorCode::PhysicalStorageFailure);
}
else
{
const auto copy_hash_res = filesystem->standard->CopyFile(src_hash, dst_hash);
if (!copy_hash_res)
{
logger->LogError() << "Failed to copy snapshot hash file to '" << dst_hash << "'";
result = score::MakeUnexpected(ErrorCode::PhysicalStorageFailure);
}
else
{
result = new_snapshot_id;
}
}
}
}
}
}
else
{
result = count;
result = score::MakeUnexpected(ErrorCode::MutexLockFailed);
}

return result;
}

/* Retrieve the max snapshot count*/
size_t Kvs::snapshot_max_count() const
std::size_t Kvs::snapshot_max_count() const
{
return KVS_MAX_SNAPSHOTS;
}

/* Rotate Snapshots */
score::ResultBlank Kvs::snapshot_rotate()
/* Restore the key-value store from a snapshot*/
score::ResultBlank Kvs::snapshot_restore(const SnapshotId& snapshot_id)
{
score::ResultBlank result = score::MakeUnexpected(ErrorCode::UnmappedError);
std::unique_lock<std::mutex> lock(kvs_mutex, std::try_to_lock);
if (lock.owns_lock())
{
bool error = false;
for (size_t idx = KVS_MAX_SNAPSHOTS; idx > 0; --idx)
auto snapshot_count_res = snapshot_count();
if (!snapshot_count_res)
{
score::filesystem::Path hash_old = filename_prefix.Native() + "_" + to_string(idx - 1) + ".hash";
score::filesystem::Path hash_new = filename_prefix.Native() + "_" + to_string(idx) + ".hash";
score::filesystem::Path snap_old = filename_prefix.Native() + "_" + to_string(idx - 1) + ".json";
score::filesystem::Path snap_new = filename_prefix.Native() + "_" + to_string(idx) + ".json";

logger->LogInfo() << "rotating: " << snap_old << " -> " << snap_new;
/* Rename hash */
int32_t hash_rename = std::rename(hash_old.CStr(), hash_new.CStr());
if (0 != hash_rename)
result = score::MakeUnexpected(static_cast<ErrorCode>(*snapshot_count_res.error()));
}
else
{
if (snapshot_count_res.value() == 0 || snapshot_id.id >= snapshot_max_count())

@arkjedrz arkjedrz Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From previous methods I inferred that range is <1; max snapshot>, but this condition indicates that restoring for max snapshot is disallowed. Similar issue is observed in other methods.

{
if (errno != ENOENT)
{
error = true;
logger->LogError() << "error: could not rename hash file " << snap_old << ". Rename Errorcode "
<< errno;
result = score::MakeUnexpected(ErrorCode::PhysicalStorageFailure);
}
result = score::MakeUnexpected(ErrorCode::InvalidSnapshotId);
}
if (!error)
else
{
/* Rename snapshot */
int32_t snap_rename = std::rename(snap_old.CStr(), snap_new.CStr());
if (0 != snap_rename)
score::filesystem::Path restore_path = filename_prefix.Native() + "_" + to_string(snapshot_id.id + 1);
auto data_res = open_json(restore_path, OpenJsonNeedFile::Required);
if (!data_res)
{
if (errno != ENOENT)
{
error = true;
logger->LogError()
<< "error: could not rename snapshot file " << snap_old << ". Rename Errorcode " << errno;
result = score::MakeUnexpected(ErrorCode::PhysicalStorageFailure);
}
result = score::MakeUnexpected(static_cast<ErrorCode>(*data_res.error()));
}
else
{
kvs = std::move(data_res.value());
result = score::ResultBlank{};
}
}
if (error)
{
break;
}
}
if (!error)
{
result = score::ResultBlank{};
}
}
else
{
Expand All @@ -715,41 +737,61 @@ score::ResultBlank Kvs::snapshot_rotate()
return result;
}

/* Restore the key-value store from a snapshot*/
score::ResultBlank Kvs::snapshot_restore(const SnapshotId& snapshot_id)
/* Delete a snapshot*/
score::ResultBlank Kvs::snapshot_delete(const SnapshotId& snapshot_id)
{
score::ResultBlank result = score::MakeUnexpected(ErrorCode::UnmappedError);
std::unique_lock<std::mutex> lock(kvs_mutex, std::try_to_lock);
if (lock.owns_lock())
{
{
auto snapshot_count_res = snapshot_count();
if (!snapshot_count_res)
{
result = score::MakeUnexpected(static_cast<ErrorCode>(*snapshot_count_res.error()));
return score::MakeUnexpected(static_cast<ErrorCode>(*snapshot_count_res.error()));
}

const std::size_t count = snapshot_count_res.value();
if (count == 0 || snapshot_id.id >= snapshot_max_count())
{
result = score::MakeUnexpected(ErrorCode::InvalidSnapshotId);
}
else
{
/* Fail if the snapshot ID is the current KVS */
if (0 == snapshot_id.id)
/* Delete the target snapshot files (snapshot files are 1-indexed: _1.json, _2.json, ...) */
const score::filesystem::Path json_path{filename_prefix.Native() + "_" + to_string(snapshot_id.id + 1) + ".json"};
const score::filesystem::Path hash_path{filename_prefix.Native() + "_" + to_string(snapshot_id.id + 1) + ".hash"};

const auto json_exists_res = filesystem->standard->Exists(json_path);
if (!json_exists_res)
{
result = score::MakeUnexpected(ErrorCode::InvalidSnapshotId);
logger->LogError() << "Failed to check existence of snapshot JSON file '" << json_path << "'";
result = score::MakeUnexpected(ErrorCode::PhysicalStorageFailure);
}
else if (snapshot_count_res.value() < snapshot_id.id)
else if (!json_exists_res.value())
{
logger->LogError() << "Snapshot JSON file does not exist: '" << json_path << "'";

@arkjedrz arkjedrz Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please check if it prints correctly, without added whitespaces surrouding path. If so, consider reworking those logs to logger->LogError() << "Snapshot JSON file does not exist:" << json_path;

result = score::MakeUnexpected(ErrorCode::InvalidSnapshotId);
}
else
{
score::filesystem::Path restore_path = filename_prefix.Native() + "_" + to_string(snapshot_id.id);
auto data_res = open_json(restore_path, OpenJsonNeedFile::Required);
if (!data_res)
const auto remove_json_res = filesystem->standard->Remove(json_path);
if (!remove_json_res)
{
result = score::MakeUnexpected(static_cast<ErrorCode>(*data_res.error()));
logger->LogError() << "Failed to delete snapshot JSON file '" << json_path << "'";
result = score::MakeUnexpected(ErrorCode::PhysicalStorageFailure);
}
else
{
kvs = std::move(data_res.value());
result = score::ResultBlank{};
const auto remove_hash_res = filesystem->standard->Remove(hash_path);
if (!remove_hash_res)
{
logger->LogError() << "Failed to delete snapshot hash file '" << hash_path << "'";
result = score::MakeUnexpected(ErrorCode::PhysicalStorageFailure);
}
else
{
result = score::ResultBlank{};
}
}
}
}
Expand All @@ -762,6 +804,27 @@ score::ResultBlank Kvs::snapshot_restore(const SnapshotId& snapshot_id)
return result;
}

/* Returns the index (1-based) of the first snapshot slot that is free.
Checks _1, _2, _3 (up to KVS_MAX_SNAPSHOTS) and returns the first index
whose .json file does not exist. Returns OutOfStorageSpace if all slots are occupied. */
score::Result<std::size_t> Kvs::first_free_slot() const
{
for (size_t idx = 1U; idx <= KVS_MAX_SNAPSHOTS; ++idx)
{
const score::filesystem::Path fname = filename_prefix.Native() + "_" + to_string(idx) + ".json";
const auto exists_res = filesystem->standard->Exists(fname);
if (!exists_res)
{
return score::MakeUnexpected(ErrorCode::PhysicalStorageFailure);
}
if (!exists_res.value())
{
return idx; /* First free slot found */
}
}
return score::MakeUnexpected(ErrorCode::OutOfStorageSpace);
}

/* Get the filename for a snapshot*/
score::Result<score::filesystem::Path> Kvs::get_kvs_filename(const SnapshotId& snapshot_id) const
{
Expand Down
Loading
Loading