-
Notifications
You must be signed in to change notification settings - Fork 41
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
fix(*): add readiness & liveness probes #149
Changes from all commits
64cde22
21ad624
557bf6e
0dbe716
78ae20d
c375e8a
20c7317
d733f4c
8891583
9214a24
c1aebfc
f58ac45
f3755f6
96ec946
e35609a
e35e855
b3dba97
0dae532
b039013
4f1d853
b6a29a5
4808fe1
b3fbb2b
928a064
99f9d81
c9113b1
d774e5f
5081ba5
5a209b6
f8fa37a
ac6e83b
666bb24
c925148
2b8fc3b
8952b25
958e0b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ apiVersion: v1 | |
kind: ReplicationController | ||
metadata: | ||
name: deis-builder | ||
namespace: deis | ||
labels: | ||
heritage: deis | ||
spec: | ||
|
@@ -15,43 +16,50 @@ spec: | |
spec: | ||
containers: | ||
- name: deis-builder | ||
imagePullPolicy: Always | ||
image: quay.io/deisci/builder:v2-beta | ||
imagePullPolicy: Always | ||
ports: | ||
- containerPort: 2223 | ||
- containerPort: 3000 | ||
name: ssh | ||
- containerPort: 8092 | ||
name: healthsrv | ||
env: | ||
- name: BUILDER_FETCHER_PORT | ||
value: "3000" | ||
- name: BUILDER_SSH_HOST_IP | ||
value: "0.0.0.0" | ||
- name: BUILDER_SSH_HOST_PORT | ||
value: "2223" | ||
- name: "HEALTH_SERVER_PORT" | ||
value: "8092" | ||
- name: "EXTERNAL_PORT" | ||
value: "2223" | ||
- name: POD_NAMESPACE | ||
# This var needs to be passed so that the minio client (https://github.com/minio/mc) will work in Alpine linux | ||
- name: "DOCKERIMAGE" | ||
value: "1" | ||
- name: "DEBUG" | ||
value: "true" | ||
- name: "POD_NAMESPACE" | ||
valueFrom: | ||
fieldRef: | ||
fieldPath: metadata.namespace | ||
livenessProbe: | ||
httpGet: | ||
path: /healthz | ||
port: 8092 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would you mind just removing this code entirely for now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, it's removed here ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh I meant like removing the commented-out code entirely rather than leaving it as a comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, it's removed entirely. Here is the file on my branch, which is represented in the PR - notice no mention of SSL There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow. I was really derping hard there. Carry on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it happens ;) |
||
initialDelaySeconds: 2 | ||
timeoutSeconds: 1 | ||
readinessProbe: | ||
httpGet: | ||
path: /healthz | ||
port: 8092 | ||
initialDelaySeconds: 2 | ||
timeoutSeconds: 1 | ||
volumeMounts: | ||
- name: minio-user | ||
mountPath: /var/run/secrets/object/store | ||
readOnly: true | ||
- name: builder-key-auth | ||
mountPath: /var/run/secrets/api/auth | ||
readOnly: true | ||
# not currently running minio with SSL support. see https://github.com/deis/minio/pull/22 for more detail | ||
# - name: minio-ssl | ||
# mountPath: /var/run/secrets/object/ssl | ||
# readOnly: true | ||
volumes: | ||
- name: minio-user | ||
secret: | ||
secretName: minio-user | ||
- name: builder-key-auth | ||
secret: | ||
secretName: builder-key-auth | ||
# not currently running minio with SSL support. see https://github.com/deis/minio/pull/22 for more detail | ||
# - name: minio-ssl | ||
# secret: | ||
# secretName: minio-ssl |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,13 +5,12 @@ package pkg | |
|
||
import ( | ||
"fmt" | ||
"log" | ||
"os" | ||
|
||
"github.com/Masterminds/cookoo" | ||
clog "github.com/Masterminds/cookoo/log" | ||
"github.com/deis/builder/pkg/sshd" | ||
|
||
"log" | ||
"os" | ||
) | ||
|
||
// Return codes that will be sent to the shell. | ||
|
@@ -28,7 +27,7 @@ const ( | |
// Git. | ||
// | ||
// Run returns on of the Status* status code constants. | ||
func Run(sshHostIP string, sshHostPort int, cmd string) int { | ||
func RunBuilder(sshHostIP string, sshHostPort int, sshServerCircuit *sshd.Circuit) int { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a reason we're renaming this from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed For context, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the reason I had to touch this function in the first place was to add the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. No objections here, just curious :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool. Thanks for letting me slip this sorta-not-related change in ;) |
||
reg, router, ocxt := cookoo.Cookoo() | ||
log.SetFlags(0) // Time is captured elsewhere. | ||
|
||
|
@@ -59,7 +58,7 @@ func Run(sshHostIP string, sshHostPort int, cmd string) int { | |
// Start the SSH service. | ||
// TODO: We could refactor Serve to be a command, and then run this as | ||
// a route. | ||
if err := sshd.Serve(reg, router, cxt); err != nil { | ||
if err := sshd.Serve(reg, router, sshServerCircuit, cxt); err != nil { | ||
clog.Errf(cxt, "SSH server failed: %s", err) | ||
return StatusLocalError | ||
} | ||
|
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 have to check if namepsace is present or not ? builder won't start if namespace is not present or k8s cluster is not working
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.
Well, the goal here is to check that the k8s API is accessible. Given that the builder has started, we can assume that the namespace under which it runs (also specified in
POD_NAMESPACE
) exists. The call that the health server makes is to just get a list of namespaces, and the goal there is only to see if the master is accessible.I think @aledbf had an idea for a cheaper method of determining whether the API server was accessible, but if you have ideas I'd like to hear them too.
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.
@arschles if the goal is just check the status of API server why not just run:
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.
@aledbf that's much better - thanks!
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.
@aledbf here's the rendered error when that code runs:
couldn't get version/kind; json parse error: invalid character 'o' looking for beginning of value
I'd like to keep this code as-is for now, and put in an issue to use the
/healthz
way later. Objections?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.
None
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.
#180