Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

kubeadm: drop applyFlags.newK8sVersionStr field #74256

Merged
merged 1 commit into from
Feb 21, 2019
Merged

kubeadm: drop applyFlags.newK8sVersionStr field #74256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 17 additions & 20 deletions cmd/kubeadm/app/cmd/upgrade/apply.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ type applyFlags struct {
dryRun bool
etcdUpgrade bool
criSocket string
newK8sVersionStr string
imagePullTimeout time.Duration
}

Expand All @@ -77,6 +76,7 @@ func NewCmdApply(apf *applyPlanFlags) *cobra.Command {
DisableFlagsInUseLine: true,
Short: "Upgrade your Kubernetes cluster to the specified version.",
Run: func(cmd *cobra.Command, args []string) {
var userVersion string
var err error
flags.ignorePreflightErrorsSet, err = validation.ValidateIgnorePreflightErrors(flags.ignorePreflightErrors)
kubeadmutil.CheckErr(err)
Expand All @@ -88,31 +88,30 @@ func NewCmdApply(apf *applyPlanFlags) *cobra.Command {

// If the version is specified in config file, pick up that value.
if flags.cfgPath != "" {
// Note that cfg isn't preserved here, it's just an one-off to populate flags.newK8sVersionStr based on --config
// Note that cfg isn't preserved here, it's just an one-off to populate userVersion based on --config
cfg, err := configutil.LoadInitConfigurationFromFile(flags.cfgPath)
kubeadmutil.CheckErr(err)

if cfg.KubernetesVersion != "" {
flags.newK8sVersionStr = cfg.KubernetesVersion
userVersion = cfg.KubernetesVersion
}
}

// If the new version is already specified in config file, version arg is optional.
if flags.newK8sVersionStr == "" {
if userVersion == "" {
err = cmdutil.ValidateExactArgNumber(args, []string{"version"})
kubeadmutil.CheckErr(err)
}

// If option was specified in both args and config file, args will overwrite the config file.
if len(args) == 1 {
flags.newK8sVersionStr = args[0]
userVersion = args[0]
}

// Default the flags dynamically, based on each others' value
err = SetImplicitFlags(flags)
err = assertVersionStringIsEmpty(userVersion)
kubeadmutil.CheckErr(err)

err = runApply(flags)
err = runApply(flags, userVersion)
kubeadmutil.CheckErr(err)
},
}
Expand Down Expand Up @@ -145,12 +144,12 @@ func NewCmdApply(apf *applyPlanFlags) *cobra.Command {
// - Creating the RBAC rules for the bootstrap tokens and the cluster-info ConfigMap
// - Applying new kube-dns and kube-proxy manifests
// - Uploads the newly used configuration to the cluster ConfigMap
func runApply(flags *applyFlags) error {
func runApply(flags *applyFlags, userVersion string) error {

// Start with the basics, verify that the cluster is healthy and get the configuration from the cluster (using the ConfigMap)
klog.V(1).Infof("[upgrade/apply] verifying health of cluster")
klog.V(1).Infof("[upgrade/apply] retrieving configuration from cluster")
client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, flags.dryRun, flags.newK8sVersionStr)
client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, flags.dryRun, userVersion)
if err != nil {
return err
}
Expand All @@ -167,7 +166,6 @@ func runApply(flags *applyFlags) error {
}

// Use normalized version string in all following code.
flags.newK8sVersionStr = cfg.KubernetesVersion
newK8sVersion, err := version.ParseSemantic(cfg.KubernetesVersion)
if err != nil {
return errors.Errorf("unable to parse normalized version %q as a semantic version", cfg.KubernetesVersion)
Expand All @@ -179,7 +177,7 @@ func runApply(flags *applyFlags) error {

// Enforce the version skew policies
klog.V(1).Infof("[upgrade/version] enforcing version skew policies")
if err := EnforceVersionPolicies(newK8sVersion, flags, versionGetter); err != nil {
if err := EnforceVersionPolicies(cfg.KubernetesVersion, newK8sVersion, flags, versionGetter); err != nil {
return errors.Wrap(err, "[upgrade/version] FATAL")
}

Expand Down Expand Up @@ -222,16 +220,15 @@ func runApply(flags *applyFlags) error {
}

fmt.Println("")
fmt.Printf("[upgrade/successful] SUCCESS! Your cluster was upgraded to %q. Enjoy!\n", flags.newK8sVersionStr)
fmt.Printf("[upgrade/successful] SUCCESS! Your cluster was upgraded to %q. Enjoy!\n", cfg.KubernetesVersion)
fmt.Println("")
fmt.Println("[upgrade/kubelet] Now that your control plane is upgraded, please proceed with upgrading your kubelets if you haven't already done so.")

return nil
}

// SetImplicitFlags handles dynamically defaulting flags based on each other's value
func SetImplicitFlags(flags *applyFlags) error {
if len(flags.newK8sVersionStr) == 0 {
func assertVersionStringIsEmpty(version string) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This is missing a Not from its name (e. g. assertVersionStringIsNotEmpty)
  2. I'd rather have the single if statement moved to the only place the func gets called. But that's only if you are not going to add more checks to it in a follow up PR(s).

WDYT?

if len(version) == 0 {
return errors.New("version string can't be empty")
}

Expand All @@ -240,10 +237,10 @@ func SetImplicitFlags(flags *applyFlags) error {

// EnforceVersionPolicies makes sure that the version the user specified is valid to upgrade to
// There are both fatal and skippable (with --force) errors
func EnforceVersionPolicies(newK8sVersion *version.Version, flags *applyFlags, versionGetter upgrade.VersionGetter) error {
fmt.Printf("[upgrade/version] You have chosen to change the cluster version to %q\n", flags.newK8sVersionStr)
func EnforceVersionPolicies(newK8sVersionStr string, newK8sVersion *version.Version, flags *applyFlags, versionGetter upgrade.VersionGetter) error {
fmt.Printf("[upgrade/version] You have chosen to change the cluster version to %q\n", newK8sVersionStr)

versionSkewErrs := upgrade.EnforceVersionPolicies(versionGetter, flags.newK8sVersionStr, newK8sVersion, flags.allowExperimentalUpgrades, flags.allowRCUpgrades)
versionSkewErrs := upgrade.EnforceVersionPolicies(versionGetter, newK8sVersionStr, newK8sVersion, flags.allowExperimentalUpgrades, flags.allowRCUpgrades)
if versionSkewErrs != nil {

if len(versionSkewErrs.Mandatory) > 0 {
Expand All @@ -268,7 +265,7 @@ func EnforceVersionPolicies(newK8sVersion *version.Version, flags *applyFlags, v
func PerformControlPlaneUpgrade(flags *applyFlags, client clientset.Interface, waiter apiclient.Waiter, internalcfg *kubeadmapi.InitConfiguration) error {

// OK, the cluster is hosted using static pods. Upgrade a static-pod hosted cluster
fmt.Printf("[upgrade/apply] Upgrading your Static Pod-hosted control plane to version %q...\n", flags.newK8sVersionStr)
fmt.Printf("[upgrade/apply] Upgrading your Static Pod-hosted control plane to version %q...\n", internalcfg.KubernetesVersion)

if flags.dryRun {
return DryRunStaticPodUpgrade(internalcfg)
Expand Down
47 changes: 15 additions & 32 deletions cmd/kubeadm/app/cmd/upgrade/apply_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,48 +19,31 @@ 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) {
var tests = []struct {
name string
flags *applyFlags
expectedFlags applyFlags
errExpected bool
func TestAssertVersionStringIsEmpty(t *testing.T) {
var tcases = []struct {
name string
version string
expectedErr bool
}{
{
name: "if the new version is empty; it should error out",
flags: &applyFlags{
newK8sVersionStr: "",
},
expectedFlags: applyFlags{
newK8sVersionStr: "",
},
errExpected: true,
name: "Version string is not empty",
version: "some string",
},
{
name: "Version string is empty",
expectedErr: true,
},
}
for _, rt := range tests {
t.Run(rt.name, func(t *testing.T) {
actualErr := SetImplicitFlags(rt.flags)

if !reflect.DeepEqual(*rt.flags, rt.expectedFlags) {
t.Errorf(
"failed SetImplicitFlags:\n\texpected flags: %v\n\t actual: %v",
rt.expectedFlags,
*rt.flags,
)
}
if (actualErr != nil) != rt.errExpected {
t.Errorf(
"failed SetImplicitFlags:\n\texpected error: %t\n\t actual: %t",
rt.errExpected,
(actualErr != nil),
)
for _, tt := range tcases {
t.Run(tt.name, func(t *testing.T) {
if assertVersionStringIsEmpty(tt.version) == nil && tt.expectedErr {
t.Errorf("No error triggered for string '%s'", tt.version)
}
})
}
Expand Down
13 changes: 6 additions & 7 deletions cmd/kubeadm/app/cmd/upgrade/plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ import (

type planFlags struct {
*applyPlanFlags

newK8sVersionStr string
}

// NewCmdPlan returns the cobra command for `kubeadm upgrade plan`
Expand All @@ -52,6 +50,7 @@ func NewCmdPlan(apf *applyPlanFlags) *cobra.Command {
Use: "plan [version] [flags]",
Short: "Check which versions are available to upgrade to and validate whether your current cluster is upgradeable. To skip the internet check, pass in the optional [version] parameter.",
Run: func(_ *cobra.Command, args []string) {
var userVersion string
var err error
flags.ignorePreflightErrorsSet, err = validation.ValidateIgnorePreflightErrors(flags.ignorePreflightErrors)
kubeadmutil.CheckErr(err)
Expand All @@ -65,15 +64,15 @@ func NewCmdPlan(apf *applyPlanFlags) *cobra.Command {
kubeadmutil.CheckErr(err)

if cfg.KubernetesVersion != "" {
flags.newK8sVersionStr = cfg.KubernetesVersion
userVersion = cfg.KubernetesVersion
}
}
// If option was specified in both args and config file, args will overwrite the config file.
if len(args) == 1 {
flags.newK8sVersionStr = args[0]
userVersion = args[0]
}

err = runPlan(flags)
err = runPlan(flags, userVersion)
kubeadmutil.CheckErr(err)
},
}
Expand All @@ -84,11 +83,11 @@ func NewCmdPlan(apf *applyPlanFlags) *cobra.Command {
}

// runPlan takes care of outputting available versions to upgrade to for the user
func runPlan(flags *planFlags) error {
func runPlan(flags *planFlags, userVersion string) error {
// Start with the basics, verify that the cluster is healthy, build a client and a versionGetter. Never dry-run when planning.
klog.V(1).Infof("[upgrade/plan] verifying health of cluster")
klog.V(1).Infof("[upgrade/plan] retrieving configuration from cluster")
client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, false, flags.newK8sVersionStr)
client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, false, userVersion)
if err != nil {
return err
}
Expand Down