feat: enable Open CAS Framework #311
Conversation
WalkthroughThe pull request modifies the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Dockerfile.dapper (2)
53-59: Consider adding build-time verification for OCF.While the OCF configuration is added, it would be beneficial to verify that OCF was successfully enabled during the build process.
Consider adding these verification steps after the make commands:
./configure --target-arch=nehalem --disable-tests --disable-unit-tests --disable-examples --without-nvme-cuse --with-ocf && \ make -j$(nproc) && \ - make install; \ + make install && \ + # Verify OCF was properly enabled + ldd /usr/local/lib/libspdk.so | grep -i ocf; \ elif [ ${ARCH} = "arm64" ]; then \ ./configure --target-arch=native --disable-tests --disable-unit-tests --disable-examples --without-nvme-cuse --with-ocf && \ DPDKBUILD_FLAGS="-Dplatform=generic" make -j$(nproc) && \ - make install; \ + make install && \ + # Verify OCF was properly enabled + ldd /usr/local/lib/libspdk.so | grep -i ocf;
53-59: Document OCF configuration impact.Since this is preparatory work for V2 data engine caching capabilities:
- Consider documenting the OCF configuration in the project's documentation
- Add a comment in the Dockerfile explaining why OCF is enabled
- Consider creating a feature flag or build argument to conditionally enable OCF support
This will help future maintainers understand the purpose and impact of these changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile.dapper(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
Dockerfile.dapper (3)
Line range hint
1-1: Verify golang version compatibility.The base image
registry.suse.com/bci/golang:1.23specifies Go version 1.23, which doesn't exist yet (current stable is 1.21.x as of January 2025).Please verify if this is a typo or if there's a specific reason for using this version. Consider using a stable version like 1.21.
94-94: LGTM! Good addition of ldconfig.The addition of
ldconfigis necessary after installing new shared libraries to update the shared library cache. This is a good practice.
53-53: Verify OCF dependencies and compatibility.The addition of
--with-ocfflag enables OCF support in SPDK. However, we should ensure all necessary OCF dependencies are properly installed.Run this script to check if OCF dependencies are available in the base image:
Also applies to: 57-57
✅ Verification successful
OCF dependencies are properly handled via git submodule
The
--with-ocfflag is correctly configured to use OCF's git submodule by default, which ensures all necessary OCF dependencies are built from source during SPDK compilation. No additional system packages are required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for OCF dependencies in the base image # Check if OCF related packages are available in the repositories zypper search -s ocf zypper search -s libocf # Verify if SPDK's OCF documentation mentions additional dependencies curl -s https://raw.githubusercontent.com/spdk/spdk/master/doc/ocf.md | grep -i "requirement\|depend\|install"Length of output: 355
Script:
#!/bin/bash # Check Dockerfile base image and OCF setup fd Dockerfile --exec cat {} \; # Check SPDK's configure script for OCF requirements curl -s https://raw.githubusercontent.com/spdk/spdk/master/configure | grep -A 5 -B 5 "with-ocf" # Look for any OCF setup files fd -t f -e sh -e mk | xargs grep -l "ocf"Length of output: 1309
liyimeng
force-pushed
the
main
branch
4 times, most recently
from
cec88ac to
e0a241c
Compare
longhorn 10292 Enable OCF https://github.com/Open-CAS, prepare to add caching to V2 data engine partially fix longhorn/longhorn#10292 Signed-off-by: Liyi Meng <[email protected]>
Enable OCF https://github.com/Open-CAS, prepare to add caching to V2 data engine
partially fix longhorn/longhorn#10292
Which issue(s) this PR fixes:
Issue #10292
What this PR does / why we need it:
Prepare for OCF support
Special notes for your reviewer:
This is a minor change, unlikely adding too much into the finally binary. So even we are not able to implement caching right now, this won't add too much negative impact, I guess.
Additional documentation or context