Save kubeadm manifest backup directories #64337

liztio · 2018-05-25T21:19:07Z

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

Special notes for your reviewer:

Release note:

kubeadm now preserves previous manifests after upgrades

liztio · 2018-05-25T21:19:22Z

/wip

liztio · 2018-05-25T21:21:22Z

/label wip

liztio · 2018-05-25T21:35:13Z

/test pull-kubernetes-node-e2e

liztio · 2018-05-29T14:33:50Z

/test pull-kubernetes-kubemark-e2e-gce pull-kubernetes-node-e2e pull-kubernetes-kubemark-e2e-gce-big

liztio · 2018-05-29T14:34:51Z

/test pull-kubernetes-node-e2e

timothysc · 2018-05-30T19:45:04Z

/assign @timothysc @luxas

liztio · 2018-05-30T20:31:21Z

$ sudo ls /etc/kubernetes/tmp/kubeadm-backup-manifests-2018-05-30-20-27-24/
etcd.yaml  kube-apiserver.yaml	kube-controller-manager.yaml  kube-scheduler.yaml

liztio · 2018-05-30T20:32:05Z

/remove-label wip

timothysc

Minor nits, otherwise lgtm

/approve

timothysc · 2018-05-30T20:41:28Z

cmd/kubeadm/app/cmd/upgrade/apply.go

+// GetPathManagerForUpgrade returns a path manager properly configured for the given MasterConfiguration.
+func GetPathManagerForUpgrade(internalcfg *kubeadmapi.MasterConfiguration, etcdUpgrade bool) (upgrade.StaticPodPathManager, error) {
+	isHAEtcd := false
+	if internalcfg.Etcd.External != nil && len(internalcfg.Etcd.External.Endpoints) > 1 {


This logic can probably be a part of the existing etcd utils. There are a bunch.

timothysc · 2018-05-30T20:42:09Z

cmd/kubeadm/app/cmd/upgrade/apply_test.go

+	haEtcd := &kubeadmapi.MasterConfiguration{
+		Etcd: kubeadmapi.Etcd{
+			External: &kubeadmapi.ExternalEtcd{
+				Endpoints: []string{"10.100.0.1:2379", "10.100.0.2:2379"},


Should probably be 3 for consistency.

timothysc · 2018-05-30T20:52:05Z

cmd/kubeadm/app/constants/constants.go

 	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) {


I'm genuinely surprised that I couldn't find a utility f(n) in k8s land that didn't do this.

neolit123 · 2018-05-30T20:58:58Z

cmd/kubeadm/app/constants/constants.go

+	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 "", err


possibly wrap this error in fmt.Errorf("could not create timestamp directory %q: %v", timestampDir, err) ?

neolit123 · 2018-05-30T21:03:51Z

cmd/kubeadm/app/phases/upgrade/staticpods_test.go

+			}
+
+			if _, err := os.Stat(tempManifestDir); !os.IsNotExist(err) {
+				t.Errorf("%v should not have existed", tempManifestDir)


neolit123 · 2018-05-30T21:06:36Z

well done @liztio .
added a couple of small nits. feel free to ignore 👍

liztio · 2018-05-31T14:00:16Z

/hold

liztio · 2018-05-31T14:56:22Z

/remove-hold

liztio · 2018-05-31T14:57:01Z

/hold cancel

neolit123 · 2018-05-31T16:17:50Z

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-kubemark-e2e-gce-big

timothysc · 2018-05-31T18:01:58Z

cmd/kubeadm/app/phases/upgrade/staticpods.go

+			return err
+		}
+	}
+


minor format consistency things are usually nice to stick in editors to auto-save. totally non-blocking just an FYI.

I do have a lot of autocommit things, just not removing trailing space I guess

timothysc

please squash commits then lgtm.

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.

timothysc

/lgtm

luxas

/lgtm

Minor nits, but you don't have to address them now

luxas · 2018-05-31T19:53:33Z

cmd/kubeadm/app/util/etcd/etcd.go

@@ -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 {


nit: we call MasterConfiguration generally cfg. etcd or etcdCfg or etcdObj or e would probably be better here.

luxas · 2018-05-31T19:54:47Z

cmd/kubeadm/app/constants/constants.go

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


Can you make a constant for this or use one of the formats defined in time?

k8s-ci-robot · 2018-05-31T19:58:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liztio, luxas, timothysc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [luxas,timothysc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-05-31T20:00:16Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@liztio @luxas @timothysc

Pull Request Labels

sig/cluster-lifecycle: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/bug: Fixes a bug discovered during the current release.

Help

jberkus · 2018-05-31T21:07:58Z

/retest

k8s-github-robot · 2018-05-31T21:12:23Z

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 here.

k8s-ci-robot · 2018-06-01T00:51:18Z

@liztio: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-kubemark-e2e-gce-big	`6ed91fc`	link	`/test pull-kubernetes-kubemark-e2e-gce-big`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 25, 2018

k8s-ci-robot requested review from kad and xiangpengzhao May 25, 2018 21:19

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels May 25, 2018

liztio changed the title ~~Save etcd dir~~ [wip] Save etcd dir May 25, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2018

liztio force-pushed the save-etcd-dir branch 3 times, most recently from e202724 to c0e1f40 Compare May 30, 2018 17:40

timothysc added this to the v1.11 milestone May 30, 2018

k8s-github-robot added the milestone/incomplete-labels label May 30, 2018

timothysc added kind/enhancement priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed milestone/incomplete-labels labels May 30, 2018

k8s-ci-robot assigned luxas and timothysc May 30, 2018

k8s-github-robot added the milestone/incomplete-labels label May 30, 2018

liztio changed the title ~~[wip] Save etcd dir~~ Save etcd dir May 30, 2018

timothysc approved these changes May 30, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2018

neolit123 reviewed May 30, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2018

liztio force-pushed the save-etcd-dir branch from c0e1f40 to 4ef8022 Compare May 31, 2018 14:56

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2018

liztio changed the title ~~Save etcd dir~~ Save kubeadm manifest backup directories May 31, 2018

timothysc reviewed May 31, 2018

View reviewed changes

timothysc approved these changes May 31, 2018

View reviewed changes

Save kubeadm manifest backup directories

6ed91fc

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.

liztio force-pushed the save-etcd-dir branch from 4ef8022 to 6ed91fc Compare May 31, 2018 18:43

timothysc approved these changes May 31, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2018

luxas approved these changes May 31, 2018

View reviewed changes

timothysc added kind/bug Categorizes issue or PR as related to a bug. and removed kind/enhancement milestone/incomplete-labels labels May 31, 2018

k8s-github-robot merged commit 9fee771 into kubernetes:master May 31, 2018

liztio mentioned this pull request Jun 1, 2018

RFE: backup manifest dir gets wiped after upgrade kubernetes/kubeadm#489

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save kubeadm manifest backup directories #64337

Save kubeadm manifest backup directories #64337

liztio commented May 25, 2018

liztio commented May 25, 2018

liztio commented May 25, 2018

liztio commented May 25, 2018

liztio commented May 29, 2018

liztio commented May 29, 2018

timothysc commented May 30, 2018

liztio commented May 30, 2018

liztio commented May 30, 2018

timothysc left a comment

timothysc May 30, 2018

timothysc May 30, 2018

timothysc May 30, 2018

neolit123 May 30, 2018

neolit123 May 30, 2018

neolit123 commented May 30, 2018 •

edited

liztio commented May 31, 2018

liztio commented May 31, 2018

liztio commented May 31, 2018

neolit123 commented May 31, 2018

timothysc May 31, 2018

liztio May 31, 2018

timothysc left a comment

timothysc left a comment

luxas left a comment

luxas May 31, 2018

luxas May 31, 2018

k8s-ci-robot commented May 31, 2018

k8s-github-robot commented May 31, 2018

jberkus commented May 31, 2018

k8s-github-robot commented May 31, 2018

k8s-ci-robot commented Jun 1, 2018

Save kubeadm manifest backup directories #64337

Save kubeadm manifest backup directories #64337

Conversation

liztio commented May 25, 2018

liztio commented May 25, 2018

liztio commented May 25, 2018

liztio commented May 25, 2018

liztio commented May 29, 2018

liztio commented May 29, 2018

timothysc commented May 30, 2018

liztio commented May 30, 2018

liztio commented May 30, 2018

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented May 30, 2018 • edited

liztio commented May 31, 2018

liztio commented May 31, 2018

liztio commented May 31, 2018

neolit123 commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 31, 2018

k8s-github-robot commented May 31, 2018

jberkus commented May 31, 2018

k8s-github-robot commented May 31, 2018

k8s-ci-robot commented Jun 1, 2018

neolit123 commented May 30, 2018 •

edited