kubernetes-sigs / prometheus-adapter

An implementation of the custom.metrics.k8s.io API using Prometheus
Apache License 2.0
1.9k stars 551 forks source link

*: bump go to 1.20 and k8s deps to 0.27.2 #586

Closed dgrisonnet closed 1 year ago

dgrisonnet commented 1 year ago

This PR bumps the various dependency and add support for OpenAPI v3.

Fixes #581

dgrisonnet commented 1 year ago

This requires https://github.com/kubernetes/test-infra/pull/29683.

booleanbetrayal commented 1 year ago

@dgrisonnet - Built an image off of this branch to test K8s v1.27 compatibility. Seems to work, except I see this regression again: https://github.com/kubernetes-sigs/prometheus-adapter/issues/525

Had to hot-patch the Helm produced manifest from the project to remove the '--logtostderr=true' line it generates.

Possibly broken in a prior commit?

dgrisonnet commented 1 year ago

This flag alongside others were removed from Kubernetes components in 1.26 and since prometheus-adapter is reusing code from the kube-apiserver, the same applies here. https://github.com/kubernetes/enhancements/issues/2845

I'll update the manifests in this repo to reflect that, but the helm chart will have to be fixed separately.

booleanbetrayal commented 1 year ago

This flag alongside others were removed from Kubernetes components in 1.26 and since prometheus-adapter is reusing code from the kube-apiserver, the same applies here. kubernetes/enhancements#2845

I'll update the manifests in this repo to reflect that, but the helm chart will have to be fixed separately.

Thanks for that explanation!

k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, olivierlemasle

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/prometheus-adapter/blob/master/OWNERS)~~ [dgrisonnet] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
olivierlemasle commented 1 year ago

/retest

olivierlemasle commented 1 year ago

/retest

justenwalker commented 1 year ago

FWIW I confirmed this change worked for me. I did my own build of the docker images and re-packaged the helm chart (removing the --logtostderr=true flag)

dgrisonnet commented 1 year ago

In this PR's state, it shouldn't work on 1.27. Both when I tried it locally and in CI the pod is crashlooping because I need to update metrics-server to support 1.27 before this can go in.

logicalhan commented 1 year ago

/triage accepted /assign

k8s-ci-robot commented 1 year ago

New changes are detected. LGTM label has been removed.

dgrisonnet commented 1 year ago

589 is required to support k8s 1.27

dgrisonnet commented 1 year ago

I added the OpenAPI v3 support to this PR directly since it requires client updates, but they require OpenAPI v3 support for the tests to pass.

dgrisonnet commented 1 year ago

/retest

justenwalker commented 1 year ago

@dgrisonnet

In this PR's state, it shouldn't work on 1.27. Both when I tried it locally and in CI the pod is crashlooping because I need to update metrics-server to support 1.27 before this can go in.

Not sure why it worked for me then. I'm running v0.6.3 (Helm Chart v3.10.0)

I updated to the latest commit today and it continues to work.

booleanbetrayal commented 1 year ago

Not sure why it worked for me then. I'm running v0.6.3 (Helm Chart v3.10.0)

I updated to the latest commit today and it continues to work.

Have also been running this branch's changes for the past couple of weeks without crash.

dgrisonnet commented 1 year ago

Now it should be all good, but before my change, if you didn't touch you the OpenAPI v3 feature gate on a 1.27 cluster, it wasn't working properly.

justenwalker commented 1 year ago

@dgrisonnet

before my change, if you didn't touch you the OpenAPI v3 feature gate on a 1.27 cluster, it wasn't working properly.

I'm deploying on EKS so I do not have access to these feature gates (so they are likely the defaults) - maybe that's why it worked?

justenwalker commented 1 year ago

@dgrisonnet is there any additional feedback or testing I can provide to validate this PR? Or is this waiting on approval from someone else?

colinrgodsey commented 1 year ago

also stuck on an EKS 1.27 upgrade here. any new developments here on what's needed to move forward with this PR ?

dgrisonnet commented 1 year ago

I am just waiting for a review from @olivierlemasle if possible

Aym3nTN commented 1 year ago

Can @booleanbetrayal review it?

dgrisonnet commented 1 year ago

I'd prefer having someone from kubernetes-sig signing it off, but if it still hasn't made any move next week, I'll merge it and create a new release.

justenwalker commented 1 year ago

@dgrisonnet seems unlikely we'll get that review from @olivierlemasle ; IMO since we've got some independent tests from myself and @booleanbetrayal and its relatively straight forward my vote would be to merge.

Only one note is that we're currently on Go 1.20.6 so it might be good to make that small tweak just to include the security fixes if its an easy enough change. https://github.com/kubernetes-sigs/prometheus-adapter/pull/586/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R11

justenwalker commented 1 year ago

@dgrisonnet thanks for closing this out! How does the helm chart for this get built; is that automatic?

The current app version available is v0.10.0

$ helm search hub prometheus-adapter
URL                                                     CHART VERSION   APP VERSION     DESCRIPTION
https://artifacthub.io/packages/helm/prometheus...      4.2.0           v0.10.0         A Helm chart for k8s prometheus adapter
...
dgrisonnet commented 1 year ago

I don't know, the chart is maintained separately.

colinrgodsey commented 1 year ago

@justenwalker helm chart PR here: https://github.com/prometheus-community/helm-charts/pull/3641

justenwalker commented 1 year ago

@justenwalker helm chart PR here: prometheus-community/helm-charts#3641

Nice! Thanks @colinrgodsey

colinrgodsey commented 1 year ago

@justenwalker merged!

$ helm search hub prometheus-adapter
URL                                                 CHART VERSION   APP VERSION DESCRIPTION                            
https://artifacthub.io/packages/helm/prometheus...  4.3.0           v0.11.0     A Helm chart for k8s prometheus adapter