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.

Sign up for GitHub

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

Jump to bottom

External Metrics support for VPA recommender #5576

Merged

k8s-ci-robot merged 1 commit into kubernetes:master from DataDog:lally/ext-upstream

Sep 4, 2023

Contributor

lallydd commented Mar 7, 2023 •

edited

Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

This adds a mode to the VPA recommender to use External Metrics. It also adds (much more substantially in lines-of-changes) local KIND (Kubernetes IN Docker) support for the recommender in both metrics-server and External Metrics modes.

Extending out the testing to support the rest of the VPA tests is certainly possible, and probably 95% done, but this PR was taking a long time already to get sorted.

Which issue(s) this PR fixes:

Fixes #5153

Special notes for your reviewer:

Does this PR introduce a user-facing change?

ALPHA.  Added External Metrics server support.  To enable this mode, and disable the VPA recommender's use 
of `metrics-server` with  `--use-external-metrics=true` and identify the metrics to use for CPU and memory 
recommendations with `--external-metrics-cpu-metric=YOURMETRIC` and 
`--external-metrics-memory-metric=YOURMETRIC`, respectively.  You must use all three flags together to 
configure the VPA in this way.  This is new functionality and is not as mature or as well-tested as the 
existing `metrics-server` functionality.

Will require additional permissions to use:
- apiGroups:
   - "external.metrics.k8s.io"
  resources:
   - "*"
  verbs:
   - get
   - list

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot added the kind/feature label

Contributor

k8s-ci-robot commented Mar 7, 2023

Welcome @lallydd!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot added size/XXL area/vertical-pod-autoscaler labels

k8s-ci-robot requested review from jbartosik and krzysied

March 7, 2023 17:52

k8s-triage-robot commented Mar 7, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

17 similar comments

k8s-triage-robot commented Mar 7, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 8, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 9, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 9, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 9, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 9, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

k8s-triage-robot commented Mar 9, 2023

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

Contributor

voelzmo commented Mar 9, 2023

Seems like there's some issues with easyCLA: communitybridge/easycla#3843
I contacted sig-contribex about this: https://kubernetes.slack.com/archives/C1TU9EB9S/p1678351853346579

MadhavJivrajani commented Mar 9, 2023

/close
Going to try closing and re-opening the PR to try to get CLA to refresh

k8s-ci-robot closed this

Contributor

k8s-ci-robot commented Mar 9, 2023

@MadhavJivrajani: Closed this PR.

In response to this:

/close
Going to try closing and re-opening the PR to try to get CLA to refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

MadhavJivrajani commented Mar 9, 2023

/reopen
fingers crossed

k8s-ci-robot reopened this

lallydd force-pushed the lally/ext-upstream branch from 6aeaef8 to 379b950 Compare

July 5, 2023 18:43

lallydd requested a review from jbartosik

July 5, 2023 19:29

Contributor Author

lallydd commented Jul 5, 2023

Tests pass after another rebase.

voelzmo suggested changes

View reviewed changes

Contributor

voelzmo left a comment

In the current state, the local e2e tests fail with this in the recommender logs

Cannot update VPA vertical-pod-autoscaling-3092/hamster-vpa object. Reason: verticalpodautoscalers.autoscaling.k8s.io "hamster-vpa" is forbidden: User "system:serviceaccount:kube-system:vpa-recommender" cannot patch resource "verticalpodautoscalers/status" in API group "autoscaling.k8s.io" in the namespace "vertical-pod-autoscaling-3092"

Now that #5911 has been merged, the permissions in hack/e2e/vpa-rbac.yaml need to be adapted as well.

This is a bit brittle, people will have to remember to adapt permissions in two places, because it is duplicated. Not sure if there's a good alternative, though.

On a more general note: I really like adding the ability to run the e2e tests locally in a kind cluster! If we get all tests to run in this scenario, this would be very helpful to everyone developing the VPA! I just really wish you would have PR'ed this separately. Right now this is a big chunk with different concerns and hard to review/approve (at least for me, it is).

Contributor Author

lallydd commented Jul 6, 2023

That's a really good point. I'm testing a diff/patch based approach now, where deploy-for-e2e-locally.sh generates hack/e2e/vpa-rbac.yaml from deploy/vpa-rbac.yaml with a context diff.

lallydd force-pushed the lally/ext-upstream branch from 379b950 to 68c9db8 Compare

July 6, 2023 18:15

Contributor Author

lallydd commented Jul 6, 2023

@voelzmo It worked! Now this is just the diff necessary: 68c9db8#diff-debb7e710c67f3afe14c9605ad0d7ab3b59f6272a15e21d55679c27b139d732d

And that's all just to enable external metrics, so it's a useful artifact to maintain generally.

Contributor Author

lallydd commented Jul 6, 2023

Also added permissions note to the release-notes PR header. Please LMK if that's not the right place for it.

jbartosik requested changes

View reviewed changes

Collaborator

jbartosik left a comment

I took a look at parts of the PR other than the core code change. Mostly they look good but I'm asking for some changes (should be small) or examplations.

Thank you!

vertical-pod-autoscaler/hack/deploy-for-e2e-locally.sh Outdated

    
              kubectl apply -f ${SCRIPT_ROOT}/hack/e2e/k8s-metrics-server.yaml

              for i in ${COMPONENTS}; do

                if [ $i == admission-controller ] ; then

Collaborator

jbartosik Aug 14, 2023

If I understand correctly this should never happen. Check in line 43 ensures that COMPONENTS is recommender or recommender-externalmetrics.

Contributor Author

lallydd Aug 14, 2023

Removed.

vertical-pod-autoscaler/hack/deploy-for-e2e-locally.sh Outdated

    
                echo "<suite> should be one of:"

                echo " - recommender"

                echo " - recommender-externalmetrics"

                echo "If component is not specified all above will be started."

Collaborator

jbartosik Aug 14, 2023

This doesn't seem to be true. If there is no component specified script prints help and exits

Contributor Author

lallydd Aug 14, 2023

Removed.

vertical-pod-autoscaler/hack/deploy-for-e2e-locally.sh Outdated

Comment on lines 54 to 55

    
              export REGISTRY=localhost:5001

              export TAG=latest

Collaborator

jbartosik Aug 14, 2023

I think it would be nice if the script allowed setting REGISTRY and TAG

Contributor Author

lallydd Aug 14, 2023

Fixed. Now if they're in the environment, they'll get used.

vertical-pod-autoscaler/hack/deploy-for-e2e-locally.sh Outdated

Comment on lines 64 to 69

    
              for i in ${COMPONENTS}; do

                if [ $i == admission-controller ] ; then

                  (cd ${SCRIPT_ROOT}/pkg/${i} && bash ./gencerts.sh || true)

                elif [ $i == recommender-externalmetrics ] ; then

                  i=recommender

                fi

                ALL_ARCHITECTURES=amd64 make --directory ${SCRIPT_ROOT}/pkg/${i} release REGISTRY=${REGISTRY} TAG=${TAG}

                kind load docker-image ${REGISTRY}/vpa-${i}-amd64:${TAG}

              done

Collaborator

jbartosik Aug 14, 2023

Suggested change

      
            for i in ${COMPONENTS}; do
          
              if [ $i == admission-controller ] ; then
          
                (cd ${SCRIPT_ROOT}/pkg/${i} && bash ./gencerts.sh || true)
          
              elif [ $i == recommender-externalmetrics ] ; then
          
                i=recommender
          
              fi
          
              ALL_ARCHITECTURES=amd64 make --directory ${SCRIPT_ROOT}/pkg/${i} release REGISTRY=${REGISTRY} TAG=${TAG}
          
              kind load docker-image ${REGISTRY}/vpa-${i}-amd64:${TAG}
          
            done
          
              kind load docker-image ${REGISTRY}/vpa-recommender-amd64:${TAG}

Contributor Author

lallydd Aug 14, 2023

Are you sure you want to remove the make too?

Collaborator

jbartosik Aug 21, 2023

No, it should stay

vertical-pod-autoscaler/hack/e2e/k8s-metrics-server.yaml Outdated

Collaborator

jbartosik Aug 14, 2023

I think I've asked about this but can't find the comment right now: is this a copy of a file from another repo?

Contributor Author

lallydd Aug 14, 2023

It's a processed version of all the templates from here: https://github.com/kubernetes-sigs/metrics-server/tree/master/charts/metrics-server/templates

vertical-pod-autoscaler/hack/e2e/vpa-rbac.diff Outdated

Collaborator

jbartosik Aug 14, 2023

Please explain what this diff is for

Contributor Author

lallydd Aug 14, 2023 •

edited

Loading

I'll put together a small README. In short, it's what else you need to turn on external metrics which we use for local end-to-end testing. It's automatically applied when running locally. This prevents us needing to put the permission in the normally deployed RBAC yaml -- people can put it in when they want to use this feature.

EDIT: added to existing hack/local-cluster.md doc.

Contributor Author

lallydd Aug 14, 2023

I've added a section "RBAC Changes" under hack/local-cluster.md to explain it.

vertical-pod-autoscaler/hack/emit-metrics.py

    
              # See the License for the specific language governing permissions and

              # limitations under the License.

              import argparse

Collaborator

jbartosik Aug 14, 2023

Can we use https://pkg.go.dev/k8s.io/kubernetes/test/images/resource-consumer#section-readme instead?

vertical-pod-autoscaler/hack/run-e2e-locally.sh

Comment on lines +59 to +63

    
                  if [ ${SUITE} == recommender-externalmetrics ]; then

                     ${SCRIPT_ROOT}/hack/run-e2e-tests.sh recommender

                  else

                    ${SCRIPT_ROOT}/hack/run-e2e-tests.sh ${SUITE}

                  fi

Collaborator

jbartosik Aug 14, 2023

Suggested change

      
                if [ ${SUITE} == recommender-externalmetrics ]; then
          
                   ${SCRIPT_ROOT}/hack/run-e2e-tests.sh recommender
          
                else
          
                  ${SCRIPT_ROOT}/hack/run-e2e-tests.sh ${SUITE}
          
                fi
          
                ${SCRIPT_ROOT}/hack/run-e2e-tests.sh recommender

Contributor Author

lallydd Aug 14, 2023

This would make it harder to add make other components testable locally. Are you sure you want this?

vertical-pod-autoscaler/hack/vpa-process-yamls.sh Show resolved Hide resolved

lallydd force-pushed the lally/ext-upstream branch from 68c9db8 to de708d2 Compare

August 14, 2023 21:11

lallydd commented

View reviewed changes

Contributor Author

lallydd left a comment

I believe I got everything, except for replacing the script as we discusssed in this morning's zoom call.

vertical-pod-autoscaler/hack/deploy-for-e2e-locally.sh Outdated

    
                echo "<suite> should be one of:"

                echo " - recommender"

                echo " - recommender-externalmetrics"

                echo "If component is not specified all above will be started."

Contributor Author

lallydd Aug 14, 2023

Removed.

vertical-pod-autoscaler/hack/deploy-for-e2e-locally.sh Outdated

Comment on lines 54 to 55

    
              export REGISTRY=localhost:5001

              export TAG=latest

Contributor Author

lallydd Aug 14, 2023

Fixed. Now if they're in the environment, they'll get used.

vertical-pod-autoscaler/hack/deploy-for-e2e-locally.sh Outdated

    
              kubectl apply -f ${SCRIPT_ROOT}/hack/e2e/k8s-metrics-server.yaml

              for i in ${COMPONENTS}; do

                if [ $i == admission-controller ] ; then

Contributor Author

lallydd Aug 14, 2023

Removed.

vertical-pod-autoscaler/hack/deploy-for-e2e-locally.sh Outdated

Comment on lines 64 to 69

    
              for i in ${COMPONENTS}; do

                if [ $i == admission-controller ] ; then

                  (cd ${SCRIPT_ROOT}/pkg/${i} && bash ./gencerts.sh || true)

                elif [ $i == recommender-externalmetrics ] ; then

                  i=recommender

                fi

                ALL_ARCHITECTURES=amd64 make --directory ${SCRIPT_ROOT}/pkg/${i} release REGISTRY=${REGISTRY} TAG=${TAG}

                kind load docker-image ${REGISTRY}/vpa-${i}-amd64:${TAG}

              done

Contributor Author

lallydd Aug 14, 2023

Are you sure you want to remove the make too?

vertical-pod-autoscaler/hack/e2e/k8s-metrics-server.yaml Outdated

Contributor Author

lallydd Aug 14, 2023

It's a processed version of all the templates from here: https://github.com/kubernetes-sigs/metrics-server/tree/master/charts/metrics-server/templates

vertical-pod-autoscaler/hack/run-e2e-locally.sh

Comment on lines +59 to +63

    
                  if [ ${SUITE} == recommender-externalmetrics ]; then

                     ${SCRIPT_ROOT}/hack/run-e2e-tests.sh recommender

                  else

                    ${SCRIPT_ROOT}/hack/run-e2e-tests.sh ${SUITE}

                  fi

Contributor Author

lallydd Aug 14, 2023

This would make it harder to add make other components testable locally. Are you sure you want this?

vertical-pod-autoscaler/hack/e2e/vpa-rbac.diff Outdated

Contributor Author

lallydd Aug 14, 2023 •

edited

Loading

I'll put together a small README. In short, it's what else you need to turn on external metrics which we use for local end-to-end testing. It's automatically applied when running locally. This prevents us needing to put the permission in the normally deployed RBAC yaml -- people can put it in when they want to use this feature.

EDIT: added to existing hack/local-cluster.md doc.

vertical-pod-autoscaler/hack/emit-metrics.py

    
              # See the License for the specific language governing permissions and

              # limitations under the License.

              import argparse

Contributor Author

lallydd Aug 14, 2023

It wouldn't be a drop-in replacement. This script generates reasonable resource-usage sizes for every pod, making the existing recommender tests work by giving them resource-use values. As discussed in-meeting, we can work to remove it (by adjusting tests or other infra) in a follow-up PR.

vertical-pod-autoscaler/hack/e2e/vpa-rbac.diff Outdated

Contributor Author

lallydd Aug 14, 2023

I've added a section "RBAC Changes" under hack/local-cluster.md to explain it.

vertical-pod-autoscaler/hack/vpa-process-yamls.sh Show resolved Hide resolved

jbartosik reviewed

View reviewed changes

Collaborator

jbartosik left a comment

Mostly looks good to me. A few comments to clean this up.

vertical-pod-autoscaler/e2e/go.sum Outdated

Collaborator

jbartosik Aug 21, 2023

Please remove changes in vertical-pod-autoscaler/e2e/go.sum and vertical-pod-autoscaler/e2e/vendor/ from the PR

Contributor Author

lallydd Aug 21, 2023

Done and done.

vertical-pod-autoscaler/hack/emit-metrics.py

    
              # See the License for the specific language governing permissions and

              # limitations under the License.

              import argparse

Collaborator

jbartosik Aug 21, 2023

Please open a follow up issue to make this more maintainable

vertical-pod-autoscaler/vendor/k8s.io/metrics/pkg/apis/external_metrics/doc.go Outdated

Collaborator

jbartosik Aug 21, 2023

There aren't any changes to go.mod / go.sum causing this?

Contributor Author

lallydd Aug 21, 2023 •

edited

Loading

The external_metrics api was referenced in vendor/modules.txt. I think it's just pulled in as part of the existing k8s.io/metrics reference in go.mod

Contributor Author

lallydd Aug 21, 2023

Ref: a5148d2#diff-34952f46f28ac4a379ca6d8979d2d2acff269f9b50db916304f99a5ae556e5d1R653-R656

lallydd mentioned this pull request

Fix maintainability issues in vertical-pod-autoscaler/hack/emit-metrics.py #6053

Closed

lallydd force-pushed the lally/ext-upstream branch from de708d2 to a5148d2 Compare

August 21, 2023 15:38

jbartosik reviewed

View reviewed changes

Collaborator

jbartosik left a comment

/lgtm

As discussed at the last SIG meeting we should clearly document flags we add as alpha and communicate clearly that this isn't as mature as metrics server support yet.

https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-a-flag-or-cli

k8s-ci-robot assigned jbartosik

k8s-ci-robot added the lgtm label

Contributor Author

lallydd commented Aug 22, 2023

I just watched the SIG meeting. Should the flags be documented as alpha under the flag declaration the VPA Recommender README, or somewhere else?


          External Metrics w/E2E testing, prometheus, & KIND.

034632a

lallydd force-pushed the lally/ext-upstream branch from a5148d2 to 034632a Compare

August 28, 2023 14:18

k8s-ci-robot removed the lgtm label

Contributor Author

lallydd commented Aug 28, 2023

@jbartosik I added ALPHA designations to the command-line flags and added an ALPHA designation to the release notes. Also this sentence: "This is new functionality and is not as mature or as well-tested as the
existing metrics-server functionality."

jbartosik approved these changes

View reviewed changes

Collaborator

jbartosik left a comment

/lgtm
/approve

k8s-ci-robot added the lgtm label

Contributor

k8s-ci-robot commented Sep 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbartosik, lallydd

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~vertical-pod-autoscaler/OWNERS~~ [jbartosik]

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

k8s-ci-robot added the approved label

k8s-ci-robot merged commit e31031c into kubernetes:master

zroubalik mentioned this pull request

Provide support for Vertical Pod Autoscaler (VPA) kedacore/keda#1788

Open

raywainman mentioned this pull request

Fix vpa-up.sh script by removing duplicate recommender #6151

Merged

raywainman mentioned this pull request

VPA release 1.0.0 #6113

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

sftim sftim left review comments

kwiesmueller kwiesmueller left review comments

voelzmo voelzmo requested changes

jbartosik jbartosik approved these changes

krzysied Awaiting requested review from krzysied

Labels

approved area/vertical-pod-autoscaler cncf-cla: yes kind/feature lgtm size/XXL ¯\_(ツ)_/¯