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.

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

Lots of resources have status fields which breaks ACM #1174

Closed
jlewi opened this issue May 15, 2020 · 4 comments
Closed

Lots of resources have status fields which breaks ACM #1174

jlewi opened this issue May 15, 2020 · 4 comments
Assignees
@jlewi
Labels
kind/bug platform/gcp

Comments

@jlewi
Copy link
Contributor

jlewi commented May 15, 2020

A lot of our K8s resources (e.g. Jupyter CRD) have status set

manifests/jupyter/notebook-controller/base/crd.yaml

Line 65 in d82342c

status:

This breaks ACM validation and likely other validation as well. I suspect having status fields breaks apply. So we need to fix this as part of upgrades (kubeflow/kfctl#304)

As part of fixing this we should add a validation test.

Related to: GoogleCloudPlatform/kubeflow-distribution#4

/assign @jlewi
@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the labels:

Label Probability
kind/bug 0.99
area/jupyter 0.91

Please mark this comment with 👍 or 👎 to give our bot feedback!
Links: app homepage, dashboard and code for this bot.

1 similar comment
@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the labels:

Label Probability
kind/bug 0.99
area/jupyter 0.91

Please mark this comment with 👍 or 👎 to give our bot feedback!
Links: app homepage, dashboard and code for this bot.

@jlewi jlewi mentioned this issue May 16, 2020
Closed
@jlewi jlewi changed the title Lots of resources have status fields which breaks validation Lots of resources have status fields which breaks ACM May 16, 2020
@jlewi
Copy link
Contributor Author

jlewi commented May 16, 2020

It looks like the existence of status fields in CRDs is rampant.

Argo doesn't have it
https://github.com/argoproj/argo/blob/master/manifests/base/crds/workflow-crd.yaml

Not seeing it in cert-manager either
https://github.com/jetstack/cert-manager/blob/master/deploy/crds/crd-clusterissuers.yaml

KFServing has it:
https://github.com/kubeflow/kfserving/blob/e25758f6da020e3e8ad63f0880ab95de6af8395b/config/default/crds/base/serving.kubeflow.org_inferenceservices.yaml#L6662

Notebook has it
https://github.com/kubeflow/kubeflow/blob/c7150a18b3a567a7f1d59893a0ef2adc1245ae14/components/notebook-controller/config/crd/bases/kubeflow.org_notebooks.yaml#L4265

kfctl doesn't have it.
https://github.com/kubeflow/kfctl/blob/master/deploy/crds/kfdef.apps.kubeflow.org_kfdefs_crd.yaml

My conjecture is that this is a bug in controller-runtime that got fixed in later versions.
Looks like kfctl is using 0.4.0
https://github.com/kubeflow/kfctl/blob/aaba770b461a27fc59d4a7960df48a3da003eda2/go.mod#L61

Looks like notebook controller is using 0.2.0
https://github.com/kubeflow/kubeflow/blob/master/components/notebook-controller/go.mod

jlewi pushed a commit to jlewi/manifests that referenced this issue May 16, 2020
CRDs should not have status field.
6d7d6a2 
* kubeflow#1174 lots of CRDs have status fields which
  causes ACM to complain that the CRD is invalid.

* This PR cleans up those CRDs and adds an appropriate validation test.

* Fix typos in AWS application specs (kubeflow/testing#668)
@jlewi jlewi mentioned this issue May 16, 2020
Merged
k8s-ci-robot pushed a commit that referenced this issue May 18, 2020
@jlewi
CRDs should not have status field. (#1175)
9e38132 
* CRDs should not have status field.

* #1174 lots of CRDs have status fields which
  causes ACM to complain that the CRD is invalid.

* This PR cleans up those CRDs and adds an appropriate validation test.

* Fix typos in AWS application specs (kubeflow/testing#668)

* Update tests.
jlewi pushed a commit to jlewi/manifests that referenced this issue May 19, 2020
Remove status fields from CRDs and empty annotations
193ca31 
* Related to kubeflow#1174.

* This is a follow on to kubeflow#1175. kubeflow#1175 attempted to fix this issue
  but the unittest was insufficient because it didn't handle the
  case where there were multiple K8s resources in a file.

  * The test is updated to handle that case. We use a more powerful
    YAML processing library kyaml from kustomize.

* Also remove empty annotations (GoogleCloudPlatform/kubeflow-distribution#16) because
  this triggers a bug in kpt.
@jlewi jlewi mentioned this issue May 19, 2020
Merged
k8s-ci-robot pushed a commit that referenced this issue May 20, 2020
@jlewi
Remove status fields from CRDs and empty annotations (#1179)
a6755ff 
* Related to #1174.

* This is a follow on to #1175. #1175 attempted to fix this issue
  but the unittest was insufficient because it didn't handle the
  case where there were multiple K8s resources in a file.

  * The test is updated to handle that case. We use a more powerful
    YAML processing library kyaml from kustomize.

* Also remove empty annotations (GoogleCloudPlatform/kubeflow-distribution#16) because
  this triggers a bug in kpt.
@jlewi
Copy link
Contributor Author

jlewi commented May 21, 2020 •
edited
Loading

This should be fixed by #1175 and #1179.

@jlewi jlewi closed this as completed May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jlewi jlewi

Labels
kind/bug platform/gcp
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@jlewi