kubeadm: add timeouts to v1beta4.UpgradeConfiguration #123595

neolit123 · 2024-02-29T18:02:09Z

What type of PR is this?

/kind feature cleanup

What this PR does / why we need it:

Follow the same process of adding the Timeouts struct
to UpgradeConfiguration similarly to how it was done for
other API Kinds.

In the Timeouts struct include one new timeout:

UpgradeManifests
StaticPodMirroringTimeout and StaticPodMirroringRetryInterval
are use for just an API call to get Pods(). The already existing
constants.KubernetesAPICallRetryInterval
and kubeadmapi.GetActiveTimeouts().KubernetesAPICall.Duration
can be used for that instead.

Which issue(s) this PR fixes:

xref:

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?

NONE

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

neolit123 · 2024-02-29T18:02:37Z

/hold
/cc @SataQiu @pacoxu @calvin0327
PTAL

neolit123 · 2024-02-29T18:02:51Z

/triage accepted
/priority important-soon

neolit123 · 2024-02-29T18:03:46Z

cmd/kubeadm/app/constants/constants.go

-	// StaticPodMirroringTimeout specifies how much time kubeadm should wait for the static pods
-	// to be mirrored on the API server.
-	StaticPodMirroringTimeout = 30 * time.Second
-	// StaticPodMirroringRetryInterval specifies how often to check if static pods are mirrored at the
-	// API server.
-	StaticPodMirroringRetryInterval = 500 * time.Millisecond
-


this commits is just a cleanup to use the general k8s API-call for static pod get() calls too.

neolit123 · 2024-02-29T18:07:33Z

cmd/kubeadm/app/apis/kubeadm/timeoututils.go

@@ -32,6 +32,7 @@ func SetDefaultTimeouts(t **Timeouts) {
 		EtcdAPICall:                      &metav1.Duration{Duration: constants.EtcdAPICallTimeout},
 		TLSBootstrap:                     &metav1.Duration{Duration: constants.TLSBootstrapTimeout},
 		Discovery:                        &metav1.Duration{Duration: constants.DiscoveryTimeout},
+		UpgradeManifests:                 &metav1.Duration{Duration: constants.UpgradeManifestsTimeout},


i could not find anything else that is worth adding. if you find something LMK.

well...there are two more things that can be added, but they might be a bit tricky because they are used very early in the command execution.

our global kubeconfig -> client timeout is 10s:

kubernetes/cmd/kubeadm/app/util/kubeconfig/kubeconfig.go

Line 83 in d8122df

overrides := clientcmd.ConfigOverrides{Timeout: "10s"}

our "get k8s version" from label is also hardcoded:

kubernetes/cmd/kubeadm/app/util/version.go

Line 38 in d8122df

getReleaseVersionTimeout = 10 * time.Second

neolit123 · 2024-02-29T18:08:12Z

cmd/kubeadm/app/cmd/upgrade/diff_test.go

@@ -57,7 +58,9 @@ func TestRunDiff(t *testing.T) {
 	testUpgradeDiffConfigContents := []byte(fmt.Sprintf(`
 apiVersion: %s
 kind: UpgradeConfiguration
-contextLines: 4`, kubeadmapiv1.SchemeGroupVersion.String()))
+diff:
+  contextLines: 4`, kubeadmapiv1.SchemeGroupVersion.String()))


neolit123 · 2024-02-29T18:09:15Z

cmd/kubeadm/app/util/config/common.go

@@ -390,11 +390,13 @@ func isKubeadmPrereleaseVersion(versionInfo *apimachineryversion.Info, k8sVersio
 func prepareStaticVariables(config any) {
 	switch c := config.(type) {
 	case *kubeadmapi.InitConfiguration:
-		kubeadmapi.SetActiveTimeouts(c.Timeouts.DeepCopy())


these deepcopy are removed because SetActiveTimeouts() already creates a deepcopy.

neolit123 · 2024-02-29T18:10:45Z

cmd/kubeadm/app/util/config/upgradeconfiguration.go

@@ -38,17 +38,6 @@ import (

 var componentCfgGV = sets.New(kubeproxyconfig.GroupName, kubeletconfig.GroupName)

-// DefaultUpgradeConfiguration return a default UpgradeConfiguration


some of the refactors of this file is to make it look like other xxconfigruation.go files in the same package.

neolit123 · 2024-02-29T18:11:13Z

cmd/kubeadm/app/util/config/upgradeconfiguration.go

+// Then the external, versioned configuration is defaulted and converted to the internal type.
+// Right thereafter, the configuration is defaulted again with dynamic values
+// Lastly, the internal config is validated and returned.
+func LoadOrDefaultUpgradeConfiguration(cfgPath string, defaultversionedcfg *kubeadmapiv1.UpgradeConfiguration, opts LoadOrDefaultConfigurationOptions) (*kubeadmapi.UpgradeConfiguration, error) {


same here, even if the defaulting part is not used on call sites.

The refactor seems to be unrelated, but good to have.

neolit123 · 2024-02-29T18:12:21Z

cmd/kubeadm/app/util/config/upgradeconfiguration_test.go

@@ -96,7 +98,7 @@ func TestDocMapToUpgradeConfiguration(t *testing.T) {
 			if err != nil {
 				t.Fatalf("unexpected error of DocMapToUpgradeConfiguration: %v\nconfig: %s", err, string(b))
 			}
-			if diff := cmp.Diff(*cfg, tc.expectedCfg); diff != "" {
+			if diff := cmp.Diff(*cfg, tc.expectedCfg, cmpopts.IgnoreFields(kubeadmapi.UpgradeConfiguration{}, "Timeouts")); diff != "" {


this is common in similar tests, to just ignore checking the timeouts.

pacoxu · 2024-03-01T05:10:35Z

/cc @carlory

pacoxu

only some typos.

cmd/kubeadm/app/apis/kubeadm/types.go

cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go

cmd/kubeadm/app/constants/constants.go

pacoxu · 2024-03-01T10:04:27Z

cmd/kubeadm/app/util/config/upgradeconfiguration.go

+// Then the external, versioned configuration is defaulted and converted to the internal type.
+// Right thereafter, the configuration is defaulted again with dynamic values
+// Lastly, the internal config is validated and returned.
+func LoadOrDefaultUpgradeConfiguration(cfgPath string, defaultversionedcfg *kubeadmapiv1.UpgradeConfiguration, opts LoadOrDefaultConfigurationOptions) (*kubeadmapi.UpgradeConfiguration, error) {


The refactor seems to be unrelated, but good to have.

Follow the same process of adding the Timeouts struct to UpgradeConfiguration similarly to how it was done for other API Kinds. In the Timeouts struct include one new timeout: - UpgradeManifests

StaticPodMirroringTimeout and StaticPodMirroringRetryInterval are use for just an API call to get Pods(). The already existing constants.KubernetesAPICallRetryInterval and kubeadmapi.GetActiveTimeouts().KubernetesAPICall.Duration can be used for that instead.

neolit123 · 2024-03-03T07:53:19Z

updated. 3 more days before CF.

/cc @SataQiu @pacoxu

pacoxu

/lgtm

k8s-ci-robot · 2024-03-04T06:03:06Z

LGTM label has been added.

Git tree hash: 9573d2fd0ea295aeae04c0d4d397286e0bd5e4b4

k8s-ci-robot · 2024-03-04T06:03:26Z

[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,pacoxu]

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

neolit123

/hold cancel

neolit123 · 2024-03-04T07:45:42Z

/retest

k8s-ci-robot requested review from calvin0327, pacoxu and SataQiu February 29, 2024 18:02

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

k8s-ci-robot requested a review from chendave February 29, 2024 18:02

neolit123 commented Feb 29, 2024

View reviewed changes

neolit123 force-pushed the 1.30-add-timeouts-upgradeconfig branch from e721612 to a7a2c79 Compare February 29, 2024 18:38

k8s-ci-robot requested a review from carlory March 1, 2024 05:10

pacoxu reviewed Mar 1, 2024

View reviewed changes

neolit123 added 2 commits March 1, 2024 13:04

kubeadm: add Timeouts struct to v1beta4.UpgradeConfiguration

ded6354

Follow the same process of adding the Timeouts struct to UpgradeConfiguration similarly to how it was done for other API Kinds. In the Timeouts struct include one new timeout: - UpgradeManifests

neolit123 force-pushed the 1.30-add-timeouts-upgradeconfig branch from a7a2c79 to 99313be Compare March 1, 2024 11:04

k8s-ci-robot requested a review from pacoxu March 3, 2024 07:53

pacoxu approved these changes Mar 4, 2024

View reviewed changes

k8s-ci-robot assigned pacoxu Mar 4, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2024

neolit123 commented Mar 4, 2024

View reviewed changes

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

k8s-ci-robot merged commit 9d39ce0 into kubernetes:master Mar 4, 2024
14 checks passed

k8s-ci-robot added this to the v1.30 milestone Mar 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: add timeouts to v1beta4.UpgradeConfiguration #123595

kubeadm: add timeouts to v1beta4.UpgradeConfiguration #123595

neolit123 commented Feb 29, 2024 •

edited

neolit123 commented Feb 29, 2024

neolit123 commented Feb 29, 2024

neolit123 Feb 29, 2024

neolit123 Feb 29, 2024

neolit123 Feb 29, 2024

neolit123 Feb 29, 2024

neolit123 Feb 29, 2024

neolit123 Feb 29, 2024

pacoxu Mar 1, 2024

neolit123 Feb 29, 2024

pacoxu commented Mar 1, 2024

pacoxu left a comment

pacoxu Mar 1, 2024

neolit123 commented Mar 3, 2024

pacoxu left a comment

k8s-ci-robot commented Mar 4, 2024

k8s-ci-robot commented Mar 4, 2024

neolit123 left a comment

neolit123 commented Mar 4, 2024

		@@ -38,17 +38,6 @@ import (

		var componentCfgGV = sets.New(kubeproxyconfig.GroupName, kubeletconfig.GroupName)

		// DefaultUpgradeConfiguration return a default UpgradeConfiguration

kubeadm: add timeouts to v1beta4.UpgradeConfiguration #123595

kubeadm: add timeouts to v1beta4.UpgradeConfiguration #123595

Conversation

neolit123 commented Feb 29, 2024 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

neolit123 commented Feb 29, 2024

neolit123 commented Feb 29, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacoxu commented Mar 1, 2024

pacoxu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Mar 3, 2024

pacoxu left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 4, 2024

k8s-ci-robot commented Mar 4, 2024

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Mar 4, 2024

neolit123 commented Feb 29, 2024 •

edited