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

Downward API implementation for resources limits and requests #24179

Conversation

aveshagarwal
Copy link
Member

@aveshagarwal aveshagarwal commented Apr 13, 2016

This is an implementation of Downward API for resources limits and requests, and it works with environment variables and volume plugin.

This is based on proposal #24051. This implementation follows API with magic keys approach as discussed in the proposal.

@kubernetes/rh-cluster-infra


This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 13, 2016
@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests-implementation branch from 587f290 to 6fafbea Compare April 14, 2016 02:07
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2016
@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests-implementation branch from 6fafbea to 960e33b Compare April 14, 2016 15:40
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 14, 2016
@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests-implementation branch from 960e33b to 16dc24c Compare April 18, 2016 17:55
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 18, 2016
@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests-implementation branch from 16dc24c to 85bfb54 Compare April 20, 2016 02:40
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2016
@lavalamp lavalamp assigned thockin and unassigned lavalamp May 9, 2016
@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests-implementation branch from 85bfb54 to 54893f1 Compare May 23, 2016 06:25
@aveshagarwal
Copy link
Member Author

Updated PR. Will add more tests soon.
@bgrant0607 @derekwaynecarr @pmorie PTAL.

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 23, 2016
@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests-implementation branch from 54893f1 to 260841c Compare May 23, 2016 18:20
@aveshagarwal
Copy link
Member Author

Added some validation tests and fixed some issues.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2016
@aveshagarwal
Copy link
Member Author

I have tested it with following in case any one is interested:

Env variables:

apiVersion: v1
kind: Pod
metadata:
  name: dapi-test-pod
spec:
  containers:
    - name: test-container
      image: gcr.io/google_containers/busybox
      command: [ "/bin/sh", "-c", "env" ]
      resources:
        requests:
          memory: "64Mi"
          cpu: "250m"
        limits:
          memory: "128Mi"
          cpu: "1500m"
      env:
        - name: CPU_LIMIT
          valueFrom:
            resourceFieldRef:
              resource: limits.cpu
    - name: test-container2
      image: gcr.io/google_containers/busybox
      command: [ "/bin/sh", "-c", "env" ]
      resources:
        requests:
          memory: "32Mi"
          cpu: "125m"
        limits:
          memory: "64Mi"
          cpu: "250m"
      env:
        - name: MEMORY_LIMIT
          valueFrom:
            resourceFieldRef:
              resource: limits.memory
        - name: CPU_LIMIT
          valueFrom:
            resourceFieldRef:
              resource: requests.cpu
  restartPolicy: Never

Volume plugin:

apiVersion: v1
kind: Pod
metadata:
  name: kubernetes-downwardapi-volume-example
  labels:
    zone: us-est-coast
    cluster: test-cluster1
    rack: rack-22
  annotations:
    build: two
    builder: john-doe
spec:
  containers:
    - name: client-container
      image: gcr.io/google_containers/busybox
      command: ["sh", "-c", "while true; do if [[ -e /etc/labels ]]; then cat /etc/labels; fi; if [[ -e /etc/annotations ]]; then cat /etc/annotations; fi; sleep 5; done"]
      resources:
        requests:
          memory: "32Mi"
          cpu: "125m"
        limits:
          memory: "64Mi"
          cpu: "250m"
      volumeMounts:
        - name: podinfo
          mountPath: /etc
          readOnly: false
  volumes:
    - name: podinfo
      downwardAPI:
        items:
          - path: "labels"
            fieldRef:
              fieldPath: metadata.labels
          - path: "annotations"
            fieldRef:
              fieldPath: metadata.annotations
          - path: "client-container/pod_name"
            fieldRef:
              fieldPath: metadata.name
          - path: "cpu_limit"
            resourceFieldRef:
              containerName: client-container
              resource: limits.cpu
              divisor: "1"
          - path: "cpu_request"
            resourceFieldRef:
              containerName: client-container
              resource: requests.cpu
              divisor: "1m"
          - path: "memory_limit"
            resourceFieldRef:
              containerName: client-container
              resource: limits.memory
              divisor: "1M"
          - path: "memory_request"
            resourceFieldRef:
              containerName: client-container
              resource: requests.memory

@aveshagarwal
Copy link
Member Author

@bgrant0607 @derekwaynecarr @pmorie I have tested it with the above configs and also some other variations including error conditions.

@pmorie pmorie added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 23, 2016
ContainerName string `json:"containerName,omitempty"`
// Required: resource to select
Resource string `json:"resource"`
// Specifies the output format of the exposed resources
Copy link
Member

Choose a reason for hiding this comment

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

Add info about the default here.

@pmorie
Copy link
Member

pmorie commented May 23, 2016

@aveshagarwal this is moving in the right direction. Please add e2es for envs and volumes.

@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests-implementation branch from 260841c to 55caeb0 Compare May 23, 2016 22:08
@aveshagarwal
Copy link
Member Author

@bgrant0607 @derekwaynecarr @pmorie I have updated this PR.

  1. Added e2e tests for env vars, and tested locally and they work.
  2. Improved validation for memory divisor.
  3. Addressed pmorie's comments.

Still working on e2e for volume.

@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests-implementation branch from 55caeb0 to d31acbd Compare May 23, 2016 23:19
@aveshagarwal
Copy link
Member Author

Added e2e tests for volumes too now and tested locally to make sure they work.

@bgrant0607 @pmorie @derekwaynecarr PTAL.

@aveshagarwal aveshagarwal changed the title [WIP] Downward API implementation for resources limits and requests Downward API implementation for resources limits and requests May 23, 2016
if downwardAPIVolumeFile.FieldRef != nil {
allErrs = append(allErrs, validateObjectFieldSelector(downwardAPIVolumeFile.FieldRef, &validDownwardAPIFieldPathExpressions, fldPath.Child("fieldRef"))...)
if downwardAPIVolumeFile.ResourceFieldRef != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("path"), "resource", "fieldRef and resourceFieldRef can not be specified simultaneously"))
Copy link
Member

Choose a reason for hiding this comment

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

s/.Child("path")//

@aveshagarwal

@pmorie
Copy link
Member

pmorie commented May 24, 2016

@aveshagarwal I'll take the nits in a follow-up. LGTM. Thanks a lot.

@pmorie pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2016
@aveshagarwal
Copy link
Member Author

I see a failure in unit tests: FAIL k8s.io/kubernetes/pkg/api , it seems to be failing in pkg/api/serialization_test.go.

@aveshagarwal
Copy link
Member Author

I regenerated auto-generated files and when I test ./hack/test-go.sh pkg/api now it fails in pkg/api/copy_test.go. Not sure what is happening.

@aveshagarwal
Copy link
Member Author

I have found the solution and will just run the tests locally and update it and hopefully it should be good to go.

@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests-implementation branch from d31acbd to 1931931 Compare May 24, 2016 16:24
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2016
@pmorie
Copy link
Member

pmorie commented May 24, 2016

I will re-tag once green

@aveshagarwal
Copy link
Member Author

This e2e failure does not seem related to this PR:

[Fail] [k8s.io] Pods [It] should invoke init containers on a RestartNever pod 
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/pods.go:710

@aveshagarwal
Copy link
Member Author

Seems similar to this: #26131

@pmorie
Copy link
Member

pmorie commented May 24, 2016

@k8s-bot test this issue #26131

@aveshagarwal
Copy link
Member Author

@pmorie all green, LGTM again?

@aveshagarwal
Copy link
Member Author

@bgrant0607 @derekwaynecarr can you tag it LGTM again?

@pmorie pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2016
@ncdc ncdc added this to the v1.3 milestone May 24, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 25, 2016

GCE e2e build/test passed for commit 1931931.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 83c78fd into kubernetes:master May 25, 2016
@aveshagarwal aveshagarwal deleted the master-downward-api-resources-limits-requests-implementation branch May 25, 2016 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants