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

feat: allow configuring clear-cache statically and injecting torn writes at runtime #7

Merged
merged 13 commits into from
Jun 18, 2024

Conversation

mj-ramos
Copy link
Collaborator

@mj-ramos mj-ramos commented Jun 12, 2024

API update:

  • allow configuring clear-cache as part of the static config
  • allow injecting torn writes (torn-op and torn-seq) through the FIFO

@@ -93,6 +93,14 @@ file="output1.txt"
occurrence=5
parts=3 #or parts_bytes=[4096,3600,1260]
persist=[1,3]

[[injection]]
type="clear"
Copy link
Collaborator

@devzizu devzizu Jun 12, 2024

Choose a reason for hiding this comment

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

A couple of suggestions here before diving deep into a review:

  1. If we want to add a type="clear" fault, then we must maintain the already known syntax, which is type=clear-cache, which indicates that specifying crash=false (assume default) has the same effect has sending lazyfs::clear-cache to the faults fifo.

    • With this new type, and the option to specify crash=true, we must remove lazyfs::crash from the API and replace by lazyfs::clear-cache with more options, namely the timing, op, occurrence, and crash (e.g., lazyfs::clear-cache::timing=...::op=...::occurrence=...::crash), where crash may be omitted (optional parameter).
    • We must also ensure the user never pre-configures a clear-cache without a timing and op, but he can send lazyfs::clear-cache::crash to the fifo to simulate "crash right now".
  2. Instead of the above, why can't we just add support for the existing lazyfs::crash fault under the config file? Do we need to specify a timing for the clear cache without crashing LazyFS?

WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

TL;DR: We going with option 1, but maintaining the crash fault option for now and will probably deprecate it soon. @mj-ramos Can you please change the fault syntax of the clear fault type to clear-cache?

lazyfs/config/example.toml Outdated Show resolved Hide resolved
lazyfs/include/lazyfs/lazyfs.hpp Outdated Show resolved Hide resolved
lazyfs/include/lazyfs/lazyfs.hpp Outdated Show resolved Hide resolved
lazyfs/include/lazyfs/lazyfs.hpp Outdated Show resolved Hide resolved
lazyfs/src/main.cpp Outdated Show resolved Hide resolved
lazyfs/src/main.cpp Outdated Show resolved Hide resolved
lazyfs/src/main.cpp Show resolved Hide resolved
lazyfs/src/main.cpp Outdated Show resolved Hide resolved
lazyfs/src/main.cpp Show resolved Hide resolved
lazyfs/src/main.cpp Outdated Show resolved Hide resolved
libs/libpcache/include/faults/faults.hpp Outdated Show resolved Hide resolved
libs/libpcache/src/config/config.cpp Outdated Show resolved Hide resolved
libs/libpcache/src/faults.cpp Outdated Show resolved Hide resolved
libs/libpcache/src/faults.cpp Outdated Show resolved Hide resolved
@mj-ramos mj-ramos changed the title API update API update: clear-cache statically configured and torn writes injected in runtime Jun 13, 2024
Copy link
Collaborator

@devzizu devzizu left a comment

Choose a reason for hiding this comment

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

We'll leave some of the suggestions for later improvements.

@devzizu devzizu changed the title API update: clear-cache statically configured and torn writes injected in runtime feat: allow configuring clear-cache statically and injecting torn writes at runtime Jun 18, 2024
@devzizu devzizu merged commit 50f2aaa into main Jun 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants