Generate VPA CRD v1 from types.go #3606

Ladicle · 2020-10-12T16:46:50Z

ref: #3508
In this PR, I generate CRD v1 from types.go using controller-gen@v0.4.0.

Here are some things to note:

Generates only the API version that was served in the previous manifest.
The not recommended float64 is used as the CRD value, so allowDangerousType option is enabled. (ref: Support floats? kubernetes-sigs/controller-tools#245 (comment))
viacheckpoint have int map key that is not supported in JSON, so disable validation only for that field. Along with this, generation script ignores error returned by controller-gen. (ref: CRD: bad map key type: map keys must be strings #3574)
metav1.ObjectMeta does not accept optional tags, so remove it from types.go.

bskiba

Thanks, this looks interesting.
My main question is, what will happen for an existing vpa deployment that used the old yaml? Is the generated yaml compatible, so that the existing vpa objects will continue working?

bskiba · 2020-10-16T15:59:22Z

vertical-pod-autoscaler/hack/generate-crd-yaml.sh

+fi
+
+# The following commands always returns an error because controller-gen does not accept keys other than strings.
+${CONTROLLER_GEN} ${CRD_OPTS} paths="${APIS_PATH}/..." output:crd:dir=${WORKSPACE} ||:


Is there any way we can check for this specific error? What will happen if the generator fails for a different error?

Currently, controller-gen does not support feature to check for specific error or ignore only the specific error, but we can find other errors in the output. If the generator fails for a different error, this script will also output other than map keys must be strings.

$ hack/generate-crd-yaml.sh /Users/ladicle/src/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go:360:20: map keys must be strings, not int /Users/ladicle/src/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1beta2/types.go:340:20: map keys must be strings, not int Error: not all generators ran successfully run `controller-gen crd:trivialVersions=false,allowDangerousTypes=true paths=/Users/ladicle/src/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/... output:crd:dir=/var/folders/p_/hzm_gclx7vs5cdbfbzpl0s4w0000gn/T/tmp.Cytp8OtyCn -w` to see all available markers, or `controller-gen crd:trivialVersions=false,allowDangerousTypes=true paths=/Users/ladicle/src/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/... output:crd:dir=/var/folders/p_/hzm_gclx7vs5cdbfbzpl0s4w0000gn/T/tmp.Cytp8OtyCn -h` for usage

Another way is to grep the controller-gen output and display a warning message.

If there is a not-overly complicated way to indicate that something other went wrong, I would really like to see it. Otherwise it might get really confusing for someone encountering issues.

bskiba · 2020-10-16T16:02:33Z

vertical-pod-autoscaler/deploy/kustomization.yaml

@@ -3,4 +3,4 @@ resources:
 - recommender-deployment.yaml
 - updater-deployment.yaml
 - vpa-rbac.yaml
- vpa-v1-crd.yaml
+- vpa-v1-crd-v1.yaml


maybe vpa-v1-crd-gen.yaml

Ladicle · 2020-10-19T05:50:11Z

what will happen for an existing vpa deployment that used the old yaml? Is the generated yaml compatible, so that the existing vpa objects will continue working?

Yes, there is no change in the yaml field, so it's compatible. If you applied to a cluster which has old vpa, .spec.preserveUnknownFileds will be true, so the behaviour also will not change. Also, I've confirmed that vpa objects continue to work. However, clusters below v1.16 do not support CRD v1, so the new yaml can not be applied.

When the first time to install vpa, .spec.preserveUnknownFields will be false, so if you apply an invalid object to the cluster, you will get an error message.

like this:

$ kubectl apply -f examples/hamster.yaml
error: error validating "examples/hamster.yaml": error validating data: ValidationError(VerticalPodAutoscaler.spec.resourcePolicy.containerPolicies[0]): unknown field "controlledResources" in io.k8s.autoscaling.v1beta2.VerticalPodAutoscaler.spec.resourcePolicy.containerPolicies; if you choose to ignore these errors, turn validation off with --validate=false

bskiba

Thanks, left a couple more minor comments.

bskiba · 2020-11-03T15:29:02Z

vertical-pod-autoscaler/hack/vpa-process-yamls.sh

@@ -39,8 +39,17 @@ if [ $# -gt 2 ]; then
  exit 1
 fi

+SEMVER_RE='v[^0-9]*\([0-9]*\)[.]\([0-9]*\)[.]\([0-9]*\)\([0-9A-Za-z-]*\)'
+K8S_VERSION=$(kubectl version --short=true|grep Server|awk '{print $3}')


I don't think we need to check the k8s version - the 0.9 VPA requires 1.16 anyway: https://github.com/kubernetes/autoscaler/tree/master/vertical-pod-autoscaler#compatibility

bskiba · 2020-11-03T15:29:31Z

vertical-pod-autoscaler/hack/vpa-process-yamls.sh

+    COMPONENTS="vpa-v1-crd ${COMPONENTS}"
+else
+    COMPONENTS="vpa-v1-crd-v1-gen ${COMPONENTS}"
+fi
 case ${ACTION} in
 delete|diff|print) COMPONENTS+=" vpa-beta2-crd" ;;


What I meant is that we need to add vpa-v1-crd here to delete it on vpa-down.sh

Remove version-specific processing and specify only vpa-v1-crd-v1-gen; although the versions of the CRD are different, they define the same VPA resources, we cand delete CR using either crd-v1-crd or crd-v1-crd-gen files.

Makes sence, thank you

bskiba · 2020-11-03T15:30:55Z

vertical-pod-autoscaler/hack/generate-crd-yaml.sh

+fi
+
+# The following commands always returns an error because controller-gen does not accept keys other than strings.
+${CONTROLLER_GEN} ${CRD_OPTS} paths="${APIS_PATH}/..." output:crd:dir=${WORKSPACE} ||:


If there is a not-overly complicated way to indicate that something other went wrong, I would really like to see it. Otherwise it might get really confusing for someone encountering issues.

tamalsaha · 2020-11-03T22:48:51Z

viacheckpoint have int map key that is not supported in JSON, so disable validation only for that field. Along with this, generation script ignores error returned by controller-gen. (ref: #3574)

I don't think this is the right approach, since we are preserving a mistake. I recommend that @kubernetes/sig-api-machinery-api-reviews is consulted for this one. Thanks.

I think one way this can addressed is that introduce a new version of this api that has the correct data types. Then use CRD conversion webhook to migrate to that version.

fejta-bot · 2020-11-03T23:22:51Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

Ladicle · 2020-11-04T07:08:47Z

I agree with @tamalsaha that we should finally modify the VPA APIs to use string as the map key. Disabling validation of the BucketWeights field is just a workaround.
However, updating both the CRD API version and the VPA API version at once requires a lot of changes. IMHO, updating the CRD version and modifying VPA field will have less impact on the user if we work on them separately.

bskiba · 2020-11-24T14:54:38Z

I agree with @Ladicle on not mixing too many changes at once. I think there is already a lot of benefit from moving to the controller-gen here and the issue with incorrectly defined field is a separate one, which needs to be approached with a lot of care to not break existing users.

bskiba

Thanks, this looks good.
Small naming change and we should be good to merge.

bskiba · 2020-11-24T15:01:44Z

vertical-pod-autoscaler/deploy/kustomization.yaml

@@ -3,4 +3,4 @@ resources:
 - recommender-deployment.yaml
 - updater-deployment.yaml
 - vpa-rbac.yaml
- vpa-v1-crd.yaml
+- vpa-v1-crd-v1-gen.yaml


vpa-v1-crd-gen

Recheck all file names and unified to vpa-v1-crd-gen. Also, recreated the CRD manifest.

bskiba · 2020-11-24T15:02:04Z

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

@@ -73,7 +73,7 @@ for i in ${COMPONENTS}; do
  ALL_ARCHITECTURES=amd64 make --directory ${SCRIPT_ROOT}/pkg/${i} release
 done

-kubectl create -f ${SCRIPT_ROOT}/deploy/vpa-v1-crd.yaml
+kubectl create -f ${SCRIPT_ROOT}/deploy/vpa-v1-crd-v1-gen.yaml


vpa-v1-crd-gen

bskiba · 2020-12-02T16:46:42Z

Looks good, thanks a lot!

Can you squash the commits? Then it's ready to go :)

Ladicle · 2020-12-03T01:54:57Z

Thank you for reviewing :) I've squashed all commits.

mwielgus

/lgtm
/approve

k8s-ci-robot · 2020-12-07T11:26:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bskiba, Ladicle, mwielgus

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~~ [bskiba,mwielgus]

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 12, 2020

k8s-ci-robot requested review from jbartosik and krzysied October 12, 2020 16:47

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2020

Ladicle force-pushed the vpa-crd-v1 branch from a956106 to b674b69 Compare October 15, 2020 01:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2020

bskiba self-assigned this Oct 16, 2020

bskiba added the area/vertical-pod-autoscaler label Oct 16, 2020

bskiba reviewed Oct 16, 2020

View reviewed changes

bskiba reviewed Nov 3, 2020

View reviewed changes

bskiba mentioned this pull request Nov 3, 2020

CRD: bad map key type: map keys must be strings #3574

Closed

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Nov 3, 2020

bskiba approved these changes Nov 24, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2020

Generate VPA CRD v1 from types.go

6dc7f7a

Ladicle force-pushed the vpa-crd-v1 branch from c911b31 to 6dc7f7a Compare December 3, 2020 01:54

mwielgus approved these changes Dec 7, 2020

View reviewed changes

k8s-ci-robot assigned mwielgus Dec 7, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2020

k8s-ci-robot merged commit 5781b68 into kubernetes:master Dec 7, 2020

Ladicle deleted the vpa-crd-v1 branch December 9, 2020 01:29

jbartosik mentioned this pull request Jun 17, 2022

i am facing the same issue as https://github.com/kubernetes/autoscaler/issues/4775 #4968

Closed

Shubham82 mentioned this pull request Jul 21, 2022

CustomResourceDefinition problem with VPA #4897

Closed

wu0407 mentioned this pull request Apr 12, 2023

vpa CRD not contain status subresource #5675

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate VPA CRD v1 from types.go #3606

Generate VPA CRD v1 from types.go #3606

Ladicle commented Oct 12, 2020 •

edited

Loading

bskiba left a comment

bskiba Oct 16, 2020

Ladicle Oct 19, 2020

bskiba Nov 3, 2020

bskiba Oct 16, 2020

Ladicle commented Oct 19, 2020 •

edited

Loading

bskiba left a comment

bskiba Nov 3, 2020

bskiba Nov 3, 2020

Ladicle Nov 4, 2020

bskiba Nov 24, 2020

bskiba Nov 3, 2020

tamalsaha commented Nov 3, 2020 •

edited

Loading

fejta-bot commented Nov 3, 2020

Ladicle commented Nov 4, 2020

bskiba commented Nov 24, 2020

bskiba left a comment

bskiba Nov 24, 2020

Ladicle Nov 27, 2020

bskiba Nov 24, 2020

bskiba commented Dec 2, 2020

Ladicle commented Dec 3, 2020

mwielgus left a comment

k8s-ci-robot commented Dec 7, 2020

Generate VPA CRD v1 from types.go #3606

Generate VPA CRD v1 from types.go #3606

Conversation

Ladicle commented Oct 12, 2020 • edited Loading

bskiba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicle commented Oct 19, 2020 • edited Loading

bskiba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamalsaha commented Nov 3, 2020 • edited Loading

fejta-bot commented Nov 3, 2020

Ladicle commented Nov 4, 2020

bskiba commented Nov 24, 2020

bskiba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bskiba commented Dec 2, 2020

Ladicle commented Dec 3, 2020

mwielgus left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 7, 2020

Ladicle commented Oct 12, 2020 •

edited

Loading

Ladicle commented Oct 19, 2020 •

edited

Loading

tamalsaha commented Nov 3, 2020 •

edited

Loading