kubeflow / common

Common APIs and libraries shared by other Kubeflow operator repositories.
Apache License 2.0
51 stars 73 forks source link

pr 172 breaks training-operator #175

Open zw0610 opened 2 years ago

zw0610 commented 2 years ago

172 breaks training-operator when installing crd

reproduce steps:

  1. change go.mod and go.sum

    diff --git a/go.mod b/go.mod
    index 58f089d7..d491f115 100644
    --- a/go.mod
    +++ b/go.mod
    @@ -5,7 +5,7 @@ go 1.17
    require (
        github.com/go-logr/logr v0.3.0
        github.com/go-openapi/spec v0.20.3
    -       github.com/kubeflow/common v0.3.7
    +       github.com/kubeflow/common v0.4.1-0.20211106174700-a1f26a9cd2f5
        github.com/onsi/ginkgo v1.14.1
        github.com/onsi/gomega v1.10.2
        github.com/prometheus/client_golang v1.10.0
  2. run make manifets

    
    On branch master
    Your branch is behind 'origin/master' by 1 commit, and can be fast-forwarded.
    (use "git pull" to update your local branch)

Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git restore ..." to discard changes in working directory) modified: go.mod modified: go.sum modified: manifests/base/crds/kubeflow.org_mxjobs.yaml modified: manifests/base/crds/kubeflow.org_pytorchjobs.yaml modified: manifests/base/crds/kubeflow.org_tfjobs.yaml modified: manifests/base/crds/kubeflow.org_xgboostjobs.yaml

no changes added to commit (use "git add" and/or "git commit -a")


3. run `make install`
```shell
which: no golangci-lint in (/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/root/bin:/usr/local/go/bin)
/root/test/training-operator/bin/controller-gen "crd:trivialVersions=true,preserveUnknownFields=false,generateEmbeddedObjectMeta=true" rbac:roleName=manager-role webhook paths="./pkg/apis/..." output:crd:artifacts:config=manifests/base/crds
go: creating new go.mod: module tmp
Downloading sigs.k8s.io/kustomize/kustomize/v3@v3.8.7
go: downloading sigs.k8s.io/kustomize/kustomize/v3 v3.8.7
...
go get: added sigs.k8s.io/yaml v1.2.0
/root/test/training-operator/bin/kustomize build manifests/base/crds | kubectl apply -f -
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "mxjobs.kubeflow.org" is invalid: spec.validation.openAPIV3Schema.properties[status].properties[conditions].items.x-kubernetes-map-type: Invalid value: "null": must be atomic as item of a list with x-kubernetes-list-type=set
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "pytorchjobs.kubeflow.org" is invalid: spec.validation.openAPIV3Schema.properties[status].properties[conditions].items.x-kubernetes-map-type: Invalid value: "null": must be atomic as item of a list with x-kubernetes-list-type=set
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "tfjobs.kubeflow.org" is invalid: spec.validation.openAPIV3Schema.properties[status].properties[conditions].items.x-kubernetes-map-type: Invalid value: "null": must be atomic as item of a list with x-kubernetes-list-type=set
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "xgboostjobs.kubeflow.org" is invalid: spec.validation.openAPIV3Schema.properties[status].properties[conditions].items.x-kubernetes-map-type: Invalid value: "null": must be atomic as item of a list with x-kubernetes-list-type=set
make: *** [Makefile:83: install] Error 1
zw0610 commented 2 years ago

anyone knows how to fix it?

gaocegege commented 2 years ago

cc @pugangxa

pugangxa commented 2 years ago

There's bunch of this errors like https://github.com/kubernetes/kubernetes/issues/91395 when searched and occur even after I changed to listType=map in https://github.com/kubeflow/common/pull/174 . Either with a type of set or map will add the item as required in schema. Investigated and seems the solution to add defaulter to default the values for the map key. But I don't think it's reasonable to add default value to the condition. Also since there's a lot of this kind of API rule violation which is just a warning but the open api definition is still generated. I will rollback it in https://github.com/kubeflow/common/pull/174

pugangxa commented 2 years ago

The error is reported from https://github.com/kubernetes/apiextensions-apiserver/blob/e52d5683d8e566b9989f59c09d74bdb0da937981/pkg/apis/apiextensions/validation/validation.go#L852-L861 , I have rollback the changes in https://github.com/kubeflow/common/pull/174 , please take a look.

alculquicondor commented 2 years ago

Have you tried adding // +optional and omitempty?

pugangxa commented 2 years ago

Have you tried adding // +optional and omitempty?

Did a quick test by adding // +optional and omitempty, although this can remove condition from required in the CRD, but still report this error when applying. I think there's lots of this api rule violation(before I think it's only this one) during compiling common so we can leave this for now and address together if needed.