-
Notifications
You must be signed in to change notification settings - Fork 25
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
Unified Dockerfile Image for Spark Jobs #174
Unified Dockerfile Image for Spark Jobs #174
Conversation
2a222be
to
8cda64f
Compare
Can you update the documentation to reflect the changes? |
470ad43
to
bd70023
Compare
COPY plugins/policy-recommendation/policy_recommendation_job.py /opt/spark/work-dir/policy_recommendation_job.py | ||
COPY plugins/policy-recommendation/policy_recommendation_utils.py /opt/spark/work-dir/policy_recommendation_utils.py | ||
COPY plugins/policy-recommendation/antrea_crd.py /opt/spark/work-dir/antrea_crd.py | ||
COPY plugins/anomaly-detection/AnomalyDetection.py /opt/spark/work-dir/AnomalyDetection.py | ||
COPY plugins/anomaly-detection/requirements.txt /opt/spark/work-dir/requirements.txt |
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.
nit: let's have a dedicated filename for anomaly-detection/requirements.txt, since there might be other requirements.txt for future spark jobs.
RUN pip3 install --upgrade pip && \ | ||
pip3 install pyyaml && \ | ||
pip3 install kubernetes && \ | ||
pip3 install -r /opt/spark/work-dir/requirements.txt |
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.
indent alignment
@@ -1,7 +1,7 @@ | |||
FROM gcr.io/spark-operator/spark-py:v3.1.1 | |||
|
|||
LABEL maintainer="Antrea <[email protected]>" | |||
LABEL description="A docker image to deploy policy recommendation Spark job." | |||
LABEL description="A docker image to deploy policy recommendation and throughput anomaly detector Spark job." |
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.
detector->detection
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.
Overall LGTM
.github/workflows/build.yml
Outdated
|
||
check-policy-recommendation-changes: | ||
name: Check whether policy-recommendation image needs to be built based on diff | ||
|
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.
Remove the extra spaces?
RUN pip3 install --upgrade pip && \ | ||
pip3 install pyyaml && \ | ||
pip3 install kubernetes && \ | ||
pip3 install -r /opt/spark/work-dir/requirements.txt |
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.
pip3 install -r /opt/spark/work-dir/requirements.txt | |
pip3 install -r /opt/spark/work-dir/requirements.txt |
Extra space
23d2aaa
to
165cf8b
Compare
/theia-test-e2e |
3 similar comments
/theia-test-e2e |
/theia-test-e2e |
/theia-test-e2e |
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.
LGTM, please remember to edit your commit message merging. I guess "startTime: 2022-08-24T10:45:57Z" is added unexpectedly?
Currently Theia supports separate dockerfiles for anomalydetector and policyrecommendation, this PR helps unify these dockerfiles resulting into lesser storage space. Unified Dockerimage now uses 1.4 GB of image space than earlier 2.4GB combined Spark Jobs constants have been moved under utils.go in order to avoid duplicacy of constants Solve #161 Signed-off-by: Tushar Tathgur <[email protected]>
165cf8b
to
1d48515
Compare
/theia-test-e2e |
Codecov Report
@@ Coverage Diff @@
## main #174 +/- ##
==========================================
- Coverage 52.97% 52.80% -0.17%
==========================================
Files 74 78 +4
Lines 5720 6692 +972
Branches 41 0 -41
==========================================
+ Hits 3030 3534 +504
- Misses 2379 2852 +473
+ Partials 311 306 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
LGTM. You can merge this pull request once you fix the file name issue and the test passes. Thanks for your work.
COPY plugins/policy-recommendation/policy_recommendation_job.py /opt/spark/work-dir/policy_recommendation_job.py | ||
COPY plugins/policy-recommendation/policy_recommendation_utils.py /opt/spark/work-dir/policy_recommendation_utils.py | ||
COPY plugins/policy-recommendation/antrea_crd.py /opt/spark/work-dir/antrea_crd.py | ||
COPY plugins/anomaly-detection/AnomalyDetection.py /opt/spark/work-dir/AnomalyDetection.py | ||
COPY plugins/anomaly-detection/requirements.txt /opt/spark/work-dir/anomaly_detector_requirements.txt |
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.
Suggestion: follow our naming convention of python files, AnomalyDetection.py
-> anomaly_detection.py
, anomaly_detector_requirements.txt
-> anomaly_detection_requirements.txt
.
1d48515
to
b55d2eb
Compare
/theia-test-e2e |
Signed-off-by: Tushar Tathgur <[email protected]>
b55d2eb
to
557cd9a
Compare
/theia-test-e2e |
/theia-test-e2e |
2 similar comments
/theia-test-e2e |
/theia-test-e2e |
Currently Theia supports separate docker files for anomalydetector and policyrecommendation, this PR helps unify these docker files resulting into lesser storage space.
Solve #161