-
Notifications
You must be signed in to change notification settings - Fork 100
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
Add simple installation process for the VMware VDDK #666
Conversation
@@ -9,6 +9,17 @@ LABEL name="manageiq-base-worker" \ | |||
|
|||
COPY container-assets/entrypoint /usr/local/bin | |||
COPY container-assets/manageiq_liveness_check /usr/local/bin | |||
COPY container-assets/VMware-* /tmp/ | |||
|
|||
RUN cd /tmp && \ |
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.
Can you add a comment here, something like:
# Install the VDDK if found in /tmp/VMware-*
Aside, we may want to provide an ARG, defaulted to /tmp/VMware-*
instead of guessing the location?
cc @agrare
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.
Oh sorry...I misunderstood that the caller puts it in the container-assets, and it's copied into /tmp inside the container. This is fine.
Would be nice to avoid the layer that has the VDDK in it however I'm not sure how.
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.
Maybe use a multi-stage build and COPY it into the first stage?
cp -rf vmware-vix-disklib-distrib/ /usr/lib/vmware-vix-disklib/ && \ | ||
for SO in $(ls /usr/lib/vmware-vix-disklib/lib64/libvixDiskLib.so*); do ln -s $SO /usr/lib/$(basename $SO); done && \ | ||
ldconfig && \ | ||
rm -rf /tmp/VMware-* /tmp/vmware-* |
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.
@bdunne could this live in a helper script that also lives in container-assets/
like container-assets/install-vmdk
so that we don't have to one-line this relatively complex bash statement?
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.
should it not be container-assets/install-vddk
or container-assets/install-vmware-vddk
?
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.
👍 yes s/vmdk/vddk/
was a typo
Checked commit bdunne@af8f9d0 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint |
filename=$(ls VMware-*) | ||
|
||
# Skip the install if the VDDK library is not found | ||
[[ -f $filename ]] && tar -zxvf VMware-* || mkdir vmware-vix-disklib-distrib |
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.
Looked weird to create an empty directory if there were no vddk files but I see you need it so that https://github.com/ManageIQ/manageiq-pods/pull/666/files#diff-cd386fc86f89ae5eef0d1a52e06f621b5d8286f39ddc055e1cfaa7229b6316d9R20 will work (note to self)
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.
Yeah, I don't really like it either, but it seemed like the cleanest approach
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
Fixes: #538
I'm not sure if we want to go this route or have a dedicated
Dockerfile
that builds on top of the worker base image, but since it's used for both scanning and webmks, I thought this might be easier. Either way, the code would be the same.