Skip to content

Commit

Permalink
[#1290] improvement(operator): Avoid accidentally deleting data of ot…
Browse files Browse the repository at this point in the history
…her 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 be10697)
  • Loading branch information
qijiale76 authored and xianjingfeng committed Nov 20, 2023
1 parent 42b5c21 commit cf25897
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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, ",")
Expand Down
3 changes: 2 additions & 1 deletion deploy/kubernetes/operator/pkg/controller/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
51 changes: 51 additions & 0 deletions deploy/kubernetes/operator/pkg/controller/util/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package util

import (
"sort"
"strings"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -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"
Expand Down

0 comments on commit cf25897

Please sign in to comment.