From 6ed91fc07ca75591691d3aacc30dc8d56f6cbf62 Mon Sep 17 00:00:00 2001 From: liz Date: Tue, 29 May 2018 17:04:39 -0400 Subject: [PATCH] Save kubeadm manifest backup directories When kubeadm upgrades a static pod cluster, the old manifests were previously deleted. This patch alters this behaviour so they are now stored in a timestamped temporary directory. --- cmd/kubeadm/app/cmd/upgrade/BUILD | 1 + cmd/kubeadm/app/cmd/upgrade/apply.go | 10 +- cmd/kubeadm/app/cmd/upgrade/apply_test.go | 92 +++++++++++++++++++ cmd/kubeadm/app/constants/constants.go | 28 +++++- cmd/kubeadm/app/phases/upgrade/staticpods.go | 43 +++++++-- .../app/phases/upgrade/staticpods_test.go | 91 ++++++++++++++++++ cmd/kubeadm/app/util/etcd/BUILD | 6 +- cmd/kubeadm/app/util/etcd/etcd.go | 6 ++ cmd/kubeadm/app/util/etcd/etcd_test.go | 48 ++++++++++ 9 files changed, 309 insertions(+), 16 deletions(-) diff --git a/cmd/kubeadm/app/cmd/upgrade/BUILD b/cmd/kubeadm/app/cmd/upgrade/BUILD index 9a74d83aa07..4018c1c876b 100644 --- a/cmd/kubeadm/app/cmd/upgrade/BUILD +++ b/cmd/kubeadm/app/cmd/upgrade/BUILD @@ -53,6 +53,7 @@ go_test( embed = [":go_default_library"], deps = [ "//cmd/kubeadm/app/apis/kubeadm:go_default_library", + "//cmd/kubeadm/app/constants:go_default_library", "//cmd/kubeadm/app/phases/upgrade:go_default_library", ], ) diff --git a/cmd/kubeadm/app/cmd/upgrade/apply.go b/cmd/kubeadm/app/cmd/upgrade/apply.go index 1acb2f895b1..94f00b1771d 100644 --- a/cmd/kubeadm/app/cmd/upgrade/apply.go +++ b/cmd/kubeadm/app/cmd/upgrade/apply.go @@ -37,6 +37,7 @@ import ( "k8s.io/kubernetes/cmd/kubeadm/app/util/apiclient" configutil "k8s.io/kubernetes/cmd/kubeadm/app/util/config" dryrunutil "k8s.io/kubernetes/cmd/kubeadm/app/util/dryrun" + etcdutil "k8s.io/kubernetes/cmd/kubeadm/app/util/etcd" "k8s.io/kubernetes/pkg/util/version" ) @@ -266,12 +267,19 @@ func PerformControlPlaneUpgrade(flags *applyFlags, client clientset.Interface, w return DryRunStaticPodUpgrade(internalcfg) } + // Don't save etcd backup directory if etcd is HA, as this could cause corruption return PerformStaticPodUpgrade(client, waiter, internalcfg, flags.etcdUpgrade) } +// GetPathManagerForUpgrade returns a path manager properly configured for the given MasterConfiguration. +func GetPathManagerForUpgrade(internalcfg *kubeadmapi.MasterConfiguration, etcdUpgrade bool) (upgrade.StaticPodPathManager, error) { + isHAEtcd := etcdutil.CheckConfigurationIsHA(&internalcfg.Etcd) + return upgrade.NewKubeStaticPodPathManagerUsingTempDirs(constants.GetStaticPodDirectory(), true, etcdUpgrade && !isHAEtcd) +} + // PerformStaticPodUpgrade performs the upgrade of the control plane components for a static pod hosted cluster func PerformStaticPodUpgrade(client clientset.Interface, waiter apiclient.Waiter, internalcfg *kubeadmapi.MasterConfiguration, etcdUpgrade bool) error { - pathManager, err := upgrade.NewKubeStaticPodPathManagerUsingTempDirs(constants.GetStaticPodDirectory()) + pathManager, err := GetPathManagerForUpgrade(internalcfg, etcdUpgrade) if err != nil { return err } diff --git a/cmd/kubeadm/app/cmd/upgrade/apply_test.go b/cmd/kubeadm/app/cmd/upgrade/apply_test.go index 24d1778f8be..a7cd2394b5f 100644 --- a/cmd/kubeadm/app/cmd/upgrade/apply_test.go +++ b/cmd/kubeadm/app/cmd/upgrade/apply_test.go @@ -17,8 +17,13 @@ limitations under the License. package upgrade import ( + "io/ioutil" + "os" "reflect" "testing" + + kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm" + "k8s.io/kubernetes/cmd/kubeadm/app/constants" ) func TestSetImplicitFlags(t *testing.T) { @@ -145,3 +150,90 @@ func TestSetImplicitFlags(t *testing.T) { } } } + +func TestGetPathManagerForUpgrade(t *testing.T) { + + haEtcd := &kubeadmapi.MasterConfiguration{ + Etcd: kubeadmapi.Etcd{ + External: &kubeadmapi.ExternalEtcd{ + Endpoints: []string{"10.100.0.1:2379", "10.100.0.2:2379", "10.100.0.3:2379"}, + }, + }, + } + + noHAEtcd := &kubeadmapi.MasterConfiguration{} + + tests := []struct { + name string + cfg *kubeadmapi.MasterConfiguration + etcdUpgrade bool + shouldDeleteEtcd bool + }{ + { + name: "ha etcd but no etcd upgrade", + cfg: haEtcd, + etcdUpgrade: false, + shouldDeleteEtcd: true, + }, + { + name: "non-ha etcd with etcd upgrade", + cfg: noHAEtcd, + etcdUpgrade: true, + shouldDeleteEtcd: false, + }, + { + name: "ha etcd and etcd upgrade", + cfg: haEtcd, + etcdUpgrade: true, + shouldDeleteEtcd: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Use a temporary directory + tmpdir, err := ioutil.TempDir("", "TestGetPathManagerForUpgrade") + if err != nil { + t.Fatalf("unexpected error making temporary directory: %v", err) + } + oldK8sDir := constants.KubernetesDir + constants.KubernetesDir = tmpdir + defer func() { + constants.KubernetesDir = oldK8sDir + os.RemoveAll(tmpdir) + }() + + pathmgr, err := GetPathManagerForUpgrade(test.cfg, test.etcdUpgrade) + if err != nil { + t.Fatalf("unexpected error creating path manager: %v", err) + } + + if _, err := os.Stat(pathmgr.BackupManifestDir()); os.IsNotExist(err) { + t.Errorf("expected manifest dir %s to exist, but it did not (%v)", pathmgr.BackupManifestDir(), err) + } + + if _, err := os.Stat(pathmgr.BackupEtcdDir()); os.IsNotExist(err) { + t.Errorf("expected etcd dir %s to exist, but it did not (%v)", pathmgr.BackupEtcdDir(), err) + } + + if err := pathmgr.CleanupDirs(); err != nil { + t.Fatalf("unexpected error cleaning up directories: %v", err) + } + + if _, err := os.Stat(pathmgr.BackupManifestDir()); os.IsNotExist(err) { + t.Errorf("expected manifest dir %s to exist, but it did not (%v)", pathmgr.BackupManifestDir(), err) + } + + if test.shouldDeleteEtcd { + if _, err := os.Stat(pathmgr.BackupEtcdDir()); !os.IsNotExist(err) { + t.Errorf("expected etcd dir %s not to exist, but it did (%v)", pathmgr.BackupEtcdDir(), err) + } + } else { + if _, err := os.Stat(pathmgr.BackupEtcdDir()); os.IsNotExist(err) { + t.Errorf("expected etcd dir %s to exist, but it did not", pathmgr.BackupEtcdDir()) + } + } + }) + } + +} diff --git a/cmd/kubeadm/app/constants/constants.go b/cmd/kubeadm/app/constants/constants.go index 3a6f4233f4f..544f3a3f7cd 100644 --- a/cmd/kubeadm/app/constants/constants.go +++ b/cmd/kubeadm/app/constants/constants.go @@ -21,6 +21,7 @@ import ( "io/ioutil" "net" "os" + "path" "path/filepath" "time" @@ -38,7 +39,8 @@ const ( // ManifestsSubDirName defines directory name to store manifests ManifestsSubDirName = "manifests" // TempDirForKubeadm defines temporary directory for kubeadm - TempDirForKubeadm = "/etc/kubernetes/tmp" + // should be joined with KubernetesDir. + TempDirForKubeadm = "tmp" // CACertAndKeyBaseName defines certificate authority base name CACertAndKeyBaseName = "ca" @@ -343,18 +345,36 @@ func AddSelfHostedPrefix(componentName string) string { // CreateTempDirForKubeadm is a function that creates a temporary directory under /etc/kubernetes/tmp (not using /tmp as that would potentially be dangerous) func CreateTempDirForKubeadm(dirName string) (string, error) { + tempDir := path.Join(KubernetesDir, TempDirForKubeadm) // creates target folder if not already exists - if err := os.MkdirAll(TempDirForKubeadm, 0700); err != nil { - return "", fmt.Errorf("failed to create directory %q: %v", TempDirForKubeadm, err) + if err := os.MkdirAll(tempDir, 0700); err != nil { + return "", fmt.Errorf("failed to create directory %q: %v", tempDir, err) } - tempDir, err := ioutil.TempDir(TempDirForKubeadm, dirName) + tempDir, err := ioutil.TempDir(tempDir, dirName) if err != nil { return "", fmt.Errorf("couldn't create a temporary directory: %v", err) } return tempDir, nil } +// CreateTimestampDirForKubeadm is a function that creates a temporary directory under /etc/kubernetes/tmp formatted with the current date +func CreateTimestampDirForKubeadm(dirName string) (string, error) { + tempDir := path.Join(KubernetesDir, TempDirForKubeadm) + // creates target folder if not already exists + if err := os.MkdirAll(tempDir, 0700); err != nil { + return "", fmt.Errorf("failed to create directory %q: %v", tempDir, err) + } + + timestampDirName := fmt.Sprintf("%s-%s", dirName, time.Now().Format("2006-01-02-15-04-05")) + timestampDir := path.Join(tempDir, timestampDirName) + if err := os.Mkdir(timestampDir, 0700); err != nil { + return "", fmt.Errorf("could not create timestamp directory: %v", err) + } + + return timestampDir, nil +} + // GetDNSIP returns a dnsIP, which is 10th IP in svcSubnet CIDR range func GetDNSIP(svcSubnet string) (net.IP, error) { // Get the service subnet CIDR diff --git a/cmd/kubeadm/app/phases/upgrade/staticpods.go b/cmd/kubeadm/app/phases/upgrade/staticpods.go index 77056dcadda..33b1a0dae8a 100644 --- a/cmd/kubeadm/app/phases/upgrade/staticpods.go +++ b/cmd/kubeadm/app/phases/upgrade/staticpods.go @@ -51,6 +51,8 @@ type StaticPodPathManager interface { BackupManifestDir() string // BackupEtcdDir should point to the backup directory used for backuping manifests during the transition BackupEtcdDir() string + // CleanupDirs cleans up all temporary directories + CleanupDirs() error } // KubeStaticPodPathManager is a real implementation of StaticPodPathManager that is used when upgrading a static pod cluster @@ -59,34 +61,39 @@ type KubeStaticPodPathManager struct { tempManifestDir string backupManifestDir string backupEtcdDir string + + keepManifestDir bool + keepEtcdDir bool } // NewKubeStaticPodPathManager creates a new instance of KubeStaticPodPathManager -func NewKubeStaticPodPathManager(realDir, tempDir, backupDir, backupEtcdDir string) StaticPodPathManager { +func NewKubeStaticPodPathManager(realDir, tempDir, backupDir, backupEtcdDir string, keepManifestDir, keepEtcdDir bool) StaticPodPathManager { return &KubeStaticPodPathManager{ realManifestDir: realDir, tempManifestDir: tempDir, backupManifestDir: backupDir, backupEtcdDir: backupEtcdDir, + keepManifestDir: keepManifestDir, + keepEtcdDir: keepEtcdDir, } } // NewKubeStaticPodPathManagerUsingTempDirs creates a new instance of KubeStaticPodPathManager with temporary directories backing it -func NewKubeStaticPodPathManagerUsingTempDirs(realManifestDir string) (StaticPodPathManager, error) { +func NewKubeStaticPodPathManagerUsingTempDirs(realManifestDir string, saveManifestsDir, saveEtcdDir bool) (StaticPodPathManager, error) { upgradedManifestsDir, err := constants.CreateTempDirForKubeadm("kubeadm-upgraded-manifests") if err != nil { return nil, err } - backupManifestsDir, err := constants.CreateTempDirForKubeadm("kubeadm-backup-manifests") + backupManifestsDir, err := constants.CreateTimestampDirForKubeadm("kubeadm-backup-manifests") if err != nil { return nil, err } - backupEtcdDir, err := constants.CreateTempDirForKubeadm("kubeadm-backup-etcd") + backupEtcdDir, err := constants.CreateTimestampDirForKubeadm("kubeadm-backup-etcd") if err != nil { return nil, err } - return NewKubeStaticPodPathManager(realManifestDir, upgradedManifestsDir, backupManifestsDir, backupEtcdDir), nil + return NewKubeStaticPodPathManager(realManifestDir, upgradedManifestsDir, backupManifestsDir, backupEtcdDir, saveManifestsDir, saveEtcdDir), nil } // MoveFile should move a file from oldPath to newPath @@ -129,6 +136,26 @@ func (spm *KubeStaticPodPathManager) BackupEtcdDir() string { return spm.backupEtcdDir } +// CleanupDirs cleans up all temporary directories except those the user has requested to keep around +func (spm *KubeStaticPodPathManager) CleanupDirs() error { + if err := os.RemoveAll(spm.TempManifestDir()); err != nil { + return err + } + if !spm.keepManifestDir { + if err := os.RemoveAll(spm.BackupManifestDir()); err != nil { + return err + } + } + + if !spm.keepEtcdDir { + if err := os.RemoveAll(spm.BackupEtcdDir()); err != nil { + return err + } + } + + return nil +} + func upgradeComponent(component string, waiter apiclient.Waiter, pathMgr StaticPodPathManager, cfg *kubeadmapi.MasterConfiguration, beforePodHash string, recoverManifests map[string]string, isTLSUpgrade bool) error { // Special treatment is required for etcd case, when rollbackOldManifests should roll back etcd // manifests only for the case when component is Etcd @@ -449,11 +476,7 @@ func StaticPodControlPlane(waiter apiclient.Waiter, pathMgr StaticPodPathManager // Remove the temporary directories used on a best-effort (don't fail if the calls error out) // The calls are set here by design; we should _not_ use "defer" above as that would remove the directories // even in the "fail and rollback" case, where we want the directories preserved for the user. - os.RemoveAll(pathMgr.TempManifestDir()) - os.RemoveAll(pathMgr.BackupManifestDir()) - os.RemoveAll(pathMgr.BackupEtcdDir()) - - return nil + return pathMgr.CleanupDirs() } // rollbackOldManifests rolls back the backed-up manifests if something went wrong. diff --git a/cmd/kubeadm/app/phases/upgrade/staticpods_test.go b/cmd/kubeadm/app/phases/upgrade/staticpods_test.go index 1ba9042b77b..f8b85d89c59 100644 --- a/cmd/kubeadm/app/phases/upgrade/staticpods_test.go +++ b/cmd/kubeadm/app/phases/upgrade/staticpods_test.go @@ -203,6 +203,19 @@ func (spm *fakeStaticPodPathManager) BackupEtcdDir() string { return spm.backupEtcdDir } +func (spm *fakeStaticPodPathManager) CleanupDirs() error { + if err := os.RemoveAll(spm.TempManifestDir()); err != nil { + return err + } + if err := os.RemoveAll(spm.BackupManifestDir()); err != nil { + return err + } + if err := os.RemoveAll(spm.BackupEtcdDir()); err != nil { + return err + } + return nil +} + type fakeTLSEtcdClient struct{ TLS bool } func (c fakeTLSEtcdClient) HasTLS() bool { @@ -513,3 +526,81 @@ func getConfig(version, certsDir, etcdDataDir string) (*kubeadmapi.MasterConfigu kubeadmscheme.Scheme.Convert(externalcfg, internalcfg, nil) return internalcfg, nil } + +func getTempDir(t *testing.T, name string) (string, func()) { + dir, err := ioutil.TempDir(os.TempDir(), name) + if err != nil { + t.Fatalf("couldn't make temporary directory: %v", err) + } + + return dir, func() { + os.RemoveAll(dir) + } +} + +func TestCleanupDirs(t *testing.T) { + tests := []struct { + name string + keepManifest, keepEtcd bool + }{ + { + name: "save manifest backup", + keepManifest: true, + }, + { + name: "save both etcd and manifest", + keepManifest: true, + keepEtcd: true, + }, + { + name: "save nothing", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + realManifestDir, cleanup := getTempDir(t, "realManifestDir") + defer cleanup() + + tempManifestDir, cleanup := getTempDir(t, "tempManifestDir") + defer cleanup() + + backupManifestDir, cleanup := getTempDir(t, "backupManifestDir") + defer cleanup() + + backupEtcdDir, cleanup := getTempDir(t, "backupEtcdDir") + defer cleanup() + + mgr := NewKubeStaticPodPathManager(realManifestDir, tempManifestDir, backupManifestDir, backupEtcdDir, test.keepManifest, test.keepEtcd) + err := mgr.CleanupDirs() + if err != nil { + t.Errorf("unexpected error cleaning up: %v", err) + } + + if _, err := os.Stat(tempManifestDir); !os.IsNotExist(err) { + t.Errorf("%q should not have existed", tempManifestDir) + } + _, err = os.Stat(backupManifestDir) + if test.keepManifest { + if err != nil { + t.Errorf("unexpected error getting backup manifest dir") + } + } else { + if !os.IsNotExist(err) { + t.Error("expected backup manifest to not exist") + } + } + + _, err = os.Stat(backupEtcdDir) + if test.keepEtcd { + if err != nil { + t.Errorf("unexpected error getting backup etcd dir") + } + } else { + if !os.IsNotExist(err) { + t.Error("expected backup etcd dir to not exist") + } + } + }) + } +} diff --git a/cmd/kubeadm/app/util/etcd/BUILD b/cmd/kubeadm/app/util/etcd/BUILD index 8a439c27352..852e914f87f 100644 --- a/cmd/kubeadm/app/util/etcd/BUILD +++ b/cmd/kubeadm/app/util/etcd/BUILD @@ -6,6 +6,7 @@ go_library( importpath = "k8s.io/kubernetes/cmd/kubeadm/app/util/etcd", visibility = ["//visibility:public"], deps = [ + "//cmd/kubeadm/app/apis/kubeadm:go_default_library", "//cmd/kubeadm/app/constants:go_default_library", "//cmd/kubeadm/app/util/staticpod:go_default_library", "//vendor/github.com/coreos/etcd/clientv3:go_default_library", @@ -17,7 +18,10 @@ go_test( name = "go_default_test", srcs = ["etcd_test.go"], embed = [":go_default_library"], - deps = ["//cmd/kubeadm/test:go_default_library"], + deps = [ + "//cmd/kubeadm/app/apis/kubeadm:go_default_library", + "//cmd/kubeadm/test:go_default_library", + ], ) filegroup( diff --git a/cmd/kubeadm/app/util/etcd/etcd.go b/cmd/kubeadm/app/util/etcd/etcd.go index a390d670640..3470da6b5c9 100644 --- a/cmd/kubeadm/app/util/etcd/etcd.go +++ b/cmd/kubeadm/app/util/etcd/etcd.go @@ -26,6 +26,7 @@ import ( "github.com/coreos/etcd/clientv3" "github.com/coreos/etcd/pkg/transport" + kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm" "k8s.io/kubernetes/cmd/kubeadm/app/constants" "k8s.io/kubernetes/cmd/kubeadm/app/util/staticpod" ) @@ -216,3 +217,8 @@ func (c Client) WaitForClusterAvailable(delay time.Duration, retries int, retryI } return false, fmt.Errorf("timeout waiting for etcd cluster to be available") } + +// CheckConfigurationIsHA returns true if the given MasterConfiguration etcd block appears to be an HA configuration. +func CheckConfigurationIsHA(cfg *kubeadmapi.Etcd) bool { + return cfg.External != nil && len(cfg.External.Endpoints) > 1 +} diff --git a/cmd/kubeadm/app/util/etcd/etcd_test.go b/cmd/kubeadm/app/util/etcd/etcd_test.go index 1003f6cf262..3150183bc47 100644 --- a/cmd/kubeadm/app/util/etcd/etcd_test.go +++ b/cmd/kubeadm/app/util/etcd/etcd_test.go @@ -22,6 +22,7 @@ import ( "path/filepath" "testing" + kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm" testutil "k8s.io/kubernetes/cmd/kubeadm/test" ) @@ -195,3 +196,50 @@ func TestPodManifestHasTLS(t *testing.T) { } } } + +func TestCheckConfigurationIsHA(t *testing.T) { + var tests = []struct { + name string + cfg *kubeadmapi.Etcd + expected bool + }{ + { + name: "HA etcd", + cfg: &kubeadmapi.Etcd{ + External: &kubeadmapi.ExternalEtcd{ + Endpoints: []string{"10.100.0.1:2379", "10.100.0.2:2379", "10.100.0.3:2379"}, + }, + }, + expected: true, + }, + { + name: "single External etcd", + cfg: &kubeadmapi.Etcd{ + External: &kubeadmapi.ExternalEtcd{ + Endpoints: []string{"10.100.0.1:2379"}, + }, + }, + expected: false, + }, + { + name: "local etcd", + cfg: &kubeadmapi.Etcd{ + Local: &kubeadmapi.LocalEtcd{}, + }, + expected: false, + }, + { + name: "empty etcd struct", + cfg: &kubeadmapi.Etcd{}, + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if isHA := CheckConfigurationIsHA(test.cfg); isHA != test.expected { + t.Errorf("expected isHA to be %v, got %v", test.expected, isHA) + } + }) + } +}