Skip to content
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

Reduced the size of installation files for cuda 9.0 #7280

Merged
merged 2 commits into from Oct 16, 2019

Conversation

JennyChenyj
Copy link

[skip ci]

Signed-off-by: Jenny Chen Jenny.Chen@ibm.com

@JennyChenyj
Copy link
Author

Hey @AdamBrousseau! The build and run command work. Thanks for reviewing it!

@JennyChenyj
Copy link
Author

Hi @keithc-ca! I got this message when building the image from the docker file.
Step 33/40 : COPY authorized_keys /home/${USER}/.ssh/authorized_keys
COPY failed: stat /var/lib/docker/tmp/docker-builder247630549/authorized_keys: no such file or directory
Do you happen to know the reason? Thanks!

@JennyChenyj
Copy link
Author

JennyChenyj commented Oct 1, 2019

Hi @keithc-ca! I got this message when building the image from the docker file.
Step 33/40 : COPY authorized_keys /home/${USER}/.ssh/authorized_keys
COPY failed: stat /var/lib/docker/tmp/docker-builder247630549/authorized_keys: no such file or directory
Do you happen to know the reason? Thanks!

Hi! I think we know the answer. I did not put the authorized_keys file and the known_hosts file next to the dockerfile. Thanks!

@AdamBrousseau
Copy link
Contributor

I'm confused why we're making this change. I thought once more testing was added, the full install would be required. Which would also mean adding Cuda to the other Dockerfiles.
@keithc-ca and @jdekonin for comment.

@keithc-ca
Copy link
Contributor

You need to follow the instructions at https://github.com/eclipse/openj9/blob/master/buildenv/jenkins/docker-slaves/x86/centos6.9/Dockerfile#L20

@keithc-ca
Copy link
Contributor

I'm confused why we're making this change. I thought once more testing was added, the full install would be required. Which would also mean adding Cuda to the other Dockerfiles.

We need a clear understanding of the purpose of the docker images we seek to create. If it's just used for building the VM, then this is sufficient, if we want an image that can be used for testing a VM then more will be needed (not the least of which is actual GPU hardware).

@AdamBrousseau
Copy link
Contributor

purpose of the docker images

These have always been for build and/or test in the farm as well as for dev use. However, the likelihood of running them on a host with a card is slim imo. Which leads me to believe we would only ever install the full kit on a machine with the hardware to back it up. If this is true then this change makes sense to me now.

@AdamBrousseau
Copy link
Contributor

Note, there is a Cuda(7) install in the cent7 ppcle image which will need this change as well and the change from #7254. I'm not sure if we should also have one in the ub16 390 image.

@keithc-ca
Copy link
Contributor

I'm not sure if we should also have one in the ub16 390 image.

CUDA is not supported on s390.

@AdamBrousseau
Copy link
Contributor

jenkins build docker x86 centos6.9

Comment on lines 194 to 196
RUN mkdir -p ${CUDA_HOME}/nvvm
COPY --from=cuda-dev ${CUDA_HOME}/include ${CUDA_HOME}/include
COPY --from=cuda-dev ${CUDA_HOME}/nvvm/include ${CUDA_HOME}/nvvm/include
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found out the hard way that references such as these depend upon the environment where docker build runs. To be sure the paths are as expected, they should be hard-coded.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Keith. Thanks for your advice! I will make the change.

[skip ci]

Signed-off-by: Jenny Chen <Jenny.Chen@ibm.com>
@JennyChenyj
Copy link
Author

Hi @AdamBrousseau! The changes have been made. Thanks!

@AdamBrousseau
Copy link
Contributor

jenkins build docker x86 centos6.9

@@ -171,8 +173,8 @@ RUN cd /usr/src \
&& rm -rf /usr/src/git-2.5.3

# Install ant version 1.10.5.
RUN wget https://archive.apache.org/dist/ant/binaries/apache-ant-1.10.5-bin.zip \
&& unzip apache-ant-1.10.5-bin.zip -d /opt \
COPY apache-ant-1.10.5-bin.zip .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? apache-ant-1.10.5-bin.zip isn't (and shouldn't be) in the openj9 repo.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original link for downloading ant expires. Sorry, I am not sure why we use this version. @AdamBrousseau

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original link expired. We updated it in e22aa46#diff-98f2a595424a7af76747ea2c636f6d19. The COPY was put in temporarily because @JennyChenyj's laptop wasn't resolving the url. This can be removed now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just removed. Sorry. I forgot we made this change to test the dockerfile. Thanks for explaining it! @AdamBrousseau

[skip ci]

Signed-off-by: Jenny Chen <Jenny.Chen@ibm.com>
@AdamBrousseau
Copy link
Contributor

jenkins build docker x86 centos6.9

2 similar comments
@AdamBrousseau
Copy link
Contributor

jenkins build docker x86 centos6.9

@AdamBrousseau
Copy link
Contributor

jenkins build docker x86 centos6.9

@JennyChenyj
Copy link
Author

Good Afternoon! The change has made. Please let me know if there is any other change. @pshipton

@pshipton
Copy link
Member

@AdamBrousseau are you good with these changes?

@pshipton pshipton self-assigned this Oct 15, 2019
@AdamBrousseau
Copy link
Contributor

Just waiting on a compile build to verify the container works as expected. Will post back once it is complete.

Copy link
Contributor

@AdamBrousseau AdamBrousseau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pshipton pshipton merged commit 94c6cde into eclipse-openj9:master Oct 16, 2019
@pshipton
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants