-
Notifications
You must be signed in to change notification settings - Fork 37
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: simplify kbench metrics to focus on key performance indicators #15
base: main
Are you sure you want to change the base?
Changes from all commits
7990141
4d5d7bd
e0d4c88
fd6f9f3
d3a3d77
6134d8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
name: build | ||
on: | ||
push: | ||
branches: | ||
- master | ||
- v* | ||
tags: | ||
- v* | ||
pull_request: | ||
workflow_dispatch: | ||
jobs: | ||
build: | ||
name: Build binaries | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout code | ||
uses: actions/checkout@v4 | ||
|
||
- name: Build binaries | ||
run: make | ||
|
||
- uses: codecov/codecov-action@v4 | ||
with: | ||
files: ./coverage.out | ||
flags: unittests | ||
token: ${{ secrets.CODECOV_TOKEN }} | ||
|
||
- name: Upload binaries | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: binaries_artifact | ||
path: ./bin/* | ||
|
||
build_push_image: | ||
name: Build and push image | ||
runs-on: ubuntu-latest | ||
needs: build | ||
if: ${{ startsWith(github.ref, 'refs/heads/') || startsWith(github.ref, 'refs/tags/') }} | ||
steps: | ||
- name: Checkout code | ||
uses: actions/checkout@v4 | ||
|
||
- name: Download binaries | ||
uses: actions/download-artifact@v4 | ||
with: | ||
name: binaries_artifact | ||
path: ./bin/ | ||
|
||
- name: Add executable permission | ||
run: | | ||
chmod +x ./bin/* | ||
|
||
- name: Copy bin folder to package | ||
run: | | ||
cp -r ./bin ./package/ | ||
|
||
# For multi-platform support | ||
- name: Set up QEMU | ||
uses: docker/setup-qemu-action@v3 | ||
- name: Set up Docker Buildx | ||
uses: docker/setup-buildx-action@v3 | ||
|
||
- name: Declare branch | ||
run: | | ||
echo "branch=${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}" >> "$GITHUB_ENV" | ||
|
||
- name: Login to Docker Hub | ||
uses: docker/login-action@v3 | ||
with: | ||
username: ${{ secrets.DOCKER_USERNAME }} | ||
password: ${{ secrets.DOCKER_PASSWORD }} | ||
|
||
# longhornio/kbench image | ||
- name: docker-publish | ||
if: ${{ startsWith(github.ref, 'refs/heads/') }} | ||
uses: docker/build-push-action@v5 | ||
with: | ||
context: ./ | ||
push: true | ||
platforms: linux/amd64,linux/arm64 | ||
tags: longhornio/kbench:${{ env.branch }}-head | ||
file: package/Dockerfile | ||
|
||
Comment on lines
+73
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider enhancing image tag for better traceability. The Docker image build and push step for branches is well-configured with multi-platform support. However, the current tagging strategy can be improved. Consider adding a timestamp or short commit hash to the image tag for better traceability. This can help identify exactly which commit the image was built from. Here's a suggested improvement: - name: Set image tag
run: |
echo "IMAGE_TAG=${{ env.branch }}-$(date +%Y%m%d%H%M%S)-${GITHUB_SHA::8}" >> $GITHUB_ENV
- name: Build and push Docker image
uses: docker/build-push-action@v5
with:
context: ./
push: true
platforms: linux/amd64,linux/arm64
tags: longhornio/kbench:${{ env.IMAGE_TAG }}
file: package/Dockerfile This will create a tag like |
||
- name: docker-publish-with-tag | ||
if: ${{ startsWith(github.ref, 'refs/tags/') }} | ||
uses: docker/build-push-action@v5 | ||
with: | ||
context: ./ | ||
push: true | ||
platforms: linux/amd64,linux/arm64 | ||
tags: longhornio/kbench:${{ github.ref_name }} | ||
file: package/Dockerfile | ||
|
||
Comment on lines
+84
to
+93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance tagging strategy for release versions. The Docker image build and push step for tags is well-configured, but the tagging strategy can be improved to provide more flexibility for users. Consider the following enhancements:
Here's a suggested improvement: - name: Generate Docker tags
if: startsWith(github.ref, 'refs/tags/')
run: |
VERSION=${GITHUB_REF#refs/tags/v}
MAJOR=${VERSION%%.*}
MINOR=${VERSION%.*}
echo "DOCKER_TAGS=longhornio/kbench:latest,longhornio/kbench:${VERSION},longhornio/kbench:${MINOR},longhornio/kbench:${MAJOR}" >> $GITHUB_ENV
- name: Build and push Docker image for tags
if: startsWith(github.ref, 'refs/tags/')
uses: docker/build-push-action@v5
with:
context: ./
push: true
platforms: linux/amd64,linux/arm64
tags: ${{ env.DOCKER_TAGS }}
file: package/Dockerfile This will create tags like |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
bs=128K | ||
iodepth=16 | ||
numjobs=4 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,9 @@ | ||
[rand-read-bw] | ||
readwrite=randread | ||
include bw-include.fio | ||
include quick-include.fio | ||
|
||
[rand-write-bw] | ||
readwrite=randwrite | ||
include bw-include.fio | ||
include quick-include.fio | ||
|
||
[seq-read-bw] | ||
[seq-read-bandwidth] | ||
readwrite=read | ||
include bw-include.fio | ||
include bandwidth-include.fio | ||
include quick-include.fio | ||
|
||
[seq-write-bw] | ||
[seq-write-bandwidth] | ||
readwrite=write | ||
include bw-include.fio | ||
include bandwidth-include.fio | ||
include quick-include.fio |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
[rand-read-bw] | ||
[rand-read-bandwidth] | ||
readwrite=randread | ||
include bw-include.fio | ||
include common-include.fio | ||
include bandwidth-include.fio | ||
include common-include.fio |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
[rand-write-bw] | ||
[rand-write-bandwidth] | ||
readwrite=randwrite | ||
include bw-include.fio | ||
include bandwidth-include.fio | ||
include common-include.fio |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
[seq-read-bw] | ||
[seq-read-bandwidth] | ||
readwrite=read | ||
include bw-include.fio | ||
include common-include.fio | ||
include bandwidth-include.fio | ||
include common-include.fio |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
[seq-write-bw] | ||
[seq-write-bandwidth] | ||
readwrite=write | ||
include bw-include.fio | ||
include bandwidth-include.fio | ||
include common-include.fio |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,10 @@ | ||
[rand-read-bw] | ||
readwrite=randread | ||
include bw-include.fio | ||
include common-include.fio | ||
|
||
[rand-write-bw] | ||
readwrite=randwrite | ||
include bw-include.fio | ||
[seq-write-bandwidth] | ||
readwrite=write | ||
include bandwidth-include.fio | ||
include common-include.fio | ||
|
||
[seq-read-bw] | ||
[seq-read-bandwidth] | ||
readwrite=read | ||
include bw-include.fio | ||
include bandwidth-include.fio | ||
include common-include.fio | ||
|
||
[seq-write-bw] | ||
readwrite=write | ||
include bw-include.fio | ||
include common-include.fio |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider refining the build job for better specificity and efficiency.
The build job covers essential steps, but there are opportunities for improvement:
The
make
command is used without a specific target. Consider specifying the exact target(s) needed for the build to ensure consistency and avoid unnecessary work.The artifact upload step includes all files in
./bin/
. To optimize storage and download times, consider being more selective about which files are uploaded.Here's a suggested improvement for the 'Build binaries' step:
And for the 'Upload binaries' step:
This will ensure only the necessary binaries are uploaded as artifacts.