`Kubeadm upgrade plan` support json/yaml output #108447

pacoxu · 2022-03-02T11:29:58Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Based on #83941.

Which issue(s) this PR fixes:

xref kubernetes/kubeadm#494

Special notes for your reviewer:

1. text print will not be changed.

2. json example: upgrade from v1.23.3 to v1.23.4

{
    "kind": "UpgradePlan",
    "apiVersion": "output.kubeadm.k8s.io/v1alpha2",
    "components": [
        {
            "name": "kube-apiserver",
            "currentVersion": "v1.23.3",
            "newVersion": "v1.23.4"
        },
        {
            "name": "kube-controller-manager",
            "currentVersion": "v1.23.3",
            "newVersion": "v1.23.4"
        },
        {
            "name": "kube-scheduler",
            "currentVersion": "v1.23.3",
            "newVersion": "v1.23.4"
        },
        {
            "name": "kube-proxy",
            "currentVersion": "v1.23.3",
            "newVersion": "v1.23.4"
        },
        {
            "name": "CoreDNS",
            "currentVersion": "v1.8.6",
            "newVersion": "v1.8.6"
        },
        {
            "name": "etcd",
            "currentVersion": "3.5.1-0",
            "newVersion": "3.5.1-0"
        }
    ],
    "configVersions": null
}

3. yaml example

apiVersion: output.kubeadm.k8s.io/v1alpha2
components:
- currentVersion: v1.23.3
  name: kube-apiserver
  newVersion: v1.23.4
- currentVersion: v1.23.3
  name: kube-controller-manager
  newVersion: v1.23.4
- currentVersion: v1.23.3
  name: kube-scheduler
  newVersion: v1.23.4
- currentVersion: v1.23.3
  name: kube-proxy
  newVersion: v1.23.4
- currentVersion: v1.8.6
  name: CoreDNS
  newVersion: v1.8.6
- currentVersion: 3.5.1-0
  name: etcd
  newVersion: 3.5.1-0
configVersions: null
kind: UpgradePlan

Does this PR introduce a user-facing change?

kubeadm: support experimental JSON/YAML output for "kubeadm upgrade plan" with the "--output" flag

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2022-03-02T11:29:59Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

- cherry-pick 83941 and rebase

pacoxu · 2022-03-03T10:36:41Z

/priority important-longterm
/triage accepted

cmd/kubeadm/app/util/output/output.go

cmd/kubeadm/app/cmd/upgrade/plan.go

cmd/kubeadm/app/util/output/output.go

neolit123 · 2022-03-21T18:25:13Z

cmd/kubeadm/app/cmd/upgrade/plan.go

 	}

-	printLineSeparator(w)
+	printLineSeparator(writer, printer)
+	printer.Close(writer)


related to the comments about Close(), printing on close is a bit strange and this is confusing me.

would it make sense to print the object explicitly with PrintObj.
the p.Buffer = []outputapiv1alpha2.ComponentUpgradePlan{} for the JSON/YAML printer makes more sense to be in a Flush()

do we have other usage of a Close() that actually makes sense related to a "close" operation?
e.g. closing open file handles.

neolit123

here is another review pass here.

there are some minor typos in comments
the Close() is better but i think there is a problem in Flush()
there are some strange new lines here and there, we should make these consistent if we can
if a text is printed without arguments it should use {F}println() and not {F}printf()

also as mentioned before, i'd really appreciate at least another reviewer for this...
see if you like to ping someone for review.

neolit123 · 2022-03-24T21:40:09Z

cmd/kubeadm/app/cmd/upgrade/common.go

-			fmt.Println("")
+			printer.Printf("[upgrade/config] In order to upgrade, a ConfigMap called %q in the %s namespace must exist.\n", constants.KubeadmConfigConfigMap, metav1.NamespaceSystem)
+			printer.Printf("[upgrade/config] Without this information, 'kubeadm upgrade' won't know how to configure your upgraded cluster.\n")
+			printer.Printf("\n")


we can add the Println() method to the interface and use it in all cases when we are printing a line without arguments. (similarly we have the Fprintf())

we have instances of this in common.go and more places.

cmd/kubeadm/app/cmd/upgrade/plan.go

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

Co-authored-by: Lubomir I. Ivanov <neolit123@gmail.com>

pacoxu · 2022-03-25T08:29:11Z

also i think this should have at least another reviewer!

@RA489 @SataQiu
Do you have time to take a look?

pacoxu · 2022-03-25T08:30:08Z

/cc @Klaven @fabriziopandini
cc to reviewers of #83941

neolit123

thanks for all the updates @pacoxu !

a bit disappointing that nobody else wants to review this...
it looks fine to me, but i can do another quick pass and maybe we can merge it in early 1.25.

/approve
/hold

k8s-ci-robot · 2022-03-30T15:26:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, pacoxu

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~~ [neolit123]

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

neolit123 · 2022-05-12T19:07:04Z

/hold cancel
let's merge this next week if there are no blocking comments.

neolit123 · 2022-05-17T22:38:31Z

/lgtm

k8s-ci-robot requested review from fabriziopandini and neolit123 March 2, 2022 11:31

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 2, 2022

feature(kubeadm): add support of json/yaml format for upgrade plan

93daef6

- cherry-pick 83941 and rebase

pacoxu force-pushed the kubeadm-json-upgrade-plan branch from 64da4c1 to e117531 Compare March 3, 2022 09:28

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2022

pacoxu marked this pull request as ready for review March 3, 2022 10:35

pacoxu force-pushed the kubeadm-json-upgrade-plan branch from e117531 to 0476877 Compare March 3, 2022 10:35

pacoxu changed the title ~~[WIP]Kubeadm upgrade plan support json/yaml output~~ Kubeadm upgrade plan support json/yaml output Mar 3, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 3, 2022

k8s-ci-robot requested review from RA489 and SataQiu March 3, 2022 10:35