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

Refactored the OSB build system #632

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Conversation

gkamat
Copy link
Collaborator

@gkamat gkamat commented Aug 29, 2024

Description

Restructured the entire OSB build system, including standalone, Docker and via GitHub actions.

Testing

Tested out most combinations of the workflows above.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Hi @gkamat ,

Add some comments.

Thanks.

cp -a opensearch-benchmark-git/* ./
echo "Disable VERSION arg to enter docker build test mode"
PLATFORM=${{ matrix.platform }}
PLATFORM=`echo $PLATFORM | tr '/' '-'`
docker buildx build --platform ${{ matrix.platform }} --build-arg BUILD_ENV=testing --build-arg BUILD_DATE=`date -u +%Y-%m-%dT%H:%M:%SZ` -f "docker/Dockerfile" -t "osb/osb-$PLATFORM" -o type=docker .
docker images | grep "osb/osb-$PLATFORM"
tag=osb/osb-`echo ${{ matrix.platform }} | tr '/' '-'`
set -x
Copy link
Member

Choose a reason for hiding this comment

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

I understand your changes here but this change combined with the Dockerfile change basically cause the docker-build check to be inaccurate.

It will only check docker build based on merged code, instead of the commit sent with the PR. You can only detect issues once you merge the code, which defeat the purpose of PR check.

My original design was meant to have test mode testing the PR commit, while prod mode use the original pypi method. I think we should bring back the test mode, but only change prod mode to build from source instead of pulling from pypi, which should be the best of both approaches. Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The updated changes should pull in the PR commit now.

docker/Dockerfile Outdated Show resolved Hide resolved
@gkamat gkamat force-pushed the bld-fixes branch 22 times, most recently from 19b8175 to 6ebf085 Compare August 30, 2024 21:46
@gkamat gkamat force-pushed the bld-fixes branch 17 times, most recently from e82d2c6 to bb0e874 Compare September 3, 2024 06:52
@peterzhuamazon
Copy link
Member

Seems like docker-lint does not pass.

@gkamat gkamat force-pushed the bld-fixes branch 4 times, most recently from 05757c5 to 7ad5d76 Compare September 4, 2024 16:25
ARG VERSION
ARG BUILD_ENV=production
#
# Stage 1: build packages and compile where needed
Copy link
Collaborator

@rishabh6788 rishabh6788 Sep 4, 2024

Choose a reason for hiding this comment

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

did we get any significant size reduction in final image by splitting into multi-stage? It is fairly simple dockerfile.

Copy link
Member

Choose a reason for hiding this comment

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

See this comment:
#632 (comment)

Copy link
Collaborator Author

@gkamat gkamat Sep 4, 2024

Choose a reason for hiding this comment

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

Size reduction is as noted in the link above. However, the objective of this PR is not so much the size reduction as it is to ensure the full release build process is run when changes are checked in and errors are caught at that time rather than during release. Also, the standalone development build has been revamped and simplified as well. Finally, the integ test pathway has been separated out too.

Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Thanks @gkamat !

@gkamat gkamat merged commit 6f0ae23 into opensearch-project:main Sep 4, 2024
10 of 11 checks passed
gkamat added a commit to gkamat/opensearch-benchmark that referenced this pull request Sep 12, 2024
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.

3 participants