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

Add readiness healthcheck generation by label #1366

Conversation

jvitor83
Copy link
Contributor

@jvitor83 jvitor83 commented Mar 6, 2021

Related: #1262

Allow the readiness healthcheck to be configured independent from the docker-compose healthcheck (liveness).

The following docker-compose.yml

version: "3.4"

services:

  some-service:
    image: busybox:latest
    labels:
      kompose.service.healthcheck.readiness.test: CMD curl -f "http://localhost/health one-parameter"
      kompose.service.healthcheck.readiness.interval: 10s
      kompose.service.healthcheck.readiness.timeout: 10s
      kompose.service.healthcheck.readiness.retries: 3
      kompose.service.healthcheck.readiness.start_period: 120s
    healthcheck:
      test: "curl -f http://localhost/health"
      interval: 5s
      timeout: 5s
      retries: 5
      start_period: 120s
    ports:
      - "80:80"
      - "443:443"

generates the deployment.yml:

apiVersion: apps/v1
kind: Deployment
metadata:
  annotations:
    kompose.cmd: kompose.exe convert -f docker-compose.yml
    kompose.service.healthcheck.readiness.interval: 10s
    kompose.service.healthcheck.readiness.retries: "3"
    kompose.service.healthcheck.readiness.start_period: 120s
    kompose.service.healthcheck.readiness.test: CMD curl -f "http://localhost/health one-parameter"
    kompose.service.healthcheck.readiness.timeout: 10s
    kompose.version: 1.22.0 (HEAD)
  creationTimestamp: null
  labels:
    io.kompose.service: some-service
  name: some-service
spec:
  replicas: 1
  selector:
    matchLabels:
      io.kompose.service: some-service
  strategy: {}
  template:
    metadata:
      annotations:
        kompose.cmd: kompose.exe convert -f docker-compose.yml
        kompose.service.healthcheck.readiness.interval: 10s
        kompose.service.healthcheck.readiness.retries: "3"
        kompose.service.healthcheck.readiness.start_period: 120s
        kompose.service.healthcheck.readiness.test: CMD curl -f "http://localhost/health one-parameter"
        kompose.service.healthcheck.readiness.timeout: 10s
        kompose.version: 1.22.0 (HEAD)
      creationTimestamp: null
      labels:
        io.kompose.service: some-service
    spec:
      containers:
        - image: busybox:latest
          livenessProbe:
            exec:
              command:
                - curl
                - -f
                - http://localhost/health
            failureThreshold: 5
            initialDelaySeconds: 120
            periodSeconds: 5
            timeoutSeconds: 5
          name: some-service
          ports:
            - containerPort: 80
            - containerPort: 443
          readinessProbe:
            exec:
              command:
                - curl
                - -f
                - http://localhost/health one-parameter
            failureThreshold: 3
            initialDelaySeconds: 120
            periodSeconds: 10
            timeoutSeconds: 10
          resources: {}
      restartPolicy: Always
status: {}

This way, if wanted, both readiness and liveness can be configured.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 6, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jvitor83
To complete the pull request process, please assign janetkuo after the PR has been reviewed.
You can assign the PR to them by writing /assign @janetkuo in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jvitor83
Copy link
Contributor Author

jvitor83 commented Mar 7, 2021

I think this PR is ready.
Just wondering if is the right choice, like:

  1. If we should use the same HealhCheck of compose in both Liveness and Readiness (by having the same bahaviour of swarm/compose that use only one config to both behaviors) - Not this PR

    Drawback: both commands be executed and don't having the possibility of diverge

  2. If we should allow a independent config for each behavior (readiness and liveness) and deviate a bit from compose/swarm (that use only one config to both behaviors) - This PR

    Drawback: People should know that in order to achieve the same behavior it need to inform this readiness label in addition to the healtchcheck

  3. It could be both (1 and 2). If only have the healtchcheck uses it in both liveness and readiness, but if have both informed (readiness label and healthcheck) then use the healtchcheck at liveness and label at readiness - Can change this PR to that

    Drawback: Same of 1 if only used healthcheck (without readiness label)

I think the option 2 is good enouth (this PR), but i agree and can change to 3 also if wanted. If the mainteiners think the same, i believe this can be merged.

@hangyan hangyan self-assigned this Mar 11, 2021
@hangyan
Copy link
Contributor

hangyan commented Mar 17, 2021

(Sorry for the very late response, it won't happen again). I'm not total against or support this kind of feature. Since i think this project is mainly a single direction map, from compose -> kubernetes. So the main focus is on what compose provides and how they should map to kubernetes. Liveness and Readiness is a mainly a kubernetes feature, so we don't spend time many on this .But i think this PR is good enough to be merged, this can helps a lot of people. There is no perfect soultion on this kind of mismatch. I will review this code now.

@hangyan
Copy link
Contributor

hangyan commented Mar 17, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2021
@hangyan hangyan merged commit 0036f0c into kubernetes:master Mar 17, 2021
@hangyan
Copy link
Contributor

hangyan commented Mar 17, 2021

@jvitor83 Thanks very much for this contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants