Skip to content
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.

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

seldon-core-operator CRD's incompatible with K8s v1.18 #1675

Closed
iamlittle opened this issue Apr 8, 2020 · 16 comments · Fixed by #2339
Closed

seldon-core-operator CRD's incompatible with K8s v1.18 #1675

iamlittle opened this issue Apr 8, 2020 · 16 comments · Fixed by #2339
Assignees
Projects
Milestone

Comments

@iamlittle
Copy link

Installing the seldon-core-operator on a fresh v1.18 Kubernetes install throws an error:

Error: CustomResourceDefinition.apiextensions.k8s.io "seldondeployments.machinelearning.seldon.io" is invalid: [spec.validation.openAPIV3Schema.properties[spec].properties[predictors].items.properties[componentSpecs].items.properties[spec].properties[containers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property, spec.validation.openAPIV3Schema.properties[spec].properties[predictors].items.properties[componentSpecs].items.properties[spec].properties[initContainers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property, spec.validation.openAPIV3Schema.properties[spec].properties[predictors].items.properties[explainer].properties[containerSpec].properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property]

I believe this is due to an API change in v1.18.

CustomResourceDefinition schemas that use x-kubernetes-list-map-keys to specify properties that uniquely identify list items must make those properties required or have a default value, to ensure those properties are present for all list items. See https://kubernetes.io/docs/reference/using-api/api-concepts/#merge-strategy for details. (#88076, @eloyekunle) [SIG API Machinery and Testing]

I was able to work around by installing a crd definition exported from a successful v1.17 install and installing seldon-core-operator with crd.create=false.

Steps to reproduce:

curl https://get.helm.sh/helm-v3.1.2-linux-amd64.tar.gz -o helm-v3.1.2-linux-amd64.tar.gz
tar -xzvf helm-v3.1.2-linux-amd64.tar.gz
chmod a+x linux-amd64/helm
mv linux-amd64/helm /usr/local/bin/

helm repo add ambassador https://www.getambassador.io
helm repo add seldon https://storage.googleapis.com/seldon-charts

helm upgrade ambassador ambassador/ambassador --install
helm upgrade seldon-core seldon/seldon-core-operator --install --version=1.0.3-SNAPSHOT --set ambassador.enabled=true --set crd.create=true
@iamlittle iamlittle added bug triage Needs to be triaged and prioritised accordingly labels Apr 8, 2020
@ukclivecox
Copy link
Contributor

Thanks for these details. We will need to see what we need to add to the Kustomize definitions.

@ukclivecox ukclivecox added priority/p0 and removed triage Needs to be triaged and prioritised accordingly labels Apr 9, 2020
@DenisOgr
Copy link

Same problem! Downgrade Kubernetes to 1.17.0 and it fixes.

@xaniasd
Copy link
Contributor

xaniasd commented May 7, 2020

FYI this affects local development as well, because kind images are not pinned to a specific version

@vega42
Copy link

vega42 commented May 29, 2020

I can confirm. After downgrading my minikube using:

minikube start --kubernetes-version v1.17.0

I can get past the issue in the seldon-core setup.

@Atharex
Copy link

Atharex commented May 29, 2020

The same error came up with the prometheus operator.
prometheus-operator/prometheus-operator#3122

I can confirm that when I adapted the schema to make the property required, it fixed the problem and the install works again on 1.18+ (though not sure if that is a proper "fix" for your operator)

In the prometheus operator they fixed the problem by upgrading their CRD API to v1 for kubernetes >= 1.18 and keeping also the v1beta1 API for kubernetes < 1.16

@ukclivecox
Copy link
Contributor

Thanks for the info we will be looking to solve this post 1.2.0 release for 1.3.0 release

@shawnzhu
Copy link

I hit this problem when installing kubeflow (w/ seldon-core-operator:1.1.0) on k8s v1.18. is this caused by kubernetes/kubernetes#91395 ?

@ukclivecox
Copy link
Contributor

Yes that is the core reason. We need to upgrade to v1 CRDs but need at same time to keep backwards compataibility for older k8s. So I think we will need options to create both.

@ukclivecox ukclivecox marked this as a duplicate of #1678 Jun 25, 2020
@ukclivecox ukclivecox reopened this Jun 25, 2020
@ukclivecox ukclivecox marked this as not a duplicate of #1678 Jun 25, 2020
@ukclivecox ukclivecox added this to To do in 1.3 via automation Jun 25, 2020
@ukclivecox ukclivecox added this to the 1.3 milestone Jun 25, 2020
@ukclivecox ukclivecox self-assigned this Aug 23, 2020
@ukclivecox ukclivecox moved this from To do to In progress in 1.3 Aug 27, 2020
@ukclivecox ukclivecox moved this from In progress to In Review in 1.3 Aug 28, 2020
1.3 automation moved this from In Review to Done Aug 30, 2020
@shadowmodder
Copy link

+1 need this backward compatible

@ukclivecox
Copy link
Contributor

It should be. The helm chart checks for k8s version and deploys a v1beta1 CRD for k8s <1.18 and a v1 CRD for >- 1.18.
There is #2367 which we need to investigate before release.

@arunbenoyv
Copy link

kubectl version
Client Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.8", GitCommit:"9f2892aab98fe339f3bd70e3c470144299398ace", GitTreeState:"clean", BuildDate:"2020-08-13T16:12:48Z", GoVersion:"go1.13.15", Compiler:"gc", Platform:"linux/amd64"}

Helm install is resulting in the same error

helm install seldon-core seldon-core-operator --repo https://storage.googleapis.com/seldon-charts --set usageMetrics.enabled=true --set ambassador.enabled=true
Error: CustomResourceDefinition.apiextensions.k8s.io "seldondeployments.machinelearning.seldon.io" is invalid: [spec.validation.openAPIV3Schema.properties[spec].properties[predictors].items.properties[explainer].properties[containerSpec].properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property, spec.validation.openAPIV3Schema.properties[spec].properties[predictors].items.properties[componentSpecs].items.properties[spec].properties[initContainers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property, spec.validation.openAPIV3Schema.properties[spec].properties[predictors].items.properties[componentSpecs].items.properties[spec].properties[containers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property]

@ukclivecox
Copy link
Contributor

The fix is in master and will be in next release @arunbenoyv

@daddydrac
Copy link

daddydrac commented Feb 17, 2022

Separate but related ques here: When we set the --set crd.create=true to true, what does that CRD do and what is it for? I could not find anything in the docs that explains this.

@ukclivecox
Copy link
Contributor

This is used for cases where we want the operator to create the CRD - its usually used just in Marketplaces where we can't install the CRD separately first

@daddydrac
Copy link

daddydrac commented Feb 17, 2022 via email

@ukclivecox
Copy link
Contributor

Worth following our install methods to see if they fit your use. With Helm the chart will install the CRD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
1.3
  
Done
Development

Successfully merging a pull request may close this issue.

10 participants