feat(agw): AGW s1ap-tester containerization #11308
Conversation
Signed-off-by: Ramon Melero <ramonmelero@fb.com>
|
Thanks for opening a PR! 💯
Howto
More infoPlease take a moment to read through the Magma project's
If this is your first Magma PR, also consider reading
|
github-actions
bot
added
the
component: ci
Signed-off-by: Ramon Melero <ramonmelero@fb.com>
pull-request-size
bot
added
size/L
Signed-off-by: Ramon Melero <ramonmelero@fb.com>
There was a problem hiding this comment.
Thanks a lot for your effort, I am looking forward to see s1ap test being executed in docker environments instead of VMs! 🚀
I think we could greatly improve readability, if every shell step would get a separate line. Otherwise it is easy to miss something and hard to review in general, imho.
E.g.
RUN echo 'A very long and difficult command' > a \
&& echo '1' > b \
&& echo 'another long command which is difficult in itself' > c.txt \
&& ls doesnotexist \
|| ls a \
; echo 'END'is much more readable to me, and especially the bash logical operators are much more visible, compared to
RUN echo 'A very long and difficult command' > a && echo '1' > b && echo 'another long command which is difficult in itself' > c.txt && ls doesnotexist || ls a ; echo 'END'
Signed-off-by: Ramon Melero <ramonmelero@fb.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Signed-off-by: Ramon Melero <ramonmelero@fb.com>
… s1ap-containerization
Signed-off-by: Ramon Melero <ramonmelero@fb.com>
Signed-off-by: Ramon Melero <ramonmelero@fb.com>
Signed-off-by: Ramon Melero <ramonmelero@fb.com>
Signed-off-by: Ramon Melero <ramonmelero@fb.com>
Signed-off-by: Ramon Melero <ramonmelero@fb.com>
There was a problem hiding this comment.
There were some previous comments which seem not to be addressed or replied:
- feat(agw): AGW s1ap-tester containerization #11308 (comment)
- feat(agw): AGW s1ap-tester containerization #11308 (comment)
- feat(agw): AGW s1ap-tester containerization #11308 (comment)
- feat(agw): AGW s1ap-tester containerization #11308 (comment)
- feat(agw): AGW s1ap-tester containerization #11308 (comment)
There have been a lot of changes to the code since the last review. Some new comments to those changed parts were made inline. Especially the robustness of the dockerfile is worth to be fixed imho.
| ENV SWAGGER_CODEGEN_JAR=/var/tmp/codegen/modules/swagger-codegen-cli/target/swagger-codegen-cli.jar | ||
| ENV SWAGGER_CODEGEN_DIR=/var/tmp/codegen/ |
There was a problem hiding this comment.
This could in principle still diverge, which would than lead to build failures. Flipping the variables and enforcing the respective dependency is safer. I.e.
ENV SWAGGER_CODEGEN_DIR=/var/tmp/codegen/
ENV SWAGGER_CODEGEN_JAR=${SWAGGER_CODEGEN_DIR}modules/swagger-codegen-cli/target/swagger-codegen-cli.jarThere was a problem hiding this comment.
optional PS:
I just see, if the only usage is in line 100 one might as well do something like
ENV SWAGGER_CODEGEN_DIR=/var/tmp/codegen/modules/swagger-codegen-cli/target/
ENV SWAGGER_CODEGEN_JAR=${SWAGGER_CODEGEN_DIR}swagger-codegen-cli.jarwhich would simplify line 100 to
mkdir -p ${SWAGGER_CODEGEN_DIR} && \The additional advantage would be that the variable names are also semantically more fitting, as the DIR would really have the directory stored.
There was a problem hiding this comment.
Not quite. See new inline comment please.
lte/gateway/docker/s1ap/readme.md
Outdated
| -----END CERTIFICATE-----" > /var/opt/magma/certs/rootCA.pem' | ||
|
|
||
| # Run bootstrap script and reboot | ||
| sudo bash agw_install_docker.sh |
There was a problem hiding this comment.
optional: Calling bash explicitly here might still be unnecessary given that the script defines the interpreter in line 1.
However, it is probably not hurting either, other than increased complexity.
There was a problem hiding this comment.
Fixed. Added a chmod +x so that the script is directly callable.
There was a problem hiding this comment.
Not quite. See new inline comment please.
| RUN sed -i 's/magtivate_cmd + " && " + //g' $MAGMA_ROOT/lte/gateway/python/integ_tests/s1aptests/s1ap_utils.py && \ | ||
| sed -i 's/"vagrant"/"ubuntu"/g' $MAGMA_ROOT/lte/gateway/python/integ_tests/s1aptests/s1ap_utils.py |
There was a problem hiding this comment.
This was not addressed, afaiks.
| RUN cp $MAGMA_ROOT/lte/gateway/deploy/roles/trfserver/files/traffic_server.py /usr/local/bin/traffic_server.py | ||
| RUN cp $MAGMA_ROOT/lte/gateway/deploy/roles/trfserver/files/util/traffic_messages.py /usr/lib/python3.8/util/ |
There was a problem hiding this comment.
This comment was not addressed, afaiks.
| $MAGMA_ROOT/lte/gateway/deploy/roles/magma_test/files/build_s1_tester.sh && \ | ||
| pip3 install --cache-dir $PIP_CACHE_HOME pyparsing && \ | ||
| python3 $MAGMA_ROOT/lte/gateway/deploy/roles/magma_test/files/c_parser.py && \ | ||
| sed -i 's/magtivate_cmd + " && " + //g' $MAGMA_ROOT/lte/gateway/python/integ_tests/s1aptests/s1ap_utils.py && \ |
There was a problem hiding this comment.
As @Neudrino mentioned, this is fragile and should be fixed. And in this specific case it broke with a commit three months ago: f70afcd#diff-6571bbc4f34804dcabb03702090a753fe91cdabb3b453d7228f4c09e2615f958R861
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
markdownlint
lte/gateway/docker/s1ap/readme.md|43 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|44 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|44 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|45 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|46 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|50 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|51 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|52 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|53 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|53 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|54 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|54 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|55 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|55 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|56 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|57 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|57 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|58 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|58 col 1| MD007/ul-indent Unordered list indentation [Expected: 8; Actual: 4]
lte/gateway/docker/s1ap/readme.md|59 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|59 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|60 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|60 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|61 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|62 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|66 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|67 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|68 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|69 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|69 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|70 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|70 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|71 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|71 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|72 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|73 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|73 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|74 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|74 col 1| MD007/ul-indent Unordered list indentation [Expected: 8; Actual: 4]
lte/gateway/docker/s1ap/readme.md|75 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|75 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|76 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|76 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|77 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|78 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|82| MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""] [lte/gateway/docker/s1ap/readme.md|92|](http://github.com/magma/magma/blob/7c6c58d230154e371ff05a824e0c86dc2a5f96d5/lte/gateway/docker/s1ap/readme.md#L92) MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
lte/gateway/docker/s1ap/readme.md|111| MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""] [lte/gateway/docker/s1ap/readme.md|119|](http://github.com/magma/magma/blob/7c6c58d230154e371ff05a824e0c86dc2a5f96d5/lte/gateway/docker/s1ap/readme.md#L119) MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
lte/gateway/docker/s1ap/readme.md|134| MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""] [lte/gateway/docker/s1ap/readme.md|140|](http://github.com/magma/magma/blob/7c6c58d230154e371ff05a824e0c86dc2a5f96d5/lte/gateway/docker/s1ap/readme.md#L140) MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
lte/gateway/docker/s1ap/readme.md|148| MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""] [lte/gateway/docker/s1ap/readme.md|153|](http://github.com/magma/magma/blob/7c6c58d230154e371ff05a824e0c86dc2a5f96d5/lte/gateway/docker/s1ap/readme.md#L153) MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: ""]
lte/gateway/docker/s1ap/readme.md|154| MD022/blanks-around-headings/blanks-around-headers Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Above] [Context: "#### You could also exec the test running command from the AGW instance"]
lte/gateway/docker/s1ap/readme.md|155| MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: ""] [lte/gateway/docker/s1ap/readme.md|155|](http://github.com/magma/magma/blob/7c6c58d230154e371ff05a824e0c86dc2a5f96d5/lte/gateway/docker/s1ap/readme.md#L155) MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
| s1ap/build-s1ap.sh | ||
| ``` | ||
|
|
||
| 2. Publish images to your registry |
There was a problem hiding this comment.
[markdownlint] reported by reviewdog 🐶
MD029/ol-prefix Ordered list item prefix [Expected: 1; Actual: 2; Style: 1/1/1]
| s1ap/publish-s1ap.sh yourregistry.com/yourrepo/ | ||
| ``` | ||
|
|
||
| # Deploy containerized S1APTester |
There was a problem hiding this comment.
[markdownlint] reported by reviewdog 🐶
MD025/single-title/single-h1 Multiple top-level headings in the same document [Context: "# Deploy containerized S1APTes..."]
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
markdownlint
lte/gateway/docker/s1ap/readme.md|70 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|70 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|71 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|71 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|72 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|73 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|73 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|74 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|74 col 1| MD007/ul-indent Unordered list indentation [Expected: 8; Actual: 4]
lte/gateway/docker/s1ap/readme.md|75 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|75 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|76 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|76 col 1| MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 2]
lte/gateway/docker/s1ap/readme.md|77 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|78 col 1| MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
lte/gateway/docker/s1ap/readme.md|82| MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""] [lte/gateway/docker/s1ap/readme.md|92|](http://github.com/magma/magma/blob/b2b253743d202a9a3d5a3007b08af4a5131c8cb4/lte/gateway/docker/s1ap/readme.md#L92) MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
lte/gateway/docker/s1ap/readme.md|111| MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""] [lte/gateway/docker/s1ap/readme.md|119|](http://github.com/magma/magma/blob/b2b253743d202a9a3d5a3007b08af4a5131c8cb4/lte/gateway/docker/s1ap/readme.md#L119) MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
lte/gateway/docker/s1ap/readme.md|134| MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""] [lte/gateway/docker/s1ap/readme.md|140|](http://github.com/magma/magma/blob/b2b253743d202a9a3d5a3007b08af4a5131c8cb4/lte/gateway/docker/s1ap/readme.md#L140) MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
lte/gateway/docker/s1ap/readme.md|148| MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""] [lte/gateway/docker/s1ap/readme.md|153|](http://github.com/magma/magma/blob/b2b253743d202a9a3d5a3007b08af4a5131c8cb4/lte/gateway/docker/s1ap/readme.md#L153) MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: ""]
lte/gateway/docker/s1ap/readme.md|154| MD022/blanks-around-headings/blanks-around-headers Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Above] [Context: "#### You could also exec the test running command from the AGW instance"]
lte/gateway/docker/s1ap/readme.md|155| MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: ""] [lte/gateway/docker/s1ap/readme.md|155|](http://github.com/magma/magma/blob/b2b253743d202a9a3d5a3007b08af4a5131c8cb4/lte/gateway/docker/s1ap/readme.md#L155) MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
| trfgen: | ||
| build: | ||
| context: ${BUILD_CONTEXT} | ||
| dockerfile: lte/gateway/docker/services/trfgen/Dockerfile |
There was a problem hiding this comment.
[yamllint] reported by reviewdog 🐶
[error] no new line character at the end of file (new-line-at-end-of-file)
| -----END CERTIFICATE-----" > /var/opt/magma/certs/rootCA.pem' | ||
|
|
||
| # Run bootstrap script and reboot | ||
| sudo agw_install_docker.sh |
There was a problem hiding this comment.
Should be sudo ./agw_install_docker.sh to work.
| /magma/third_party/build/bin/aioeventlet_build.sh ${DIRECTORY} && \ | ||
| dpkg -i ${DIRECTORY}/python3-aioeventlet* && \ | ||
| rm -rf ${DIRECTORY} && \ | ||
| mkdir -p ${SWAGGER_CODEGEN_DIR}modules/swagger-codegen-cli/target/ && \ |
There was a problem hiding this comment.
Have a look at this comment again please.
The second change needs to be implemented here, after the first one was implemented above.
| ENV SWAGGER_CODEGEN_JAR=/var/tmp/codegen/modules/swagger-codegen-cli/target/swagger-codegen-cli.jar | ||
| ENV SWAGGER_CODEGEN_DIR=/var/tmp/codegen/ |
There was a problem hiding this comment.
Not quite. See new inline comment please.
There was a problem hiding this comment.
-
The linter warnings should actually be resolved and not only be marked resolved.
This can mostly be done by a command line one-liner:
markdownlint --fix --config ./docs/readmes/.markdownlint.yaml ${YOUR_FILE_TO_FIX} -
I tried to run
magma/lte/gateway/docker/s1ap$ build-s1ap.sh.
This fails for me with=> ERROR [agw_s1aptester 5/9] RUN DIRECTORY="/tmp/aioeventlet" && mkdir -p ${DIRECTORY} && /magma/third_party/build/bin/aioeventlet_build.sh ${DIRECTORY} && dpkg -i ${DIRECTORY}/python3-aioeventlet* && rm -rf ${DIR 0.8s ------ > [agw_s1aptester 5/9] RUN DIRECTORY="/tmp/aioeventlet" && mkdir -p ${DIRECTORY} && /magma/third_party/build/bin/aioeventlet_build.sh ${DIRECTORY} && dpkg -i ${DIRECTORY}/python3-aioeventlet* && rm -rf ${DIRECTORY} && mkdir -p /var/tmp/codegen/modules/swagger-codegen-cli/target/ && wget -q https://repo1.maven.org/maven2/io/swagger/swagger-codegen-cli/2.2.3/swagger-codegen-cli-2.2.3.jar -O /var/tmp/codegen/modules/swagger-codegen-cli/target/swagger-codegen-cli.jar: #0 0.270 Cloning into 'deb-python-aioeventlet'... #0 0.742 error: unrecognized input ------ failed to solve: executor failed running [/bin/sh -c DIRECTORY="/tmp/aioeventlet" && mkdir -p ${DIRECTORY} && /magma/third_party/build/bin/aioeventlet_build.sh ${DIRECTORY} && dpkg -i ${DIRECTORY}/python3-aioeventlet* && rm -rf ${DIRECTORY} && mkdir -p ${SWAGGER_CODEGEN_DIR} && wget -q https://repo1.maven.org/maven2/io/swagger/swagger-codegen-cli/${CODEGEN_VERSION}/swagger-codegen-cli-${CODEGEN_VERSION}.jar -O ${SWAGGER_CODEGEN_JAR}]: exit code: 128 -
Somehow my inline comments did not group well. Also see above please.
| ARG PYTHON3_PIP_VERSION=20.0.2-5ubuntu1.6 | ||
| ARG PYTHON3_VENV_VERSION=3.8.2-0ubuntu2 | ||
| ARG VIRTUALENV_VERSION=20.0.17-1ubuntu0.4 | ||
| ARG PYTHON_VERSION=3.8 |
There was a problem hiding this comment.
The previously alphabetically sorted list becomes now unsorted by this addition. I think we should add it to the correct position.
|
@rmeleromira , this looks to be stale and not much moves. Can we close it? |
maxhbr
added
wontfix
|
This issue or pull request has exceeded its lifecylce and was automatically closed. Should you wish to continue working on this, please reopen it. |
Signed-off-by: Ramon Melero ramonmelero@fb.com
Summary
This is the initial commit with both build steps working for the s1aptester on an ubuntu 20.04 container.
Additional Information