Add s390x Support for Cloudstack Simulator #12337
Conversation
Signed-off-by: Viddya K <viddya.k@ibm.com> Signed-off-by: Niyam Siwach <niyam@ibm.com>
There was a problem hiding this comment.
Pull request overview
This PR adds s390x architecture support to the Apache CloudStack Simulator by introducing a dedicated Dockerfile for the s390x platform. The implementation is based on the existing Dockerfile but includes architecture-specific modifications.
Key changes:
- New s390x-specific Dockerfile with architecture-appropriate gcc symlink configuration
- Modified Node.js installation approach using nvm instead of nodesource repository
- Additional Python cryptography dependencies for s390x compatibility
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nvm install 10 && \ | ||
| nvm use 10 && \ |
There was a problem hiding this comment.
The s390x Dockerfile installs Node.js version 10 using nvm, while the main Dockerfile (for x86_64) uses Node.js version 14 from nodesource. Node.js 10 reached end-of-life in April 2021 and is no longer supported or receiving security updates. Consider using Node.js 14 or later to match the main Dockerfile and ensure security and compatibility.
| nvm install 10 && \ | |
| nvm use 10 && \ | |
| nvm install 14 && \ | |
| nvm use 14 && \ |
| RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.4/install.sh | bash && \ | ||
| . /root/.nvm/nvm.sh && \ | ||
| nvm install 10 && \ | ||
| nvm use 10 && \ | ||
| NVM_BIN="$(dirname "$(nvm which node)")" && \ | ||
| ln -sf "$NVM_BIN/node" /usr/local/bin/node && \ | ||
| ln -sf "$NVM_BIN/npm" /usr/local/bin/npm && \ | ||
| cd ui && npm install && npm rebuild node-sass |
There was a problem hiding this comment.
The nvm installation approach differs from the main Dockerfile which uses the nodesource repository. While nvm provides flexibility, this creates maintenance inconsistency between architectures. If Node.js 14 from nodesource is not available for s390x, consider documenting this architectural difference with a comment explaining why nvm is required for s390x.
| python3-mysql.connector \ | ||
| python3-bcrypt \ | ||
| python3-cryptography \ | ||
| python3-cffi \ | ||
| supervisor |
There was a problem hiding this comment.
The s390x Dockerfile includes additional Python cryptography packages (python3-bcrypt, python3-cryptography, python3-cffi) that are not present in the main Dockerfile. If these packages are required for s390x-specific functionality or to address build issues on this architecture, consider adding a comment explaining why they are needed. Alternatively, if they should be in both Dockerfiles, the main Dockerfile may need updating as well.
| RUN apt-get install -y nodejs npm build-essential python3 g++ make && \ | ||
| bash | ||
|
|
||
| RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.4/install.sh | bash && \ | ||
| . /root/.nvm/nvm.sh && \ | ||
| nvm install 10 && \ | ||
| nvm use 10 && \ |
There was a problem hiding this comment.
The RUN command on line 72 installs build tools (nodejs, npm, build-essential, python3, g++, make) followed by a standalone bash command on line 73. The bash command appears incomplete and will cause the build to fail or hang. If these build tools are prerequisites for the subsequent nvm installation, they should be installed in a separate RUN layer before line 75, and line 73 should be removed.
| RUN apt-get install -y nodejs npm build-essential python3 g++ make && \ | |
| bash | |
| RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.4/install.sh | bash && \ | |
| . /root/.nvm/nvm.sh && \ | |
| nvm install 10 && \ | |
| nvm use 10 && \ | |
| RUN apt-get install -y nodejs npm build-essential python3 g++ make | |
| RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.4/install.sh | bash && \ | |
| . /root/.nvm/nvm.sh && \ | |
| nvm install 10 && \ | |
| nvm use 10 && \ | |
| nvm use 10 && \ |
| NVM_BIN="$(dirname "$(nvm which node)")" && \ | ||
| ln -sf "$NVM_BIN/node" /usr/local/bin/node && \ | ||
| ln -sf "$NVM_BIN/npm" /usr/local/bin/npm && \ | ||
| cd ui && npm install && npm rebuild node-sass |
There was a problem hiding this comment.
The order of npm operations differs from the main Dockerfile. This Dockerfile runs "npm install && npm rebuild node-sass" while the main Dockerfile runs "npm rebuild node-sass && npm install". The main Dockerfile's order is more correct as node-sass should be rebuilt after dependencies are already installed. Consider changing the order to match the main Dockerfile for consistency.
| cd ui && npm install && npm rebuild node-sass | |
| cd ui && npm rebuild node-sass && npm install |
| RUN apt-get install -y nodejs npm build-essential python3 g++ make && \ | ||
| bash |
There was a problem hiding this comment.
This block installs Node.js dependencies before the nvm installation in lines 75-82. Consider combining these installations or reordering them to improve Docker layer caching and reduce image size. The nodejs and npm packages installed here may conflict with or be redundant with the nvm-installed versions.
| RUN apt-get install -y nodejs npm build-essential python3 g++ make && \ | ||
| bash | ||
|
|
||
| RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.4/install.sh | bash && \ |
There was a problem hiding this comment.
The curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.4/install.sh | bash pattern downloads and executes remote shell code during the image build without any integrity or authenticity verification. If the nvm-sh repository or the delivery path is compromised, an attacker can serve a malicious script that will execute with full privileges inside the build, allowing them to backdoor the resulting simulator image. Prefer installing Node.js and related tooling from distribution packages or a vendored, checksummed artifact, or at minimum verify a cryptographic checksum or signature of the script before execution.
|
@viddya673 , can you explain how this works? tnx |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12337 +/- ##
============================================
- Coverage 17.51% 17.50% -0.01%
+ Complexity 15585 15582 -3
============================================
Files 5914 5914
Lines 529867 529867
Branches 64722 64722
============================================
- Hits 92782 92772 -10
- Misses 426635 426645 +10
Partials 10450 10450
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Description
This PR adds s390x architecture support for the Apache CloudStack Simulator by introducing a dedicated Dockerfile for s390x.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?