Fix Dockerfile CMD to correct instance manager binary name. #664
Conversation
aznashwan
force-pushed
the
fix-docker-cmd
branch
from
154ba5b
to
639cc2d
Compare
This patch fixes the CMD in the longhorn-instance-manager container image. Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
aznashwan
force-pushed
the
fix-docker-cmd
branch
from
639cc2d
to
f8ea3e0
Compare
|
I am unfortunately unsure what the commit message linter wants from me, but I'll gladly take suggestions on how to fix it. |
|
This PR is stale because it has been open for 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
WalkthroughThe changes in the Dockerfile involve modifications to the build and installation processes across three stages. The base images remain unchanged, but the installation of additional development tools has been included. The build commands for various components have been slightly adjusted, particularly for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Docker
participant Components
User->>Docker: Build Image
Docker->>Components: Install golint
Docker->>Components: Install cmake, gcc, etc.
Docker->>Components: Build liblonghorn, TGT, spdk, etc.
Docker->>Components: Copy binaries
Docker->>User: Image Ready
User->>Docker: Run Container
Docker->>Components: Start longhorn-instance-manager
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- package/Dockerfile (1 hunks)
🔇 Additional comments (2)
package/Dockerfile (2)
Line range hint
175-176: Clarify the need for both instance manager binaries.The Dockerfile copies two similarly named binaries:
/usr/local/bin/longhorn-instance-manager/usr/local/bin/instance-managerPlease clarify if both are needed or if one should be removed to avoid confusion.
Let's check if they're different:
#!/bin/bash # Description: Compare the two binaries # Expected: If different, they serve different purposes. If identical, one might be redundant. # Check if binaries are identical cmp /usr/local/bin/longhorn-instance-manager /usr/local/bin/instance-manager || echo "Binaries are different"
180-180: LGTM! The CMD now correctly references the existing binary.The change fixes the "[FATAL tini (7)] exec longhorn failed" error by updating the CMD to use the correct binary name that is copied into the image.
Let's verify the binary paths and permissions:
✅ Verification successful
The CMD change is correct and safe to use
The verification confirms that:
- Only
longhorn-instance-manageris copied to/usr/local/bin/in the Dockerfile- There is no separate
instance-managerbinary being copied- The CMD correctly references the binary that is actually present in the image
The original review comment's concern about two similar binaries was unfounded, as there is only one binary being copied into the image.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify binary existence and permissions # Expected: Both commands should succeed, confirming binary exists and is executable # Check if the binary exists and is executable ls -l /usr/local/bin/longhorn-instance-manager ls -l /usr/local/bin/instance-managerLength of output: 204
Script:
#!/bin/bash # Let's verify the existence of binaries in a different way fd "longhorn-instance-manager|instance-manager" /usr/local/bin || echo "No files found" # Let's also check the Dockerfile to understand the binary copying rg "COPY.*bin.*instance-manager" package/DockerfileLength of output: 318
|
|
||
| CMD ["longhorn"] | ||
| CMD ["longhorn-instance-manager"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Docker best practices improvements.
While the current change fixes the immediate issue, consider these improvements:
- Add a WORKDIR directive before CMD
- Use the full path in CMD for clarity
+WORKDIR /usr/local/bin
-CMD ["longhorn-instance-manager"]
+CMD ["/usr/local/bin/longhorn-instance-manager"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CMD ["longhorn"] | |
| CMD ["longhorn-instance-manager"] | |
| WORKDIR /usr/local/bin | |
| CMD ["/usr/local/bin/longhorn-instance-manager"] |
|
This PR was closed because it has been stalled for 10 days with no activity. |
Which issue(s) this PR fixes:
None.
Considering the
longhorn-instance-managerexecutable requires at least some arguments to it so everyone who's ever used this image overrodeCMD, this is not really an issue in practice, it's only incorrect if you pass no arguments.What this PR does / why we need it:
Because the current
CMDarg which gets passed totinidoesn't exist, leading to this error if you run the image plainly:$ docker run -it longhornio/longhorn-instance-manager:v1.7.0 [FATAL tini (7)] exec longhorn failed: No such file or directorySpecial notes for your reviewer:
Binary built as
longhorn-instance-managerhere.Binary added to Docker image under the same
longhorn-instance-managername here.I am 95% sure the cause of this is someone just copy-pasting the Engine Dockerfile, whose binary is indeed plainly named
longhorn.Additional documentation or context
Summary by CodeRabbit
New Features
longhorn-instance-manager, enhancing functionality.Bug Fixes
Chores