Merge pull request #59628 from mtaufen/kc-hide-trial-duration

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Bury KubeletConfiguration.ConfigTrialDuration for now

Based on discussion in https://github.com/kubernetes/kubernetes/pull/53833/files#r166669046, this PR chooses not to expose a knob for the trial duration yet. It is unclear exactly which shape this functionality should take in the API.

```release-note
The alpha KubeletConfiguration.ConfigTrialDuration field is no longer available.
```

pkg/kubelet/apis/kubeletconfig/helpers_test.go

@@ -152,7 +152,6 @@ var (
 		"CgroupsPerQOS",
 		"ClusterDNS[*]",
 		"ClusterDomain",
-		"ConfigTrialDuration.Duration",
 		"ContentType",
 		"EnableContentionProfiling",
 		"EnableControllerAttachDetach",

pkg/kubelet/apis/kubeletconfig/types.go

@@ -47,9 +47,6 @@ const (
 type KubeletConfiguration struct {
 	metav1.TypeMeta
 
-	// Only used for dynamic configuration.
-	// The length of the trial period for this configuration. This configuration will become the last-known-good after this duration.
-	ConfigTrialDuration metav1.Duration
 	// podManifestPath is the path to the directory containing pod manifests to
 	// run, or the path to a single manifest file
 	PodManifestPath string

pkg/kubelet/apis/kubeletconfig/v1alpha1/defaults.go

@@ -45,10 +45,6 @@ func addDefaultingFuncs(scheme *kruntime.Scheme) error {
 }
 
 func SetDefaults_KubeletConfiguration(obj *KubeletConfiguration) {
-	// pointer because the zeroDuration is valid - if you want to skip the trial period
-	if obj.ConfigTrialDuration == nil {
-		obj.ConfigTrialDuration = &metav1.Duration{Duration: 10 * time.Minute}
-	}
 	if obj.Authentication.Anonymous.Enabled == nil {
 		obj.Authentication.Anonymous.Enabled = boolVar(true)
 	}

pkg/kubelet/apis/kubeletconfig/v1alpha1/types.go

@@ -47,9 +47,6 @@ const (
 type KubeletConfiguration struct {
 	metav1.TypeMeta `json:",inline"`
 
-	// Only used for dynamic configuration.
-	// The length of the trial period for this configuration. This configuration will become the last-known-good after this duration.
-	ConfigTrialDuration *metav1.Duration `json:"configTrialDuration"`
 	// podManifestPath is the path to the directory containing pod manifests to
 	// run, or the path to a single manifest file
 	PodManifestPath string `json:"podManifestPath"`

pkg/kubelet/apis/kubeletconfig/v1alpha1/zz_generated.conversion.go

@@ -141,9 +141,6 @@ func Convert_kubeletconfig_KubeletAuthorization_To_v1alpha1_KubeletAuthorization
 }
 
 func autoConvert_v1alpha1_KubeletConfiguration_To_kubeletconfig_KubeletConfiguration(in *KubeletConfiguration, out *kubeletconfig.KubeletConfiguration, s conversion.Scope) error {
-	if err := v1.Convert_Pointer_v1_Duration_To_v1_Duration(&in.ConfigTrialDuration, &out.ConfigTrialDuration, s); err != nil {
-		return err
-	}
 	out.PodManifestPath = in.PodManifestPath
 	out.SyncFrequency = in.SyncFrequency
 	out.FileCheckFrequency = in.FileCheckFrequency
@@ -266,9 +263,6 @@ func Convert_v1alpha1_KubeletConfiguration_To_kubeletconfig_KubeletConfiguration
 }
 
 func autoConvert_kubeletconfig_KubeletConfiguration_To_v1alpha1_KubeletConfiguration(in *kubeletconfig.KubeletConfiguration, out *KubeletConfiguration, s conversion.Scope) error {
-	if err := v1.Convert_v1_Duration_To_Pointer_v1_Duration(&in.ConfigTrialDuration, &out.ConfigTrialDuration, s); err != nil {
-		return err
-	}
 	out.PodManifestPath = in.PodManifestPath
 	out.SyncFrequency = in.SyncFrequency
 	out.FileCheckFrequency = in.FileCheckFrequency

pkg/kubelet/apis/kubeletconfig/v1alpha1/zz_generated.deepcopy.go

@@ -21,7 +21,6 @@ limitations under the License.
 package v1alpha1
 
 import (
-	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	runtime "k8s.io/apimachinery/pkg/runtime"
 )
 
@@ -90,15 +89,6 @@ func (in *KubeletAuthorization) DeepCopy() *KubeletAuthorization {
 func (in *KubeletConfiguration) DeepCopyInto(out *KubeletConfiguration) {
 	*out = *in
 	out.TypeMeta = in.TypeMeta
-	if in.ConfigTrialDuration != nil {
-		in, out := &in.ConfigTrialDuration, &out.ConfigTrialDuration
-		if *in == nil {
-			*out = nil
-		} else {
-			*out = new(v1.Duration)
-			**out = **in
-		}
-	}
 	out.SyncFrequency = in.SyncFrequency
 	out.FileCheckFrequency = in.FileCheckFrequency
 	out.HTTPCheckFrequency = in.HTTPCheckFrequency

pkg/kubelet/apis/kubeletconfig/zz_generated.deepcopy.go

@@ -80,7 +80,6 @@ func (in *KubeletAuthorization) DeepCopy() *KubeletAuthorization {
 func (in *KubeletConfiguration) DeepCopyInto(out *KubeletConfiguration) {
 	*out = *in
 	out.TypeMeta = in.TypeMeta
-	out.ConfigTrialDuration = in.ConfigTrialDuration
 	out.SyncFrequency = in.SyncFrequency
 	out.FileCheckFrequency = in.FileCheckFrequency
 	out.HTTPCheckFrequency = in.HTTPCheckFrequency

pkg/kubelet/kubeletconfig/controller.go

@@ -39,6 +39,9 @@ import (
 
 const (
 	checkpointsDir = "checkpoints"
+	// TODO(mtaufen): We may expose this in a future API, but for the time being we use an internal default,
+	// because it is not especially clear where this should live in the API.
+	configTrialDuration = 10 * time.Minute
 )
 
 // Controller manages syncing dynamic Kubelet configurations
@@ -101,7 +104,7 @@ func (cc *Controller) Bootstrap() (*kubeletconfig.KubeletConfiguration, error) {
 		// periodically checks whether the last-known good needs to be updated
 		// we only do this when the assigned config loads and passes validation
 		// wait.Forever will call the func once before starting the timer
-		go wait.Forever(func() { cc.checkTrial(assigned.ConfigTrialDuration.Duration) }, 10*time.Second)
+		go wait.Forever(func() { cc.checkTrial(configTrialDuration) }, 10*time.Second)
 
 		return assigned, nil
 	} // Assert: the assigned config failed to load, parse, or validate

test/e2e_node/dynamic_kubelet_config_test.go

@@ -95,10 +95,8 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube
 		Context("When setting new NodeConfigSources that cause transitions between ConfigOK conditions", func() {
 			It("the Kubelet should report the appropriate status and configz", func() {
 				var err error
-				// we base the "correct" configmap off of the current configuration,
+				// we base the "correct" configmap off of the current configuration
-				// but we also set the trial duration very high to prevent changing the last-known-good
 				correctKC := originalKC.DeepCopy()
-				correctKC.ConfigTrialDuration = metav1.Duration{Duration: time.Hour}
 				correctConfigMap := newKubeletConfigMap("dynamic-kubelet-config-test-correct", correctKC)
 				correctConfigMap, err = f.ClientSet.CoreV1().ConfigMaps("kube-system").Create(correctConfigMap)
 				framework.ExpectNoError(err)
@@ -209,7 +207,7 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube
 
 				L := len(states)
 				for i := 1; i <= L; i++ { // need one less iteration than the number of states
-					testBothDirections(f, &states[i-1 : i][0], states[i:L])
+					testBothDirections(f, &states[i-1 : i][0], states[i:L], 0)
 				}
 
 			})
@@ -218,10 +216,8 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube
 		Context("When a remote config becomes the new last-known-good, and then the Kubelet is updated to use a new, bad config", func() {
 			It("the Kubelet should report a status and configz indicating that it rolled back to the new last-known-good", func() {
 				var err error
-				// we base the "lkg" configmap off of the current configuration, but set the trial
+				// we base the "lkg" configmap off of the current configuration
-				// duration very low so that it quickly becomes the last-known-good
 				lkgKC := originalKC.DeepCopy()
-				lkgKC.ConfigTrialDuration = metav1.Duration{Duration: time.Nanosecond}
 				lkgConfigMap := newKubeletConfigMap("dynamic-kubelet-config-test-intended-lkg", lkgKC)
 				lkgConfigMap, err = f.ClientSet.CoreV1().ConfigMaps("kube-system").Create(lkgConfigMap)
 				framework.ExpectNoError(err)
@@ -264,7 +260,8 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube
 					},
 				}
 
-				testBothDirections(f, &states[0], states[1:])
+				// wait 12 minutes after setting the first config to ensure it has time to pass the trial duration
+				testBothDirections(f, &states[0], states[1:], 12*time.Minute)
 			})
 		})
 
@@ -314,7 +311,7 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube
 				}
 
 				for i := 0; i < 50; i++ { // change the config 101 times (changes 3 times in the first iteration, 2 times in each subsequent iteration)
-					testBothDirections(f, &states[0], states[1:])
+					testBothDirections(f, &states[0], states[1:], 0)
 				}
 			})
 		})
@@ -323,13 +320,15 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube
 
 // testBothDirections tests the state change represented by each edge, where each state is a vertex,
 // and there are edges in each direction between first and each of the states.
-func testBothDirections(f *framework.Framework, first *configState, states []configState) {
+func testBothDirections(f *framework.Framework, first *configState, states []configState, waitAfterFirst time.Duration) {
 	// set to first and check that everything got set up properly
 	By(fmt.Sprintf("setting configSource to state %q", first.desc))
 	// we don't always expect an event here, because setting "first" might not represent
 	// a change from the current configuration
 	setAndTestKubeletConfigState(f, first, false)
 
+	time.Sleep(waitAfterFirst)
+
 	// for each state, set to that state, check condition and configz, then reset to first and check again
 	for i := range states {
 		By(fmt.Sprintf("from %q to %q", first.desc, states[i].desc))