Skip to content

Add explicit schema for podTemplateSpec.metadata #676

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

Merged

Conversation

jkylling
Copy link
Contributor

Unknown metadata fields are pruned https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#field-pruning
If you change from v1beta CRDs to v1 CRDs the metadata field is pruned and the operator believes that every podSpec is incorrect.
Related to kubernetes-sigs/controller-tools#448

Unknown metadata fields are pruned https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#field-pruning
If you change from v1beta CRDs to v1 CRDs the metadata field is pruned and the operator believes that every podSpec is incorrect.
Related to kubernetes-sigs/controller-tools#448
Copy link
Member

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR! I think you have to change in the Makefile the CRD_OPTIONS and add generateEmbeddedObjectMeta=true e.g. to be CRD_OPTIONS ?= "crd:trivialVersions=true,maxDescLen=0,crdVersions=v1,generateEmbeddedObjectMeta=true" otherwise a new run of make will remove it again (that's why CI is complaining). After the change you can just run make clean all and commit the newly generated files. I just noticed that the new field is only available in 0.6.0-beta 🤔

@jkylling
Copy link
Contributor Author

Thanks! Would it be possible to add unit tests to catch issues like this?

Another tiny annoyance when upgrading the CRDs is that some extra default values are populated (e.g., podSpec.containers[].ports[].protocol was empty, and is now TCP). The operator detects that the last applied podSpec is different from what is now defined in the foundationdbcluster CR and decides to bounce the pod, although the resulting pod is identical (except of metadata.annotations["foundationdb.org/last-applied-spec"]). Would the operator be able to avoid most no-ops like this.

@jkylling jkylling requested a review from johscheuer May 10, 2021 19:20
@johscheuer
Copy link
Member

Thanks! Would it be possible to add unit tests to catch issues like this?

Using unit-tests will be probably a little bit hard (I would need to check if the default mechanisms are exposed in Kubernetes) but it should be pretty straight forward to add some integration tests for that and I think that most of our tests are actually integration tests. Feel free to add some test cases for that in the PR that would also show/prove that the change actually works like expected.

Another tiny annoyance when upgrading the CRDs is that some extra default values are populated (e.g., podSpec.containers[].ports[].protocol was empty, and is now TCP). The operator detects that the last applied podSpec is different from what is now defined in the foundationdbcluster CR and decides to bounce the pod, although the resulting pod is identical (except of metadata.annotations["foundationdb.org/last-applied-spec"]). Would the operator be able to avoid most no-ops like this.

That should be pretty simple to fix by not using foundationdb.org/last-applied-spec but rather compare the actual specs and ignore all default fields. Let me create an issue for that.

Copy link
Member

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

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

The changes look in general good, I'm just not quite sure if we want to use a beta version directly. Let me evaluate that and come back to you.

Copy link
Member

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks!

@johscheuer johscheuer merged commit 3ac1a75 into FoundationDB:master May 20, 2021
@manfontan manfontan deleted the podTemplateSpec.metadata-schema branch April 24, 2023 22:11
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