VPA admission controller should stop review Pod Update event

[phsiao]

I might be wrong, but it is my understanding that pod Update review has no impact through VPA admission controller. It would be doing work that will have no effect at the end.

There can be a lot of pod updates events, for example, updating pod annotations, and would be creating a lot of unnecessary work. I'd like to propose we remove the review of pod Update.

[phsiao]

/cc @bskiba

[bskiba]

It seems reasonable (and desired) since there should not be changes to resources in the updates (at least in the current k8s version). However, I'd like to see this tested, to make sure the changes introduced by VPA admission controller do not get overriden, for example if there are resource request specified in the deployment template. Could you perform such tests and describe outcome here?

[phsiao]

Sure, happy to verify that. Here is my understanding of what should happen, but I will verify that.

1. a workload object is updated with different resource request
2. the workload controller should start replacing the pods by creating new pods and terminating old pods
3. the vpa admission controller should still see all the Create event for the new pods, and patch the resource request accordingly.

[bskiba]

That's my understanding too. Thanks for taking the time to make sure!

[phsiao]

Here is what I did and attempt to verify:

1. Deploy this hamster vpa and deployment,

apiVersion: "poc.autoscaling.k8s.io/v1alpha1"
kind: VerticalPodAutoscaler
metadata:
  name: hamster-vpa
  namespace: sandbox
spec:
  selector:
    matchLabels:
      app: hamster
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  name: hamster
  namespace: sandbox
spec:
  replicas: 2
  template:
    metadata:
      labels:
        app: hamster
    spec:            
      containers:
      - name: hamster
        image: k8s.gcr.io/ubuntu-slim:0.1
        resources:
          requests:
            cpu: 100m
            memory: 50Mi
        command: ["/bin/sh"]
        args: ["-c", "while true; do timeout 0.5s yes >/dev/null; sleep 0.5s; done"]

2. let it run for a while and get a snapshot of vpa

apiVersion: poc.autoscaling.k8s.io/v1alpha1
kind: VerticalPodAutoscaler
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"poc.autoscaling.k8s.io/v1alpha1","kind":"VerticalPodAutoscaler","metadata":{"annotations":{},"name":"hamster-vpa","namespace":"sandbox"},"spec":{"selector":{"matchLabels":{"app":"hamster"}}}}
  clusterName: ""
  creationTimestamp: 2018-08-20T16:28:08Z
  generation: 0
  name: hamster-vpa
  namespace: sandbox
  resourceVersion: "770208756"
  selfLink: /apis/poc.autoscaling.k8s.io/v1alpha1/namespaces/sandbox/verticalpodautoscalers/hamster-vpa
  uid: 05bd24dd-a496-11e8-ae2b-90b11c5787b7
spec:
  selector:
    matchLabels:
      app: hamster
  updatePolicy:
    updateMode: Auto
status:
  conditions:
  - lastTransitionTime: 2018-08-20T16:31:47Z
    status: "True"
    type: RecommendationProvided
  recommendation:
    containerRecommendations:
    - containerName: hamster
      lowerBound:
        cpu: 555m
        memory: "104857600"
      target:
        cpu: 564m
        memory: "104857600"
      upperBound:
        cpu: 4884m
        memory: "151867269"

3. delete one of the hamster pod and let it re-create with the vpa admission controller, verify the new pod is created with the recommended request

    resources:
      requests:
        cpu: 564m
        memory: "104857600"

4. increase the request in the yaml in Fix imports in cluster autoscaler after migrating it from contrib #1 to be 1000m cpu and 500Mi memory, apply to the cluster, observe the two new pods have

    resources:
      requests:
        cpu: 564m
        memory: "104857600"

and

    resources:
      requests:
        cpu: 564m
        memory: "104857600"

(identical for both new pods)

---

Note that this is running with patched admission controller that only observes CREATE event for pod.

  rules:
  - apiGroups:
    - ""
    apiVersions:
    - v1
    operations:
    - CREATE
    resources:
    - pods
  - apiGroups:
    - poc.autoscaling.k8s.io
    apiVersions:
    - v1alpha1
    operations:
    - CREATE
    - UPDATE
    resources:
    - verticalpodautoscalers

[phsiao]

@bskiba please see above for my attempt to verify that. I don't know what we can do to put Update back when patching resource request in-place is supported, but I think that is going to be a pretty obvious bug when that happens.

[bskiba]

Thanks! Looks good. We'll be watching pretty closely for the in-place updates of resource requests as we are very keen to use them for our own advantage :)
/lgtm