diff --git a/CHANGELOG.md b/CHANGELOG.md index b63d3c0..19bb855 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +# Unreleased +[Documentation](https://github.com/awslabs/mountpoint-s3-csi-driver/blob/main/README.md) + +### Notable changes +* Add `HostToContainer` mount propagation, replacing the previous method of reading mount points via `/host/proc/mounts`. ([#321](https://github.com/awslabs/mountpoint-s3-csi-driver/pull/321)) + # v1.11.0 [Documentation](https://github.com/awslabs/mountpoint-s3-csi-driver/blob/v1.11.0/README.md) diff --git a/charts/aws-mountpoint-s3-csi-driver/templates/node.yaml b/charts/aws-mountpoint-s3-csi-driver/templates/node.yaml index 6ca7a23..7ec5914 100644 --- a/charts/aws-mountpoint-s3-csi-driver/templates/node.yaml +++ b/charts/aws-mountpoint-s3-csi-driver/templates/node.yaml @@ -93,6 +93,8 @@ spec: # mount-s3 runs in systemd context, so this is relative to the host - name: MOUNT_S3_PATH value: {{ default "/opt/mountpoint-s3-csi/bin/" .Values.node.mountpointInstallPath }}mount-s3 + - name: KUBELET_PATH + value: {{ .Values.node.kubeletPath }} - name: CSI_NODE_NAME valueFrom: fieldRef: @@ -122,14 +124,17 @@ spec: volumeMounts: - name: kubelet-dir mountPath: {{ .Values.node.kubeletPath }} + # Currently we spawn Mountpoint instances on the host using systemd, + # "HostToContainer" allows any newly created mounts inside kubelet path to propagated to the container. + # Thanks to this, we can do "is mount point?" checks for volumes provided by the CSI Driver + # without needing to mount "/proc/mounts" from host. + mountPropagation: HostToContainer - name: plugin-dir mountPath: /csi - name: systemd-bus mountPath: /run/systemd/private - name: host-dev mountPath: /host/dev - - name: proc-mounts - mountPath: /host/proc/mounts ports: - name: healthz containerPort: 9808 @@ -206,10 +211,6 @@ spec: hostPath: path: {{ default "/opt/mountpoint-s3-csi/bin/" .Values.node.mountpointInstallPath }} type: DirectoryOrCreate - - name: proc-mounts - hostPath: - path: {{ default "/proc/mounts" .Values.node.procMountPath }} - type: File - name: systemd-bus hostPath: path: /run/systemd/private diff --git a/charts/aws-mountpoint-s3-csi-driver/values.yaml b/charts/aws-mountpoint-s3-csi-driver/values.yaml index 1a8d894..5c53b1a 100644 --- a/charts/aws-mountpoint-s3-csi-driver/values.yaml +++ b/charts/aws-mountpoint-s3-csi-driver/values.yaml @@ -14,7 +14,6 @@ image: node: kubeletPath: /var/lib/kubelet mountpointInstallPath: /opt/mountpoint-s3-csi/bin/ # should end with "/" - procMountPath: /proc/mounts logLevel: 4 seLinuxOptions: user: system_u diff --git a/deploy/kubernetes/base/node-daemonset.yaml b/deploy/kubernetes/base/node-daemonset.yaml index d3f9fb1..5c0e82b 100644 --- a/deploy/kubernetes/base/node-daemonset.yaml +++ b/deploy/kubernetes/base/node-daemonset.yaml @@ -107,14 +107,17 @@ spec: volumeMounts: - name: kubelet-dir mountPath: /var/lib/kubelet + # Currently we spawn Mountpoint instances on the host using systemd, + # "HostToContainer" allows any newly created mounts inside kubelet path to propagated to the container. + # Thanks to this, we can do "is mount point?" checks for volumes provided by the CSI Driver + # without needing to mount "/proc/mounts" from host. + mountPropagation: HostToContainer - name: plugin-dir mountPath: /csi - name: systemd-bus mountPath: /run/systemd/private - name: host-dev mountPath: /host/dev - - name: proc-mounts - mountPath: /host/proc/mounts ports: - name: healthz containerPort: 9810 @@ -198,10 +201,6 @@ spec: hostPath: path: /opt/mountpoint-s3-csi/bin/ type: DirectoryOrCreate - - name: proc-mounts - hostPath: - path: /proc/mounts - type: File - name: systemd-bus hostPath: path: /run/systemd/private diff --git a/pkg/driver/node/mounter/lister.go b/pkg/driver/node/mounter/lister.go deleted file mode 100644 index 82ead6f..0000000 --- a/pkg/driver/node/mounter/lister.go +++ /dev/null @@ -1,83 +0,0 @@ -package mounter - -import ( - "bufio" - "bytes" - "fmt" - "os" - "strings" - "time" - - "k8s.io/klog/v2" - "k8s.io/mount-utils" -) - -const ( - // Due to some reason that we haven't been able to identify, reading `/host/proc/mounts` - // fails on newly spawned Karpenter/GPU nodes with "invalid argument". - // It's reported that reading `/host/proc/mounts` works after some retries, - // and we decided to add retry mechanism until we find and fix the root cause of this problem. - // See https://github.com/awslabs/mountpoint-s3-csi-driver/issues/174. - procMountsReadMaxRetry = 3 - procMountsReadRetryBackoff = 100 * time.Millisecond -) - -type ProcMountLister struct { - ProcMountPath string -} - -func (pml *ProcMountLister) ListMounts() ([]mount.MountPoint, error) { - var ( - mounts []byte - err error - ) - - for i := 1; i <= procMountsReadMaxRetry; i += 1 { - mounts, err = os.ReadFile(pml.ProcMountPath) - if err == nil { - if i > 1 { - klog.V(4).Infof("Successfully read %s after %d retries", pml.ProcMountPath, i) - } - break - } - - klog.Errorf("Failed to read %s on try %d: %v", pml.ProcMountPath, i, err) - time.Sleep(procMountsReadRetryBackoff) - } - - if err != nil { - return nil, fmt.Errorf("Failed to read %s after %d tries: %w", pml.ProcMountPath, procMountsReadMaxRetry, err) - } - - return parseProcMounts(mounts) -} - -func parseProcMounts(data []byte) ([]mount.MountPoint, error) { - var mounts []mount.MountPoint - - scanner := bufio.NewScanner(bytes.NewReader(data)) - for scanner.Scan() { - line := scanner.Text() - fields := strings.Fields(line) - if len(fields) < 6 { - return nil, fmt.Errorf("Invalid line in mounts file: %s", line) - } - - mountPoint := mount.MountPoint{ - Device: fields[0], - Path: fields[1], - Type: fields[2], - Opts: strings.Split(fields[3], ","), - } - - // Fields 4 and 5 are Freq and Pass respectively. Ignoring - - mounts = append(mounts, mountPoint) - } - - if err := scanner.Err(); err != nil { - return nil, fmt.Errorf("Error reading mounts data: %w", err) - } - - return mounts, nil -} diff --git a/pkg/driver/node/mounter/mocks/mock_mount.go b/pkg/driver/node/mounter/mocks/mock_mount.go index 1189972..6d6053c 100644 --- a/pkg/driver/node/mounter/mocks/mock_mount.go +++ b/pkg/driver/node/mounter/mocks/mock_mount.go @@ -11,47 +11,8 @@ import ( mounter "github.com/awslabs/aws-s3-csi-driver/pkg/driver/node/mounter" system "github.com/awslabs/aws-s3-csi-driver/pkg/system" gomock "github.com/golang/mock/gomock" - mount "k8s.io/mount-utils" ) -// MockMountLister is a mock of MountLister interface. -type MockMountLister struct { - ctrl *gomock.Controller - recorder *MockMountListerMockRecorder -} - -// MockMountListerMockRecorder is the mock recorder for MockMountLister. -type MockMountListerMockRecorder struct { - mock *MockMountLister -} - -// NewMockMountLister creates a new mock instance. -func NewMockMountLister(ctrl *gomock.Controller) *MockMountLister { - mock := &MockMountLister{ctrl: ctrl} - mock.recorder = &MockMountListerMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockMountLister) EXPECT() *MockMountListerMockRecorder { - return m.recorder -} - -// ListMounts mocks base method. -func (m *MockMountLister) ListMounts() ([]mount.MountPoint, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ListMounts") - ret0, _ := ret[0].([]mount.MountPoint) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// ListMounts indicates an expected call of ListMounts. -func (mr *MockMountListerMockRecorder) ListMounts() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListMounts", reflect.TypeOf((*MockMountLister)(nil).ListMounts)) -} - // MockServiceRunner is a mock of ServiceRunner interface. type MockServiceRunner struct { ctrl *gomock.Controller diff --git a/pkg/driver/node/mounter/mount_credentials.go b/pkg/driver/node/mounter/mount_credentials.go index 5d45f3e..92cdf01 100644 --- a/pkg/driver/node/mounter/mount_credentials.go +++ b/pkg/driver/node/mounter/mount_credentials.go @@ -19,7 +19,6 @@ const ( awsMaxAttemptsEnv = "AWS_MAX_ATTEMPTS" MountpointCacheKey = "UNSTABLE_MOUNTPOINT_CACHE_KEY" defaultMountS3Path = "/usr/bin/mount-s3" - procMounts = "/host/proc/mounts" userAgentPrefix = "--user-agent-prefix" awsMaxAttemptsOption = "--aws-max-attempts" ) diff --git a/pkg/driver/node/mounter/mounter.go b/pkg/driver/node/mounter/mounter.go index ab15388..ece7b7c 100644 --- a/pkg/driver/node/mounter/mounter.go +++ b/pkg/driver/node/mounter/mounter.go @@ -6,13 +6,8 @@ import ( "os" "github.com/awslabs/aws-s3-csi-driver/pkg/system" - "k8s.io/mount-utils" ) -type MountLister interface { - ListMounts() ([]mount.MountPoint, error) -} - type ServiceRunner interface { StartService(ctx context.Context, config *system.ExecConfig) (string, error) RunOneshot(ctx context.Context, config *system.ExecConfig) (string, error) diff --git a/pkg/driver/node/mounter/systemd_mounter.go b/pkg/driver/node/mounter/systemd_mounter.go index cdb5a95..e6c0db6 100644 --- a/pkg/driver/node/mounter/systemd_mounter.go +++ b/pkg/driver/node/mounter/systemd_mounter.go @@ -21,7 +21,7 @@ const mountpointDeviceName = "mountpoint-s3" type SystemdMounter struct { Ctx context.Context Runner ServiceRunner - MountLister MountLister + Mounter mount.Interface MpVersion string MountS3Path string kubernetesVersion string @@ -36,7 +36,7 @@ func NewSystemdMounter(mpVersion string, kubernetesVersion string) (*SystemdMoun return &SystemdMounter{ Ctx: ctx, Runner: runner, - MountLister: &ProcMountLister{ProcMountPath: procMounts}, + Mounter: mount.New(""), MpVersion: mpVersion, MountS3Path: MountS3Path(), kubernetesVersion: kubernetesVersion, @@ -44,12 +44,15 @@ func NewSystemdMounter(mpVersion string, kubernetesVersion string) (*SystemdMoun } // IsMountPoint returns whether given `target` is a `mount-s3` mount. +// We implement the IsMountPoint interface instead of using Kubernetes' implementation +// because we need to verify not only that the target is a mount point but also that it is specifically a mount-s3 mount point. +// This is achieved by calling the mounter.List() method to enumerate all mount points. func (m *SystemdMounter) IsMountPoint(target string) (bool, error) { if _, err := os.Stat(target); os.IsNotExist(err) { return false, err } - mountPoints, err := m.MountLister.ListMounts() + mountPoints, err := m.Mounter.List() if err != nil { return false, fmt.Errorf("Failed to list mounts: %w", err) } @@ -114,15 +117,14 @@ func (m *SystemdMounter) Mount(bucketName string, target string, credentials *Mo } } - mounts, err := m.MountLister.ListMounts() + isMountPoint, err := m.IsMountPoint(target) if err != nil { return fmt.Errorf("Could not check if %q is a mount point: %v, %v", target, statErr, err) } - for _, m := range mounts { - if m.Path == target { - klog.V(4).Infof("NodePublishVolume: Target path %q is already mounted", target) - return nil - } + + if isMountPoint { + klog.V(4).Infof("NodePublishVolume: Target path %q is already mounted", target) + return nil } env := []string{} diff --git a/pkg/driver/node/mounter/systemd_mounter_test.go b/pkg/driver/node/mounter/systemd_mounter_test.go index f11d9ab..86c8968 100644 --- a/pkg/driver/node/mounter/systemd_mounter_test.go +++ b/pkg/driver/node/mounter/systemd_mounter_test.go @@ -3,7 +3,6 @@ package mounter_test import ( "context" "errors" - "fmt" "os" "path/filepath" "reflect" @@ -19,21 +18,11 @@ import ( "k8s.io/mount-utils" ) -type TestMountLister struct { - Mounts []mount.MountPoint - Err error -} - -func (l *TestMountLister) ListMounts() ([]mount.MountPoint, error) { - return l.Mounts, l.Err -} - type mounterTestEnv struct { - ctx context.Context - mockCtl *gomock.Controller - mockRunner *mock_driver.MockServiceRunner - mockMountLister *mock_driver.MockMountLister - mounter *mounter.SystemdMounter + ctx context.Context + mockCtl *gomock.Controller + mockRunner *mock_driver.MockServiceRunner + mounter *mounter.SystemdMounter } func initMounterTestEnv(t *testing.T) *mounterTestEnv { @@ -41,18 +30,16 @@ func initMounterTestEnv(t *testing.T) *mounterTestEnv { mockCtl := gomock.NewController(t) defer mockCtl.Finish() mockRunner := mock_driver.NewMockServiceRunner(mockCtl) - mockMountLister := mock_driver.NewMockMountLister(mockCtl) mountpointVersion := "TEST_MP_VERSION-v1.1" return &mounterTestEnv{ - ctx: ctx, - mockCtl: mockCtl, - mockRunner: mockRunner, - mockMountLister: mockMountLister, + ctx: ctx, + mockCtl: mockCtl, + mockRunner: mockRunner, mounter: &mounter.SystemdMounter{ Ctx: ctx, Runner: mockRunner, - MountLister: mockMountLister, + Mounter: mount.NewFakeMounter(nil), MpVersion: mountpointVersion, MountS3Path: mounter.MountS3Path(), }, @@ -88,7 +75,6 @@ func TestS3MounterMount(t *testing.T) { credentials: testCredentials, options: []string{}, before: func(t *testing.T, env *mounterTestEnv) { - env.mockMountLister.EXPECT().ListMounts().Return(nil, nil) env.mockRunner.EXPECT().StartService(gomock.Any(), gomock.Any()).Return("success", nil) }, }, @@ -99,7 +85,6 @@ func TestS3MounterMount(t *testing.T) { credentials: nil, options: []string{}, before: func(t *testing.T, env *mounterTestEnv) { - env.mockMountLister.EXPECT().ListMounts().Return(nil, nil) env.mockRunner.EXPECT().StartService(gomock.Any(), gomock.Any()).Return("success", nil) }, }, @@ -110,7 +95,6 @@ func TestS3MounterMount(t *testing.T) { credentials: nil, options: []string{"--user-agent-prefix=mycustomuseragent"}, before: func(t *testing.T, env *mounterTestEnv) { - env.mockMountLister.EXPECT().ListMounts().Return(nil, nil) env.mockRunner.EXPECT().StartService(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, config *system.ExecConfig) (string, error) { for _, a := range config.Args { if strings.Contains(a, "mycustomuseragent") { @@ -128,7 +112,6 @@ func TestS3MounterMount(t *testing.T) { credentials: nil, options: []string{"--aws-max-attempts=10"}, before: func(t *testing.T, env *mounterTestEnv) { - env.mockMountLister.EXPECT().ListMounts().Return(nil, nil) env.mockRunner.EXPECT().StartService(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, config *system.ExecConfig) (string, error) { for _, e := range config.Env { if e == "AWS_MAX_ATTEMPTS=10" { @@ -148,7 +131,6 @@ func TestS3MounterMount(t *testing.T) { options: []string{}, expectedErr: true, before: func(t *testing.T, env *mounterTestEnv) { - env.mockMountLister.EXPECT().ListMounts().Return(nil, nil) env.mockRunner.EXPECT().StartService(gomock.Any(), gomock.Any()).Return("fail", errors.New("test failure")) }, }, @@ -335,19 +317,46 @@ func TestIsMountPoint(t *testing.T) { testDir := t.TempDir() mountpointS3MountPath := filepath.Join(testDir, "/var/lib/kubelet/pods/46efe8aa-75d9-4b12-8fdd-0ce0c2cabd99/volumes/kubernetes.io~csi/s3-mp-csi-pv/mount") tmpFsMountPath := filepath.Join(testDir, "/var/lib/kubelet/pods/3af4cdb5-6131-4d4b-bed3-4b7a74d357e4/volumes/kubernetes.io~projected/kube-api-access-tmxk4") - testProcMountsContent := []byte( - fmt.Sprintf(`proc /proc proc rw,nosuid,nodev,noexec,relatime 0 0 -sysfs /sys sysfs rw,seclabel,nosuid,nodev,noexec,relatime 0 0 -tmpfs %s tmpfs rw,seclabel,relatime,size=3364584k 0 0 -mountpoint-s3 %s fuse rw,nosuid,nodev,noatime,user_id=0,group_id=0,default_permissions 0 0`, - tmpFsMountPath, - mountpointS3MountPath), - ) + testProcMountsContent := []mount.MountPoint{ + { + Device: "proc", + Path: "/proc", + Type: "proc", + Opts: []string{"rw", "nosuid", "nodev", "noexec", "relatime"}, + Freq: 0, + Pass: 0, + }, + { + Device: "sysfs", + Path: "/sys", + Type: "sysfs", + Opts: []string{"rw", "seclabel", "nosuid", "nodev", "noexec", "relatime"}, + Freq: 0, + Pass: 0, + }, + { + Device: "tmpfs", + Path: tmpFsMountPath, + Type: "tmpfs", + Opts: []string{"rw", "seclabel", "relatime", "size=3364584k"}, + Freq: 0, + Pass: 0, + }, + { + Device: "mountpoint-s3", + Path: mountpointS3MountPath, + Type: "fuse", + Opts: []string{"rw", "nosuid", "nodev", "noatime", "user_id=0", "group_id=0", "default_permissions"}, + Freq: 0, + Pass: 0, + }, + } + os.MkdirAll(tmpFsMountPath, 0755) os.MkdirAll(mountpointS3MountPath, 0755) tests := map[string]struct { - procMountsContent []byte + procMountsContent []mount.MountPoint target string isMountPoint bool expectErr bool @@ -365,11 +374,10 @@ mountpoint-s3 %s fuse rw,nosuid,nodev,noatime,user_id=0,group_id=0,default_permi expectErr: false, }, "non existing mount on /proc/mounts": { - procMountsContent: []byte(`proc /proc proc rw,nosuid,nodev,noexec,relatime 0 0 -sysfs /sys sysfs rw,seclabel,nosuid,nodev,noexec,relatime 0 0`), - target: mountpointS3MountPath, - isMountPoint: false, - expectErr: false, + procMountsContent: testProcMountsContent[:2], + target: mountpointS3MountPath, + isMountPoint: false, + expectErr: false, }, "non existing mount on filesystem": { procMountsContent: testProcMountsContent, @@ -381,22 +389,10 @@ sysfs /sys sysfs rw,seclabel,nosuid,nodev,noexec,relatime 0 0`), for name, test := range tests { t.Run(name, func(t *testing.T) { - procMountsPath := filepath.Join(t.TempDir(), "proc", "mounts") - err := os.MkdirAll(filepath.Dir(procMountsPath), 0755) - assertNoError(t, err) - err = os.WriteFile(procMountsPath, test.procMountsContent, 0755) - assertNoError(t, err) - - mounter := &mounter.SystemdMounter{MountLister: &mounter.ProcMountLister{ProcMountPath: procMountsPath}} + mounter := &mounter.SystemdMounter{Mounter: mount.NewFakeMounter(test.procMountsContent)} isMountPoint, err := mounter.IsMountPoint(test.target) assertEquals(t, test.isMountPoint, isMountPoint) assertEquals(t, test.expectErr, err != nil) }) } } - -func assertNoError(t *testing.T, err error) { - if err != nil { - t.Errorf("Expected no error, but got: %s", err) - } -} diff --git a/pkg/driver/node/node.go b/pkg/driver/node/node.go index 7a03166..881c26b 100644 --- a/pkg/driver/node/node.go +++ b/pkg/driver/node/node.go @@ -34,8 +34,11 @@ import ( const ( volumeCtxBucketName = "bucketName" + defaultKubeletPath = "/var/lib/kubelet" ) +var kubeletPath = getKubeletPath() + var ( nodeCaps = []csi.NodeServiceCapability_RPC_Type{} ) @@ -102,6 +105,10 @@ func (ns *S3NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePubl return nil, status.Error(codes.InvalidArgument, "Target path not provided") } + if !strings.HasPrefix(target, kubeletPath) { + klog.Errorf("NodePublishVolume: target path %q is not in kubelet path %q. This might cause mounting issues, please ensure you have correct kubelet path configured.", target, kubeletPath) + } + volCap := req.GetVolumeCapability() if volCap == nil { return nil, status.Error(codes.InvalidArgument, "Volume capability not provided") @@ -173,6 +180,14 @@ func compileMountOptions(currentOptions []string, newOptions []string) []string return allMountOptions.List() } +func getKubeletPath() string { + kubeletPath := os.Getenv("KUBELET_PATH") + if kubeletPath == "" { + return defaultKubeletPath + } + return kubeletPath +} + func (ns *S3NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublishVolumeRequest) (*csi.NodeUnpublishVolumeResponse, error) { klog.V(4).Infof("NodeUnpublishVolume: called with args %+v", req)