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

Add validation for declared volume claim templates #4526

Merged
merged 2 commits into from
May 28, 2021

Conversation

pebrc
Copy link
Collaborator

@pebrc pebrc commented May 27, 2021

Partially addresses #4525

Adds a validation to check that declared volume claim templates are actually mounted in any of the containers. This validation does not apply if the user configures the documented default data volume elasticsearch-data for which ECK manages the volume mount.

There is still a possibility for misconfiguration. As we are currently supporting custom volume mounts and custom data.path settings in Elasticsearch it is quite hard to fully validate the configuration. With the proposed validation it would still be possible for a user to configure a custom volume claim template, mount it in a side car and have no persistent Elasticsearch data volume. This could be addressed in two ways:

  • also parse the Elasticsearch configuration during validation and make sure each data.path has a volume mount
  • always configure a default data volume as we had it pre-1.4.0

@pebrc pebrc added >enhancement Enhancement of existing functionality v1.7.0 labels May 27, 2021
Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

LGTM. Left a few comments for minor improvements.

One concern is that existing users with misconfigured volumes would suddenly have their cluster reconciliation error-out when they upgrade the operator to a version with that validation. I think that's OK (better point out the problem and stop reconciling rather than silently ignoring it?).

With the proposed validation it would still be possible for a user to configure a custom volume claim template, mount it in a side car and have no persistent Elasticsearch data volume. This could be addressed in two ways

I think I'm fine with not addressing this edge case.

pkg/controller/elasticsearch/validation/validations.go Outdated Show resolved Hide resolved
pkg/controller/elasticsearch/validation/validations.go Outdated Show resolved Hide resolved
@pebrc
Copy link
Collaborator Author

pebrc commented May 28, 2021

Jenkins test this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality v1.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants