From d6bfd7daeb886ba9b4f273138319289646c57da3 Mon Sep 17 00:00:00 2001 From: "Lubomir I. Ivanov" Date: Sat, 30 Dec 2023 10:49:18 +0200 Subject: [PATCH 1/2] kubeadm: throw errors on unmount instead of warnings Instead of warnings when syscall.Unmount() causes errors, store all the errors in an aggregate. Abort the reset operation if at least one unmount error was encountered. --- .../app/cmd/phases/reset/cleanupnode.go | 20 +++++++++---------- cmd/kubeadm/app/cmd/phases/reset/unmount.go | 2 +- .../app/cmd/phases/reset/unmount_linux.go | 19 ++++++++++++------ 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go b/cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go index d78a2926d91..01bbab89f4d 100644 --- a/cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go +++ b/cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go @@ -84,11 +84,15 @@ func runCleanupNode(c workflow.RunData) error { // Try to unmount mounted directories under kubeadmconstants.KubeletRunDirectory in order to be able to remove the kubeadmconstants.KubeletRunDirectory directory later fmt.Printf("[reset] Unmounting mounted directories in %q\n", kubeadmconstants.KubeletRunDirectory) // In case KubeletRunDirectory holds a symbolic link, evaluate it - kubeletRunDir, err := absoluteKubeletRunDirectory() - if err == nil { - // Only clean absoluteKubeletRunDirectory if umountDirsCmd passed without error - dirsToClean = append(dirsToClean, kubeletRunDir) + kubeletRunDirectory, err := absoluteKubeletRunDirectory() + if err != nil { + return err } + // Unmount all mount paths under kubeletRunDirectory + if err := unmountKubeletDirectory(kubeletRunDirectory); err != nil { + return err + } + dirsToClean = append(dirsToClean, kubeletRunDirectory) } else { fmt.Printf("[reset] Would unmount mounted directories in %q\n", kubeadmconstants.KubeletRunDirectory) } @@ -131,13 +135,7 @@ func runCleanupNode(c workflow.RunData) error { func absoluteKubeletRunDirectory() (string, error) { absoluteKubeletRunDirectory, err := filepath.EvalSymlinks(kubeadmconstants.KubeletRunDirectory) if err != nil { - klog.Warningf("[reset] Failed to evaluate the %q directory. Skipping its unmount and cleanup: %v\n", kubeadmconstants.KubeletRunDirectory, err) - return "", err - } - err = unmountKubeletDirectory(absoluteKubeletRunDirectory) - if err != nil { - klog.Warningf("[reset] Failed to unmount mounted directories in %s \n", kubeadmconstants.KubeletRunDirectory) - return "", err + return "", errors.Wrapf(err, "failed to evaluate the %q directory", kubeadmconstants.KubeletRunDirectory) } return absoluteKubeletRunDirectory, nil } diff --git a/cmd/kubeadm/app/cmd/phases/reset/unmount.go b/cmd/kubeadm/app/cmd/phases/reset/unmount.go index 1796da898b9..c6fbda56d87 100644 --- a/cmd/kubeadm/app/cmd/phases/reset/unmount.go +++ b/cmd/kubeadm/app/cmd/phases/reset/unmount.go @@ -24,7 +24,7 @@ import ( ) // unmountKubeletDirectory is a NOOP on all but linux. -func unmountKubeletDirectory(absoluteKubeletRunDirectory string) error { +func unmountKubeletDirectory(kubeletRunDirectory string) error { klog.Warning("Cannot unmount filesystems on current OS, all mounted file systems will need to be manually unmounted") return nil } diff --git a/cmd/kubeadm/app/cmd/phases/reset/unmount_linux.go b/cmd/kubeadm/app/cmd/phases/reset/unmount_linux.go index 993ab888dad..f7914b91026 100644 --- a/cmd/kubeadm/app/cmd/phases/reset/unmount_linux.go +++ b/cmd/kubeadm/app/cmd/phases/reset/unmount_linux.go @@ -24,30 +24,37 @@ import ( "strings" "syscall" + "github.com/pkg/errors" + "k8s.io/klog/v2" + + utilerrors "k8s.io/apimachinery/pkg/util/errors" ) // unmountKubeletDirectory unmounts all paths that contain KubeletRunDirectory -func unmountKubeletDirectory(absoluteKubeletRunDirectory string) error { +func unmountKubeletDirectory(kubeletRunDirectory string) error { raw, err := os.ReadFile("/proc/mounts") if err != nil { return err } - if !strings.HasSuffix(absoluteKubeletRunDirectory, "/") { + if !strings.HasSuffix(kubeletRunDirectory, "/") { // trailing "/" is needed to ensure that possibly mounted /var/lib/kubelet is skipped - absoluteKubeletRunDirectory += "/" + kubeletRunDirectory += "/" } + var errList []error mounts := strings.Split(string(raw), "\n") for _, mount := range mounts { m := strings.Split(mount, " ") - if len(m) < 2 || !strings.HasPrefix(m[1], absoluteKubeletRunDirectory) { + if len(m) < 2 || !strings.HasPrefix(m[1], kubeletRunDirectory) { continue } + klog.V(5).Infof("[reset] Unmounting %q", m[1]) if err := syscall.Unmount(m[1], 0); err != nil { - klog.Warningf("[reset] Failed to unmount mounted directory in %s: %s", absoluteKubeletRunDirectory, m[1]) + errList = append(errList, errors.WithMessagef(err, "failed to unmount %q", m[1])) } } - return nil + return errors.Wrapf(utilerrors.NewAggregate(errList), + "encountered the following errors while unmounting directories in %q", kubeletRunDirectory) } From 2f5121671fc9073c3fcb5f163359ebe8f460a402 Mon Sep 17 00:00:00 2001 From: "Lubomir I. Ivanov" Date: Sat, 30 Dec 2023 11:07:37 +0200 Subject: [PATCH 2/2] kubeadm: add ResetConfiguration.UnmountFlags Add new a v1beta4.ResetConfiguration.UnmountFlags field that can be used to pass in Linux unmount2() flags such as MNT_FORCE. Default value continues to be 0 - i.e. no flags. --- cmd/kubeadm/app/apis/kubeadm/types.go | 16 +++++ cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go | 6 ++ .../v1beta4/zz_generated.conversion.go | 2 + .../kubeadm/v1beta4/zz_generated.deepcopy.go | 5 ++ .../app/apis/kubeadm/validation/validation.go | 17 +++++ .../kubeadm/validation/validation_test.go | 39 +++++++++++ .../app/apis/kubeadm/zz_generated.deepcopy.go | 5 ++ .../app/cmd/phases/reset/cleanupnode.go | 2 +- cmd/kubeadm/app/cmd/phases/reset/unmount.go | 2 +- .../app/cmd/phases/reset/unmount_linux.go | 23 ++++++- .../cmd/phases/reset/unmount_linux_test.go | 65 +++++++++++++++++++ 11 files changed, 178 insertions(+), 4 deletions(-) create mode 100644 cmd/kubeadm/app/cmd/phases/reset/unmount_linux_test.go diff --git a/cmd/kubeadm/app/apis/kubeadm/types.go b/cmd/kubeadm/app/apis/kubeadm/types.go index daa2dcc79cf..373e1e6a0fe 100644 --- a/cmd/kubeadm/app/apis/kubeadm/types.go +++ b/cmd/kubeadm/app/apis/kubeadm/types.go @@ -522,8 +522,24 @@ type ResetConfiguration struct { // SkipPhases is a list of phases to skip during command execution. // The list of phases can be obtained with the "kubeadm reset phase --help" command. SkipPhases []string + + // UnmountFlags is a list of unmount2() syscall flags that kubeadm can use when unmounting + // directories during "reset". A flag can be one of: MNT_FORCE, MNT_DETACH, MNT_EXPIRE, UMOUNT_NOFOLLOW. + // By default this list is empty. + UnmountFlags []string } +const ( + // UnmountFlagMNTForce represents the flag "MNT_FORCE" + UnmountFlagMNTForce = "MNT_FORCE" + // UnmountFlagMNTDetach represents the flag "MNT_DETACH" + UnmountFlagMNTDetach = "MNT_DETACH" + // UnmountFlagMNTExpire represents the flag "MNT_EXPIRE" + UnmountFlagMNTExpire = "MNT_EXPIRE" + // UnmountFlagUmountNoFollow represents the flag "UMOUNT_NOFOLLOW" + UnmountFlagUmountNoFollow = "UMOUNT_NOFOLLOW" +) + // ComponentConfigMap is a map between a group name (as in GVK group) and a ComponentConfig type ComponentConfigMap map[string]ComponentConfig diff --git a/cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go b/cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go index 021d13503da..06221c297d1 100644 --- a/cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go +++ b/cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go @@ -517,6 +517,12 @@ type ResetConfiguration struct { // The list of phases can be obtained with the "kubeadm reset phase --help" command. // +optional SkipPhases []string `json:"skipPhases,omitempty"` + + // UnmountFlags is a list of unmount2() syscall flags that kubeadm can use when unmounting + // directories during "reset". A flag can be one of: MNT_FORCE, MNT_DETACH, MNT_EXPIRE, UMOUNT_NOFOLLOW. + // By default this list is empty. + // +optional + UnmountFlags []string `json:"unmountFlags,omitempty"` } // Arg represents an argument with a name and a value. diff --git a/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.conversion.go b/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.conversion.go index 277939145b0..3f78cbf801d 100644 --- a/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.conversion.go +++ b/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.conversion.go @@ -894,6 +894,7 @@ func autoConvert_v1beta4_ResetConfiguration_To_kubeadm_ResetConfiguration(in *Re out.Force = in.Force out.IgnorePreflightErrors = *(*[]string)(unsafe.Pointer(&in.IgnorePreflightErrors)) out.SkipPhases = *(*[]string)(unsafe.Pointer(&in.SkipPhases)) + out.UnmountFlags = *(*[]string)(unsafe.Pointer(&in.UnmountFlags)) return nil } @@ -910,6 +911,7 @@ func autoConvert_kubeadm_ResetConfiguration_To_v1beta4_ResetConfiguration(in *ku out.Force = in.Force out.IgnorePreflightErrors = *(*[]string)(unsafe.Pointer(&in.IgnorePreflightErrors)) out.SkipPhases = *(*[]string)(unsafe.Pointer(&in.SkipPhases)) + out.UnmountFlags = *(*[]string)(unsafe.Pointer(&in.UnmountFlags)) return nil } diff --git a/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.deepcopy.go b/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.deepcopy.go index 03033797d17..e4956cfdbde 100644 --- a/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.deepcopy.go +++ b/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.deepcopy.go @@ -577,6 +577,11 @@ func (in *ResetConfiguration) DeepCopyInto(out *ResetConfiguration) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.UnmountFlags != nil { + in, out := &in.UnmountFlags, &out.UnmountFlags + *out = make([]string, len(*in)) + copy(*out, *in) + } return } diff --git a/cmd/kubeadm/app/apis/kubeadm/validation/validation.go b/cmd/kubeadm/app/apis/kubeadm/validation/validation.go index a8e1c625f83..9923e5326a7 100644 --- a/cmd/kubeadm/app/apis/kubeadm/validation/validation.go +++ b/cmd/kubeadm/app/apis/kubeadm/validation/validation.go @@ -707,6 +707,7 @@ func ValidateResetConfiguration(c *kubeadm.ResetConfiguration) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, ValidateSocketPath(c.CRISocket, field.NewPath("criSocket"))...) allErrs = append(allErrs, ValidateAbsolutePath(c.CertificatesDir, field.NewPath("certificatesDir"))...) + allErrs = append(allErrs, ValidateUnmountFlags(c.UnmountFlags, field.NewPath("unmountFlags"))...) return allErrs } @@ -722,3 +723,19 @@ func ValidateExtraArgs(args []kubeadm.Arg, fldPath *field.Path) field.ErrorList return allErrs } + +// ValidateUnmountFlags validates a set of unmount flags and collects all encountered errors +func ValidateUnmountFlags(flags []string, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + for idx, flag := range flags { + switch flag { + case kubeadm.UnmountFlagMNTForce, kubeadm.UnmountFlagMNTDetach, kubeadm.UnmountFlagMNTExpire, kubeadm.UnmountFlagUmountNoFollow: + continue + default: + allErrs = append(allErrs, field.Invalid(fldPath, fmt.Sprintf("index %d", idx), fmt.Sprintf("unknown unmount flag %s", flag))) + } + } + + return allErrs +} diff --git a/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go b/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go index 32ffe5a2320..02b25392217 100644 --- a/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go +++ b/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go @@ -1464,3 +1464,42 @@ func TestValidateExtraArgs(t *testing.T) { } } } + +func TestValidateUnmountFlags(t *testing.T) { + var tests = []struct { + name string + flags []string + expectedErrors int + }{ + { + name: "nil input", + flags: nil, + expectedErrors: 0, + }, + { + name: "all valid flags", + flags: []string{ + kubeadmapi.UnmountFlagMNTForce, + kubeadmapi.UnmountFlagMNTDetach, + kubeadmapi.UnmountFlagMNTExpire, + kubeadmapi.UnmountFlagUmountNoFollow, + }, + expectedErrors: 0, + }, + { + name: "invalid two flags", + flags: []string{ + "foo", + "bar", + }, + expectedErrors: 2, + }, + } + + for _, tc := range tests { + actual := ValidateUnmountFlags(tc.flags, nil) + if len(actual) != tc.expectedErrors { + t.Errorf("case %q:\n\t expected errors: %v\n\t got: %v\n\t errors: %v", tc.name, tc.expectedErrors, len(actual), actual) + } + } +} diff --git a/cmd/kubeadm/app/apis/kubeadm/zz_generated.deepcopy.go b/cmd/kubeadm/app/apis/kubeadm/zz_generated.deepcopy.go index 29c2dadb6df..27f366c1b01 100644 --- a/cmd/kubeadm/app/apis/kubeadm/zz_generated.deepcopy.go +++ b/cmd/kubeadm/app/apis/kubeadm/zz_generated.deepcopy.go @@ -607,6 +607,11 @@ func (in *ResetConfiguration) DeepCopyInto(out *ResetConfiguration) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.UnmountFlags != nil { + in, out := &in.UnmountFlags, &out.UnmountFlags + *out = make([]string, len(*in)) + copy(*out, *in) + } return } diff --git a/cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go b/cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go index 01bbab89f4d..4979186dea4 100644 --- a/cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go +++ b/cmd/kubeadm/app/cmd/phases/reset/cleanupnode.go @@ -89,7 +89,7 @@ func runCleanupNode(c workflow.RunData) error { return err } // Unmount all mount paths under kubeletRunDirectory - if err := unmountKubeletDirectory(kubeletRunDirectory); err != nil { + if err := unmountKubeletDirectory(kubeletRunDirectory, r.ResetCfg().UnmountFlags); err != nil { return err } dirsToClean = append(dirsToClean, kubeletRunDirectory) diff --git a/cmd/kubeadm/app/cmd/phases/reset/unmount.go b/cmd/kubeadm/app/cmd/phases/reset/unmount.go index c6fbda56d87..42e8c543762 100644 --- a/cmd/kubeadm/app/cmd/phases/reset/unmount.go +++ b/cmd/kubeadm/app/cmd/phases/reset/unmount.go @@ -24,7 +24,7 @@ import ( ) // unmountKubeletDirectory is a NOOP on all but linux. -func unmountKubeletDirectory(kubeletRunDirectory string) error { +func unmountKubeletDirectory(kubeletRunDirectory string, flags []string) error { klog.Warning("Cannot unmount filesystems on current OS, all mounted file systems will need to be manually unmounted") return nil } diff --git a/cmd/kubeadm/app/cmd/phases/reset/unmount_linux.go b/cmd/kubeadm/app/cmd/phases/reset/unmount_linux.go index f7914b91026..6a24ef6716b 100644 --- a/cmd/kubeadm/app/cmd/phases/reset/unmount_linux.go +++ b/cmd/kubeadm/app/cmd/phases/reset/unmount_linux.go @@ -25,14 +25,32 @@ import ( "syscall" "github.com/pkg/errors" + "golang.org/x/sys/unix" "k8s.io/klog/v2" utilerrors "k8s.io/apimachinery/pkg/util/errors" + + kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm" ) +var flagMap = map[string]int{ + kubeadmapi.UnmountFlagMNTForce: unix.MNT_FORCE, + kubeadmapi.UnmountFlagMNTDetach: unix.MNT_DETACH, + kubeadmapi.UnmountFlagMNTExpire: unix.MNT_EXPIRE, + kubeadmapi.UnmountFlagUmountNoFollow: unix.UMOUNT_NOFOLLOW, +} + +func flagsToInt(flags []string) int { + res := 0 + for _, f := range flags { + res |= flagMap[f] + } + return res +} + // unmountKubeletDirectory unmounts all paths that contain KubeletRunDirectory -func unmountKubeletDirectory(kubeletRunDirectory string) error { +func unmountKubeletDirectory(kubeletRunDirectory string, flags []string) error { raw, err := os.ReadFile("/proc/mounts") if err != nil { return err @@ -45,13 +63,14 @@ func unmountKubeletDirectory(kubeletRunDirectory string) error { var errList []error mounts := strings.Split(string(raw), "\n") + flagsInt := flagsToInt(flags) for _, mount := range mounts { m := strings.Split(mount, " ") if len(m) < 2 || !strings.HasPrefix(m[1], kubeletRunDirectory) { continue } klog.V(5).Infof("[reset] Unmounting %q", m[1]) - if err := syscall.Unmount(m[1], 0); err != nil { + if err := syscall.Unmount(m[1], flagsInt); err != nil { errList = append(errList, errors.WithMessagef(err, "failed to unmount %q", m[1])) } } diff --git a/cmd/kubeadm/app/cmd/phases/reset/unmount_linux_test.go b/cmd/kubeadm/app/cmd/phases/reset/unmount_linux_test.go new file mode 100644 index 00000000000..f42dff5118b --- /dev/null +++ b/cmd/kubeadm/app/cmd/phases/reset/unmount_linux_test.go @@ -0,0 +1,65 @@ +//go:build linux +// +build linux + +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package phases + +import ( + "testing" + + kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm" +) + +func TestFlagsToInt(t *testing.T) { + + tests := []struct { + name string + input []string + expectedOutput int + }{ + { + name: "nil input", + input: nil, + expectedOutput: 0, + }, + { + name: "no flags", + input: []string{}, + expectedOutput: 0, + }, + { + name: "all flags", + input: []string{ + kubeadmapi.UnmountFlagMNTForce, + kubeadmapi.UnmountFlagMNTDetach, + kubeadmapi.UnmountFlagMNTExpire, + kubeadmapi.UnmountFlagUmountNoFollow, + }, + expectedOutput: 15, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + out := flagsToInt(tc.input) + if tc.expectedOutput != out { + t.Errorf("expected output %d, got %d", tc.expectedOutput, out) + } + }) + } +}