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

Inherit existing sset.volumeClaimTemplates ownerReferences #2217

Merged
merged 2 commits into from
Dec 5, 2019

Conversation

sebgl
Copy link
Contributor

@sebgl sebgl commented Dec 5, 2019

Fixes #2192.

When we update the Elasticsearch CRD to a new version, existing
ownerReferences in StatefulSets.volumeClaimTemplates still point to the
"old" Elasticsearch apiVersion (eg. v1beta1 instead of v1).

Since the volumeClaimtemplate section is immutable, the ownerRef
apiVersion cannot be updated so the StatefulSet update is rejected.
In that situation, it is fine to just keep the existing ownerRef
targeting the deprecated Elasticsearch apiVersion. It does not prevent
resource garbage collection to happen as expected.

This commit also changes the way we recently set a Namespace to the
claim, to not error out in SetControllerReference. We don't need that
namespace (and it breaks backward compatibility), so we now just set it
temporarily for SetControllerReference to not return any error.

When we update the Elasticsearch CRD to a new version, existing
ownerReferences in StatefulSets.volumeClaimTemplates still point to the
"old" Elasticsearch apiVersion (eg. `v1beta1` instead of `v1`).

Since the volumeClaimtemplate section is immutable, the ownerRef
apiVersion cannot be updated so the StatefulSet update is rejected.
In that situation, it is fine to just keep the existing ownerRef
targeting the deprecated Elasticsearch apiVersion. It does not prevent
resource garbage collection to happen as expected.

This commit also changes the way we recently set a Namespace to the
claim, to not error out in `SetControllerReference`. We don't need that
namespace (and it breaks backward compatibility), so we now just set it
temporarily for `SetControllerReference` to not return any error.
@sebgl sebgl added >bug Something isn't working v1.0.0 labels Dec 5, 2019
@sebgl sebgl mentioned this pull request Dec 5, 2019
claim.Namespace = es.Namespace
if err := reconciler.SetControllerReference(&es, &claim, scheme); err != nil {
return nil, err
}
claim.Namespace = ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This deals with bdf1e1f in a backward-compatible way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for including the explanation. Since we're using our own version for now it would have taken me a bit to understand

Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

lgtm

@sebgl sebgl merged commit 5110d6e into elastic:master Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug Something isn't working v1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StatefulSet's VolumeClaimTemplates ownerReference cannot be patched from apiVersion v1beta1 to v1
3 participants