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

Fix default elasticsearch-data volumeMount configuration #6725

Merged
merged 4 commits into from
Apr 28, 2023

Conversation

thbkrkr
Copy link
Contributor

@thbkrkr thbkrkr commented Apr 25, 2023

This commit ensures that we take into account the volumes provided by the user to append the default elasticsearch-data volumeMount.

Resolves #6186.

@thbkrkr thbkrkr added the >bug Something isn't working label Apr 25, 2023
@thbkrkr
Copy link
Contributor Author

thbkrkr commented Apr 26, 2023

Another approach could be to move the call to AppendDefaultDataVolumeMount in BuildPodTemplateSpec so that we are working with the full list of volumes resulting from merging default ECK volumes and user-provided volumes.

diff --git a/pkg/controller/elasticsearch/nodespec/podspec.go b/pkg/controller/elasticsearch/nodespec/podspec.go
index 52fcda3cd..a012d3f12 100644
--- a/pkg/controller/elasticsearch/nodespec/podspec.go
+++ b/pkg/controller/elasticsearch/nodespec/podspec.go
@@ -110,7 +110,11 @@ func BuildPodTemplateSpec(
                WithReadinessProbe(*NewReadinessProbe()).
                WithAffinity(DefaultAffinity(es.Name)).
                WithEnv(DefaultEnvVars(es.Spec.HTTP, headlessServiceName)...).
-               WithVolumes(volumes...).
+               WithVolumes(volumes...)
+
+       volumeMounts = esvolume.AppendDefaultDataVolumeMount(volumeMounts, volumes)
+
+       builder = builder.
                WithVolumeMounts(volumeMounts...).
                WithInitContainers(initContainers...).

@thbkrkr
Copy link
Contributor Author

thbkrkr commented Apr 26, 2023

Since there is always an elasticsearch-data volume, either provided by the user (with or without pvc), or configured by default by ECK, I wonder if we should not just add the corresponding volumeMount statically.

diff --git a/pkg/controller/elasticsearch/nodespec/volumes.go b/pkg/controller/elasticsearch/nodespec/volumes.go
index c5da73430..beabe273b 100644
--- a/pkg/controller/elasticsearch/nodespec/volumes.go
+++ b/pkg/controller/elasticsearch/nodespec/volumes.go
@@ -106,6 +106,7 @@ func buildVolumes(
                scriptsVolume.VolumeMount(),
                configVolume.VolumeMount(),
                downwardAPIVolume.VolumeMount(),
+               esvolume.DefaultDataVolumeMount,
        )

@thbkrkr thbkrkr requested a review from barkbay April 26, 2023 11:59
@thbkrkr thbkrkr added the v2.8.0 label Apr 27, 2023
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM I did a couple of tests and I think it works as expected. I also think it is safe as use provided mounts will still prevail.

@@ -114,7 +114,7 @@ func buildVolumes(
volumeMounts = append(volumeMounts, fileSettingsVolume.VolumeMount())
}

volumeMounts = esvolume.AppendDefaultDataVolumeMount(volumeMounts, volumes)
volumeMounts = esvolume.AppendDefaultDataVolumeMount(volumeMounts, append(volumes, nodeSpec.PodTemplate.Spec.Volumes...))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a comment here to explain why we are suddenly including the user provided PodTemplate here.

@thbkrkr thbkrkr merged commit 294f302 into elastic:main Apr 28, 2023
@thbkrkr thbkrkr deleted the fix-es-data-volume-mount-data branch May 10, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug Something isn't working v2.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

emptyDir volume is not mounted
2 participants