kubeadm: redesign printAvailableUpgrades function

[bart0sh]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Split printAvailableUpgrades into 2 functions:
- genUpgradePlan that handles business logic
- printUpgradePlan that outputs upgrade plan

Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#2029

Special notes for your reviewer:

I'm going to improve tests in a structured output PR.

Does this PR introduce a user-facing change?:

NONE

[bart0sh]

/cc @neolit123 @rosti @fabriziopandini

[neolit123]

thanks for the PR @bart0sh
EOD today is code-freeze so we have to punt it to 1.19.

/kind cleanup
/priority important-longterm

[bart0sh]

/retest

[rosti]

Thanks @bart0sh !
Sorry for the late review!
I like how things are going with this.

[rosti]

I would suggest a slightly different approach in here. Instead of having a couple of if statements here, we can divide the UpgradePlane like so:

type UpgradePlan struct {
	metav1.TypeMeta

	Before []ComponentUpgradePlan
	Main   []ComponentUpgradePlan
	After   []ComponentUpgradePlan
}

Then we'll simply print 3 tables (if they are not empty that is).

[bart0sh]

@rosti thank you for the review!

This would make the structure less flexible in my opinion. I like it more when it's less nested, i.e. when it's just a list of components. This division to 3 groups looks a bit artificial to me and only make sense for the text output.

Also "Before" and "After" names can be confusing as they may sound like "Before the upgrade" and "After the upgrade".

@neolit123, @fabriziopandini wdyt?

[neolit123]

why is the separate Before, Main, After better?

i think it makes sense to define a Component as external via a IsExternal bool field.

we can then:

• count the external components
• if > 0 print the external header
  • print each external component
• print the local components header
• iterate over the local components
• print the local component

would that work?

PS: i think we should stop reporting the version of external etcd!

[bart0sh]

It would work, but it's not enough to cover current output layout. There are 3 tables in the current output: manual upgrade components(kubelet), main components and external components.

My opinion is that we shouldn't be creating extra levels of complexity here. Text output is already quite complex, but it doesn't mean we should keep it and even do the same for structured output. I'd just go with the plain list of upgradable components in both modes (text and structured) and add notes for users at the end of the text output. would it work for you?

[bart0sh]

if you want to keep current output that's ok too. This PR does exactly that. Feel free to merge it as it is in this case :)

[rosti]

I am somewhat fine with not saying anything for etcd if it's external (probably just print it's external though). It's the responsibility of the user to handle it. It is certainly handy, though, to prompt users to upgrade their external etcd whenever the current version is being dropped by the API server (for example, when etcd 4.0 is released someday).

I do think, that it's beneficial for the output API to be able to tell components that are going to be upgraded by kubeadm and ones that need manual upgrades after that (kubelet). It's entirely possible, that some automation tool is going to use the information to, say apt-get the correct package after apply.

[rosti]

It's best to keep the user friendly component names if we can. For that matter, we can perform a quick translation here by using a static map[string]string in a helper func. Something like so:

var userFriendlyComponentNamesMap = map[string]string {
	constants.KubeAPIServer: "API Server",
	// ...
}

func getUserFriendlyComponentName(component string) string {
	if name, ok := userFriendlyComponentNamesMap[component]; ok {
		return name
	}
	return component
}

// ...
fmt.Fprintf(tabw, "%s\t%s\t%s\n", getUserFriendlyComponentName(component.Name), component.CurrentVersion, component.NewVersion)
// ...

[bart0sh]

AFAIR this was discussed in one of the previous PRs of this kind and we decided to go with the names from constants.go. @neolit123 was ok with this.

[neolit123]

i don't recall, my personal preference is to have everything coming from constants - e.g. kube-apiserver, kubelet.... our upgrade apply output is doing that.

[bart0sh]

ok, thanks. Let's agree not to have different names for the same component then. @rosti would it be ok for you?

[rosti]

It's possible that some existing tools actually parse those tables at the moment. Hence, changing API Server to kube-apiserver might prove problematic for some users. That's my only concern right now. Otherwise, I am fine with using the consts in the user readable output.

@detiber by any chance, is Cluster API doing any parsing of this table (kubeadm upgrade plan that is)?

[detiber]

We are not currently doing any parsing of this output as we currently use a loose coupling with kubeadm.

[bart0sh]

@rosti re map or not to map names, I'm ok with either approach. Just let me know what you decided.

[bart0sh]

@rosti @neolit123 as for introducing groups of components to the UpgradePlan structure. I'm proposing either not to do it at all (see my reasons explained above) or at least name it differently.

[rosti]

@bart0sh I am fine with not remapping.
Also, due to kubernetes/kubeadm#2058 external etcd will likely not be reported anymore by kubeadm upgrade plan. That leaves only the kubelet as of today as a component that needs manual upgrading.
Hence, let's merge it as is. If we need to add differentiation we can do that in alpha2 of the output API.

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bart0sh, rosti

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 [rosti]

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