This is an automated email from the ASF dual-hosted git repository. xianjingfeng pushed a commit to branch branch-0.8 in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git
commit cf25897a72f4a6cde34dccc5f5f7844a410b545a Author: QI Jiale <[email protected]> AuthorDate: Sat Nov 18 14:41:56 2023 +0800 [#1290] improvement(operator): Avoid accidentally deleting data of other services when misconfiguring the mounting directory (#1291) ### What changes were proposed in this pull request? Only delete `rssdata` directory. ### Why are the changes needed? Fix: #1290 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Tested in our cluster. 1. Mount `/data/hdfs1/rssdata1` dir for shuffle server，the server will create `/data/hdfs1/rssdata1/rssdata/` 2. Create files manually at host machine ``` touch /data/hdfs1/rssdata1/a.txt touch /data/hdfs1/rssdata1/rssdata/b.txt ``` 3. Update crd to terminate this shuffle server. 4. Without this pr, both `a.txt`, `b.txt` and `rssdata` dir are deleted 5. With this pr, only `b.txt` is deleted Co-authored-by: 齐家乐(26731624) <[email protected]> (cherry picked from commit be106976474cc02a614ca842421670debf50f7aa) --- .../operator/pkg/controller/constants/constants.go | 3 ++ .../controller/sync/shuffleserver/shuffleserver.go | 2 +- .../operator/pkg/controller/util/util.go | 3 +- .../operator/pkg/controller/util/util_test.go | 51 ++++++++++++++++++++++ 4 files changed, 57 insertions(+), 2 deletions(-) diff --git a/deploy/kubernetes/operator/pkg/controller/constants/constants.go b/deploy/kubernetes/operator/pkg/controller/constants/constants.go index 8bda1ca6b..41c0f34c0 100644 --- a/deploy/kubernetes/operator/pkg/controller/constants/constants.go +++ b/deploy/kubernetes/operator/pkg/controller/constants/constants.go @@ -58,6 +58,9 @@ const ( // ConfigurationVolumeName is the name of configMap volume records configuration of coordinators or shuffle servers. ConfigurationVolumeName = "configuration" + + //RssDataDir is the directory name for RSS data as the local storage. + RssDataDir = "rssdata" ) // PropertyKey defines property key in configuration of coordinators or shuffle servers. diff --git a/deploy/kubernetes/operator/pkg/controller/sync/shuffleserver/shuffleserver.go b/deploy/kubernetes/operator/pkg/controller/sync/shuffleserver/shuffleserver.go index 7b4baad69..6404456ba 100644 --- a/deploy/kubernetes/operator/pkg/controller/sync/shuffleserver/shuffleserver.go +++ b/deploy/kubernetes/operator/pkg/controller/sync/shuffleserver/shuffleserver.go @@ -236,7 +236,7 @@ func generateStorageBasePath(rss *unifflev1alpha1.RemoteShuffleService) string { if k == rss.Spec.ShuffleServer.LogHostPath { continue } - paths = append(paths, strings.TrimSuffix(v, "/")+"/rssdata") + paths = append(paths, strings.TrimSuffix(v, "/")+"/"+controllerconstants.RssDataDir) } sort.Strings(paths) return strings.Join(paths, ",") diff --git a/deploy/kubernetes/operator/pkg/controller/util/util.go b/deploy/kubernetes/operator/pkg/controller/util/util.go index 99cb0aff8..5a1375f43 100644 --- a/deploy/kubernetes/operator/pkg/controller/util/util.go +++ b/deploy/kubernetes/operator/pkg/controller/util/util.go @@ -86,7 +86,8 @@ func addVolumeMountsOfMainContainer(mainContainer *corev1.Container, mainContainer.VolumeMounts = append(mainContainer.VolumeMounts, GenerateHostPathVolumeMounts(hostPathMounts)...) for _, mountPath := range hostPathMounts { - clearPathCMDs = append(clearPathCMDs, fmt.Sprintf("rm -rf %v/*", strings.TrimSuffix(mountPath, "/"))) + clearPathCMDs = append(clearPathCMDs, fmt.Sprintf("rm -rf %v/%v/*", + strings.TrimSuffix(mountPath, "/"), controllerconstants.RssDataDir)) } if len(clearPathCMDs) > 0 { mainContainer.Lifecycle = &corev1.Lifecycle{ diff --git a/deploy/kubernetes/operator/pkg/controller/util/util_test.go b/deploy/kubernetes/operator/pkg/controller/util/util_test.go index 864d8ee2d..d7e6d7a14 100644 --- a/deploy/kubernetes/operator/pkg/controller/util/util_test.go +++ b/deploy/kubernetes/operator/pkg/controller/util/util_test.go @@ -19,6 +19,7 @@ package util import ( "sort" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -179,6 +180,56 @@ func TestAddOwnerReference(t *testing.T) { } } +func TestAddVolumeMountsOfMainContainer(t *testing.T) { + for _, tt := range []struct { + name string + mainContainer *corev1.Container + hostPathMounts map[string]string + volumeMounts []corev1.VolumeMount + expectedCommand string + }{ + { + name: "check add volume mount 1", + mainContainer: &corev1.Container{}, + hostPathMounts: map[string]string{ + "/rssdata1/data1": "/data1", + "/rssdata2/data2": "/data2", + "/rssdata3/data2": "/data3", + }, + volumeMounts: []corev1.VolumeMount{}, + expectedCommand: "rm -rf /data1/rssdata/*;rm -rf /data2/rssdata/*;rm -rf /data3/rssdata/*", + }, + { + name: "check add volume mount 2", + mainContainer: &corev1.Container{}, + hostPathMounts: map[string]string{ + "/rssdata4/data4": "/data4", + "/rssdata5/data5": "/data5", + "/rssdata6/data6": "/data6", + }, + volumeMounts: []corev1.VolumeMount{ + { + Name: "/rsslog", + MountPath: "/rsslog", + }, + }, + expectedCommand: "rm -rf /data4/rssdata/*;rm -rf /data5/rssdata/*;rm -rf /data6/rssdata/*", + }, + } { + t.Run(tt.name, func(t *testing.T) { + assertion := assert.New(t) + expectedVolumeMountCount := len(tt.hostPathMounts) + len(tt.volumeMounts) + assertion.Equal(0, len(tt.mainContainer.VolumeMounts)) + addVolumeMountsOfMainContainer(tt.mainContainer, tt.hostPathMounts, tt.volumeMounts) + assertion.Equal(expectedVolumeMountCount, len(tt.mainContainer.VolumeMounts)) + strSlice := strings.Split(tt.mainContainer.Lifecycle.PreStop.Exec.Command[2], ";") + sort.Strings(strSlice) + sortedString := strings.Join(strSlice, ";") + assertion.Equal(tt.expectedCommand, sortedString) + }) + } +} + func buildRssWithUID() *uniffleapi.RemoteShuffleService { rss := utils.BuildRSSWithDefaultValue() rss.UID = "uid-test"
