Merge pull request #64337 from liztio/save-etcd-dir

Automatic merge from submit-queue (batch tested with PRs 62460, 64480, 63774, 64540, 64337). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Save kubeadm manifest backup directories

**What this PR does / why we need it**:

Kubeadm will now preserves previous manifests after `kubeadm upgrade apply`. Previously these files would be deleted after the upgrade succeeded

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes # [kubeadm/489](https://github.com/kubernetes/kubeadm/issues/489)

**Special notes for your reviewer**:

**Release note**:

```release-note
kubeadm now preserves previous manifests after upgrades
```

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",
     ],
 )

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
 	}

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())
+				}
+			}
+		})
+	}
+
+}

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

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.

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")
+				}
+			}
+		})
+	}
+}

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(

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
+}

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)
+			}
+		})
+	}
+}