-
Notifications
You must be signed in to change notification settings - Fork 173
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 vic-machine-server container build #6518
Add vic-machine-server container build #6518
Conversation
346755c
to
71f3b1b
Compare
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
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
Is the |
@zjs the Dockerfile is for real |
cmd/vic-machine-server/Dockerfile
Outdated
# gcloud auth login | ||
# gcloud docker -- push gcr.io/eminent-nation-87317/vic-machine-server:1.x | ||
|
||
FROM golang:1.8 |
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.
Do we actually need to start from golang:1.8
? Since vic-machine-server
is already compiled, can we use a lighter-weight base image? (Should we be using photon
, or even scratch
?)
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.
Changed to photon - I get docker: Error response from daemon: oci runtime error: container_linux.go:247: starting container process caused "exec: \"/bin/sh\": stat /bin/sh: no such file or directory".
when using scratch
cmd/vic-machine-server/Dockerfile
Outdated
|
||
FROM golang:1.8 | ||
|
||
COPY bin/vic-machine-server /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.
Should we somehow capture the version of vic-machine-sever
in the container image? I'm not sure what the best practice for this would be.
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.
I'll leave this to a later update since we are only tagging latest
and @mdharamadas1 will be figuring out how we do versioning with the improved CI
cmd/vic-machine-server/Dockerfile
Outdated
|
||
COPY bin/vic-machine-server /vmware/ | ||
|
||
ENTRYPOINT /vmware/vic-machine-server |
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.
It seems like it would be helpful to include some ENV instructions to define defaults and serve as documentation. Something like:
ENV HOST 0.0.0.0
ENV PORT 80
ENV TLS_PORT 443
And then it seems like we should use EXPOSE instructions so that the container can just be used with -P
:
EXPOSE 80
EXPOSE 443
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.
I agree with the ENVs but on the appliance, vic-machine-server can't listen on 80/443 since there are already services running there. Do you still want me to put the EXPOSEs?
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.
Added EXPOSE since it doesn't hurt anything and can be useful for testing
cmd/vic-machine-server/Dockerfile
Outdated
|
||
COPY bin/vic-machine-server /vmware/ | ||
|
||
ENTRYPOINT /vmware/vic-machine-server |
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 we be documenting where/how we expect certificates to be supplied?
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.
Is this acceptable?
docker run -it -p 9443:443 -p 8080:80 -v ~/certs:/data -e TLS_CERTIFICATE=/data/server.cert.pem -e TLS_PRIVATE_KEY=/data/server.key.pem vic-machine-server
2017/10/06 13:47:02 Serving vic machine at http://[::]:80
2017/10/06 13:47:02 Serving vic machine at https://[::]:443
chin@CORPORATE:~|⇒ curl http://10.17.109.141:8080/container/version
v1.2.0-rc1-0-68b5a25e%
chin@CORPORATE:~|⇒ curl -k https://10.17.109.141:9443/container/version
v1.2.0-rc1-0-68b5a25e%
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.
Would it be possible to mount a volume and set a directory env var or have a default certs directory where vic-machine-server looks for key/cert/ca files with standard names?
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.
I think it'd make sense to have a default certs directory (and file names), and express that as default values for the TLS_CERTIFICATE
and TLS_PRIVATE
ENV values.
Then the person running the container can either expose the files in the expected place (e.g., just specify -v ~/certs:/data
) or put it somewhere else and override the ENV (e.g., specify -v ~/certs:/other/data -e TLS_CERTIFICATE=/other/data/server.cert.pem -e TLS_PRIVATE_KEY=/other/data/server.key.pem
)
I think this gives us reasonable default behavior without constraining use of the container.
cmd/vic-machine-server/Dockerfile
Outdated
|
||
FROM golang:1.8 | ||
|
||
COPY bin/vic-machine-server /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.
Should we be trying to follow Linux Filesystem Hierarchy, which might suggest putting this in /usr/local/bin
, instead of putting this in a custom directory?
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.
fixed
fb2d077
to
b065b4c
Compare
b065b4c
to
4434d96
Compare
Towards #6037
Adds container build and upload to Drone