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

Generate CRD specs, bump to v1beta2 #578

Merged
merged 9 commits into from
Sep 13, 2019

Conversation

khogeland
Copy link
Contributor

@khogeland khogeland commented Aug 22, 2019

The current, manually-defined CRD validation is really unsafe, which we just learned of due to an outage:

  • Since k8s will ignore unknown fields, if all fields aren't listed, values could be of the wrong type
  • Fields that are listed but are missing type annotations can be replaced by values of the wrong type
  • Deserialization errors will apparently break the Lister (and therefore the entire controller) because it deserializes the entire SparkApplicationList at once.

This PR replaces the manually written CRD specs with ones generated by the controller-gen tool (part of Kubebuilder). It also gets rid of CRD self-registration, because there is no CRD Go code generated (I explored bundling the generated YAML, but it would be a hack and doesn't seem that useful).

Also bumped the version because this is a backwards-incompatible change:

  • I changed the Cores field from float32 to int32, because Spark parses it as an integer, and controller-gen doesn't support floats
  • I assume that the stricter validation, while not being an API change, may reject some applications that previously worked

Existing validation has been migrated over, with the exception of the metadata.name field length, because I can't put annotations on a field from k8s. I'd like to figure out how to get that validation back, currently it'll fail at spark-submit.

I'm pretty sure I missed a few things when bumping the version, and some docs are now out of date, so I'll clean that up asap. Sorry I'm dumping another large PR on you without warning!

@liyinan926
Copy link
Collaborator

Please rebase. I will need to cut a final release for v1beta1 before being able to review and merge this one.

@khogeland
Copy link
Contributor Author

Might be a good time to consider finalizing this PR

@liyinan926
Copy link
Collaborator

Can you rebase this?

Copy link
Collaborator

@liyinan926 liyinan926 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized that we also need to update the Helm Chart to include the generated CRDs once we cut a release for the new v1beta2 version since the chart relies the current CRD self-registration.

README.md Outdated Show resolved Hide resolved
@liyinan926
Copy link
Collaborator

@khogeland have you been running this version in production?

@khogeland
Copy link
Contributor Author

We haven't, it seems too far-reaching (i.e. too likely to turn into a merge conflict nightmare) to maintain in an internal fork.

@liyinan926
Copy link
Collaborator

All right. Please fix the remaining typo and I will merge this.

@liyinan926
Copy link
Collaborator

Any more commits coming? If no, will merge soon.

@khogeland
Copy link
Contributor Author

Nope, that should be the last of them.

@liyinan926 liyinan926 merged commit 55a1eeb into kubeflow:master Sep 13, 2019
jbhalodia-slack pushed a commit to jbhalodia-slack/spark-operator that referenced this pull request Oct 4, 2024
* Generate CRD specs, bump to v1beta2

* Add short/singular CRD names

* Merge upstream/master

* Tweak Cores validation

* Fix typo, merge upstream

* Update remaining docs for v1beta2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants