From ad0179897d49409989560b7aae89661c4f182375 Mon Sep 17 00:00:00 2001 From: Ed Bartosh Date: Tue, 16 Oct 2018 18:01:53 +0300 Subject: [PATCH] kubeadm: skip upgrade if manifest is not changed When doing upgrades kubeadm generates new manifest and waits until kubelet restarts correspondent pod. However, kubelet won't restart pod if there are no changes in the manifest. That makes kubeadm stuck waiting for restarted pod. Skipping upgrade if new component manifest is the same as current manifest should solve this. Fixes: kubernetes/kubeadm#1054 --- cmd/kubeadm/app/phases/upgrade/BUILD | 1 + cmd/kubeadm/app/phases/upgrade/staticpods.go | 11 +++ cmd/kubeadm/app/util/staticpod/utils.go | 15 +++++ cmd/kubeadm/app/util/staticpod/utils_test.go | 71 ++++++++++++++++++++ 4 files changed, 98 insertions(+) diff --git a/cmd/kubeadm/app/phases/upgrade/BUILD b/cmd/kubeadm/app/phases/upgrade/BUILD index 3a4b859a9c6..c1accd0ecfc 100644 --- a/cmd/kubeadm/app/phases/upgrade/BUILD +++ b/cmd/kubeadm/app/phases/upgrade/BUILD @@ -37,6 +37,7 @@ go_library( "//cmd/kubeadm/app/util/apiclient:go_default_library", "//cmd/kubeadm/app/util/dryrun:go_default_library", "//cmd/kubeadm/app/util/etcd:go_default_library", + "//cmd/kubeadm/app/util/staticpod:go_default_library", "//pkg/version:go_default_library", "//staging/src/k8s.io/api/apps/v1:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", diff --git a/cmd/kubeadm/app/phases/upgrade/staticpods.go b/cmd/kubeadm/app/phases/upgrade/staticpods.go index 94f18d778b8..cae7d7a1c7e 100644 --- a/cmd/kubeadm/app/phases/upgrade/staticpods.go +++ b/cmd/kubeadm/app/phases/upgrade/staticpods.go @@ -32,6 +32,7 @@ import ( "k8s.io/kubernetes/cmd/kubeadm/app/util" "k8s.io/kubernetes/cmd/kubeadm/app/util/apiclient" etcdutil "k8s.io/kubernetes/cmd/kubeadm/app/util/etcd" + "k8s.io/kubernetes/cmd/kubeadm/app/util/staticpod" ) const ( @@ -201,6 +202,16 @@ func upgradeComponent(component string, waiter apiclient.Waiter, pathMgr StaticP // Store the backup path in the recover list. If something goes wrong now, this component will be rolled back. recoverManifests[component] = backupManifestPath + // Skip upgrade if current and new manifests are equal + equal, err := staticpod.ManifestFilesAreEqual(currentManifestPath, newManifestPath) + if err != nil { + return err + } + if equal { + fmt.Printf("[upgrade/staticpods] current and new manifests of %s are equal, skipping upgrade\n", component) + return nil + } + // Move the old manifest into the old-manifests directory if err := pathMgr.MoveFile(currentManifestPath, backupManifestPath); err != nil { return rollbackOldManifests(recoverManifests, err, pathMgr, recoverEtcd) diff --git a/cmd/kubeadm/app/util/staticpod/utils.go b/cmd/kubeadm/app/util/staticpod/utils.go index 8df649594af..6e2db42a458 100644 --- a/cmd/kubeadm/app/util/staticpod/utils.go +++ b/cmd/kubeadm/app/util/staticpod/utils.go @@ -17,6 +17,7 @@ limitations under the License. package staticpod import ( + "bytes" "fmt" "io/ioutil" "net" @@ -288,3 +289,17 @@ func GetProbeAddress(cfg *kubeadmapi.InitConfiguration, componentName string) st } return "127.0.0.1" } + +// ManifestFilesAreEqual compares 2 files. It returns true if their contents are equal, false otherwise +func ManifestFilesAreEqual(path1, path2 string) (bool, error) { + content1, err := ioutil.ReadFile(path1) + if err != nil { + return false, err + } + content2, err := ioutil.ReadFile(path2) + if err != nil { + return false, err + } + + return bytes.Equal(content1, content2), nil +} diff --git a/cmd/kubeadm/app/util/staticpod/utils_test.go b/cmd/kubeadm/app/util/staticpod/utils_test.go index ea9a1657d4e..36878e0c11b 100644 --- a/cmd/kubeadm/app/util/staticpod/utils_test.go +++ b/cmd/kubeadm/app/util/staticpod/utils_test.go @@ -22,6 +22,7 @@ import ( "path/filepath" "reflect" "sort" + "strconv" "testing" "k8s.io/api/core/v1" @@ -622,3 +623,73 @@ func TestReadStaticPodFromDisk(t *testing.T) { } } } + +func TestManifestFilesAreEqual(t *testing.T) { + var tests = []struct { + description string + podYamls []string + expectedResult bool + expectErr bool + }{ + { + description: "manifests are equal", + podYamls: []string{validPod, validPod}, + expectedResult: true, + expectErr: false, + }, + { + description: "manifests are not equal", + podYamls: []string{validPod, validPod + "\n"}, + expectedResult: false, + expectErr: false, + }, + { + description: "first manifest doesn't exist", + podYamls: []string{validPod, ""}, + expectedResult: false, + expectErr: true, + }, + { + description: "second manifest doesn't exist", + podYamls: []string{"", validPod}, + expectedResult: false, + expectErr: true, + }, + } + + for _, rt := range tests { + tmpdir := testutil.SetupTempDir(t) + defer os.RemoveAll(tmpdir) + + // write 2 manifests + for i := 0; i < 2; i++ { + if rt.podYamls[i] != "" { + manifestPath := filepath.Join(tmpdir, strconv.Itoa(i)+".yaml") + err := ioutil.WriteFile(manifestPath, []byte(rt.podYamls[i]), 0644) + if err != nil { + t.Fatalf("Failed to write manifest file\n%s\n\tfatal error: %v", rt.description, err) + } + } + } + + // compare them + result, actualErr := ManifestFilesAreEqual(filepath.Join(tmpdir, "0.yaml"), filepath.Join(tmpdir, "1.yaml")) + if result != rt.expectedResult { + t.Errorf( + "ManifestFilesAreEqual failed\n%s\nexpected result: %t\nactual result: %t", + rt.description, + rt.expectedResult, + result, + ) + } + if (actualErr != nil) != rt.expectErr { + t.Errorf( + "ManifestFilesAreEqual failed\n%s\n\texpected error: %t\n\tgot: %t\n\tactual error: %v", + rt.description, + rt.expectErr, + (actualErr != nil), + actualErr, + ) + } + } +}