Refactor kubelet config validation tests #105360

shuheiktgw · 2021-09-30T00:15:53Z

What type of PR is this?

/sig node
/kind cleanup
/area test

What this PR does / why we need it:

The background is described in #105029. I also noticed several minor problems in the validations so I fixed them:

Missing the invalid validation: prefix
Not showing a flag in an error message. For example CPUCFSQuotaPeriod
Not showing a given value in an error message. For example NodeStatusMaxImages
Not showing possible options in an error message. For example TopologyManagerPolicy
Unnecessary capitalization. For example ShutdownGracePeriod
Use a case statement to validate options. For example MemorySwap.SwapBehavior
Use %q instead of %v if necessary. For example EnforceNodeAllocatable
And a few other minor fixes (I'll leave a comment on them)

Which issue(s) this PR fixes:

Fixes #105029

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Kubelet config validation error messages are updated

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

None

k8s-ci-robot · 2021-09-30T00:16:01Z

Hi @shuheiktgw. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

shuheiktgw · 2021-09-30T00:16:46Z

pkg/kubelet/apis/config/validation/validation.go

@@ -217,7 +224,7 @@ func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration) error
 		allErrors = append(allErrors, fmt.Errorf("invalid configuration: memoryThrottlingFactor is required when MemoryQoS feature flag is enabled"))
 	}
 	if kc.MemoryThrottlingFactor != nil && (*kc.MemoryThrottlingFactor <= 0 || *kc.MemoryThrottlingFactor > 1.0) {
-		allErrors = append(allErrors, fmt.Errorf("invalid configuration: memoryThrottlingFactor %v must be greater than 0 and less than or equal to 1.0", kc.MemoryThrottlingFactor))
+		allErrors = append(allErrors, fmt.Errorf("invalid configuration: memoryThrottlingFactor %v must be greater than 0 and less than or equal to 1.0", *kc.MemoryThrottlingFactor))


I'm pretty sure a used doesn't want to see a memory location 😄

shuheiktgw · 2021-09-30T00:17:10Z

pkg/kubelet/apis/config/validation/validation.go

 		}
 		if _, err := cpuset.Parse(kc.ReservedSystemCPUs); err != nil {
-			allErrors = append(allErrors, fmt.Errorf("unable to parse reservedSystemCPUs (--reserved-cpus), error: %v", err))
+			allErrors = append(allErrors, fmt.Errorf("invalid configuration: unable to parse reservedSystemCPUs (--reserved-cpus) %v, error: %w", kc.ReservedSystemCPUs, err))
 		}


Better to use %w for an error

shuheiktgw · 2021-09-30T00:17:48Z

pkg/kubelet/apis/config/validation/validation.go

 		}
-		if kc.ShutdownGracePeriod.Duration > 0 && kc.ShutdownGracePeriod.Duration < time.Duration(time.Second) {


Casting time.Second to time.Duration is unnecessary

shuheiktgw · 2021-09-30T00:18:24Z

pkg/kubelet/apis/config/validation/validation.go

 	}

 	if localFeatureGate.Enabled(features.GracefulNodeShutdown) {
-		if kc.ShutdownGracePeriod.Duration < 0 || kc.ShutdownGracePeriodCriticalPods.Duration < 0 || kc.ShutdownGracePeriodCriticalPods.Duration > kc.ShutdownGracePeriod.Duration {


Merged this validation with the later one to simplify the logic

/cc @wzshiming

/cc @bobbypage

pacoxu · 2021-09-30T01:54:39Z

pkg/kubelet/apis/config/validation/validation_test.go

@@ -14,22 +14,25 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package validation
+package validation_test


s/validation_test/validation/g

I thought that was one of the best practices to remove test files whenever we don't have to call unexported methods/functions from a test file but no? I found some test files with the _test suffix in this project but it does not seem that popular though...

Ref: https://pkg.go.dev/cmd/go@master#hdr-Test_packages

Test files that declare a package with the suffix "_test" will be compiled as a separate package, and then linked and run with the main test binary.

Kubernetes doesn't necessarily follow all golang conventions, we do this very inconsistently in kubelet:

ehashman@fedora:~/src/k8s$ git grep 'package .\+_test' pkg/kubelet/ pkg/kubelet/container/runtime_cache_test.go:package container_test pkg/kubelet/envvars/envvars_test.go:package envvars_test pkg/kubelet/kubelet_dockerless_test.go:package kubelet_test pkg/kubelet/runtimeclass/runtimeclass_manager_test.go:package runtimeclass_test

We can leave this as is or not, I don't think it really matters.

pacoxu · 2021-09-30T01:56:44Z

/test pull-kubernetes-unit

    validation_reserved_memory_test.go:116: expected error reserved memory may not be zero for NUMA node 0 and resource "memory", got invalid configuration: reserved memory may not be zero for NUMA node 0 and resource "memory"
FAIL
FAIL	k8s.io/kubernetes/pkg/kubelet/apis/config/validation	0.653s

The test cannot pass in my PC.

shuheiktgw · 2021-09-30T10:40:15Z

Whoops, I haven't noticed there are already tests for the ReservedMemory configurations, thanks! I removed the duplicate tests and updated the existing ones 👍

shuheiktgw · 2021-10-04T06:06:04Z

@pacoxu Sorry I forgot to reassign this PR to you after I fixed it! Would you review the PR again? 🙏

ehashman · 2021-10-06T16:11:16Z

/ok-to-test
/triage accepted
/priority important-longterm

shuheiktgw · 2021-11-17T23:10:00Z

@ehashman I've squashed the commits to two! 🙏

shuheiktgw · 2021-11-17T23:36:01Z

/retest

ehashman

/lgtm

ehashman · 2021-11-18T00:52:21Z

/assign @liggitt

Looks like this is an API reviewers only OWNERS file

shuheiktgw · 2021-11-18T13:40:11Z

Rebased the PR and resolved the conflicts

shuheiktgw · 2021-11-19T01:03:05Z

/retest

ehashman

/lgtm

liggitt · 2021-11-23T17:42:23Z

/approve

k8s-ci-robot · 2021-11-23T17:43:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pacoxu, shuheiktgw

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:

~~pkg/kubelet/apis/config/OWNERS~~ [liggitt]

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

k8s-triage-robot · 2021-11-23T21:11:29Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

ehashman · 2021-11-23T22:21:13Z

flake #106125
/retest-required

reylejano · 2021-11-24T00:43:50Z

/milestone clear
Release Managers are cutting 1.23.0-rc.0 and creating the release-1.23 branch

pacoxu · 2021-11-24T02:50:55Z

/test pull-kubernetes-e2e-kind-ipv6

ehashman · 2021-11-24T20:16:53Z

boo flakes, this will have to land when 1.24 opens :\

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 30, 2021

k8s-ci-robot requested review from derekwaynecarr and ehashman September 30, 2021 00:16

shuheiktgw commented Sep 30, 2021

View reviewed changes

shuheiktgw mentioned this pull request Sep 30, 2021

Improve unit tests for Kubelet configuration APIs #105029

Closed

pacoxu reviewed Sep 30, 2021

View reviewed changes

pacoxu added this to Triage in SIG Node PR Triage Sep 30, 2021

shuheiktgw requested a review from pacoxu October 4, 2021 06:05

ehashman reviewed Nov 18, 2021

View reviewed changes

ehashman moved this from Waiting on Author to Needs Approver in SIG Node PR Triage Nov 18, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2021

k8s-ci-robot assigned liggitt Nov 18, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2021

wzshiming moved this from Needs Approver to Waiting on Author in SIG Node PR Triage Nov 18, 2021

shuheiktgw added 2 commits November 18, 2021 22:31

Refactor Kubelet config validations

35ad91a

Refactor Kubelet config validation tests

2acdaeb

shuheiktgw force-pushed the refactor_kubelet_config_validation_tests branch from 9301e80 to 2acdaeb Compare November 18, 2021 13:38

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 18, 2021

ehashman reviewed Nov 22, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 23, 2021

k8s-ci-robot removed this from the v1.23 milestone Nov 24, 2021

ehashman moved this from Waiting on Author to Done in SIG Node PR Triage Dec 6, 2021

k8s-ci-robot merged commit b685b39 into kubernetes:master Dec 8, 2021

k8s-ci-robot added this to the v1.24 milestone Dec 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor kubelet config validation tests #105360

Refactor kubelet config validation tests #105360

shuheiktgw commented Sep 30, 2021

k8s-ci-robot commented Sep 30, 2021

shuheiktgw Sep 30, 2021

shuheiktgw Sep 30, 2021

shuheiktgw Sep 30, 2021

shuheiktgw Sep 30, 2021

pacoxu Nov 2, 2021

wzshiming Nov 2, 2021

pacoxu Sep 30, 2021

shuheiktgw Sep 30, 2021

ehashman Nov 11, 2021

pacoxu commented Sep 30, 2021 •

edited

shuheiktgw commented Sep 30, 2021 •

edited

shuheiktgw commented Oct 4, 2021

ehashman commented Oct 6, 2021

shuheiktgw commented Nov 17, 2021

shuheiktgw commented Nov 17, 2021

ehashman left a comment

ehashman commented Nov 18, 2021

shuheiktgw commented Nov 18, 2021

shuheiktgw commented Nov 19, 2021

ehashman left a comment

liggitt commented Nov 23, 2021

k8s-ci-robot commented Nov 23, 2021

k8s-triage-robot commented Nov 23, 2021

ehashman commented Nov 23, 2021

reylejano commented Nov 24, 2021

pacoxu commented Nov 24, 2021

ehashman commented Nov 24, 2021

		}
		if kc.ShutdownGracePeriod.Duration > 0 && kc.ShutdownGracePeriod.Duration < time.Duration(time.Second) {

Refactor kubelet config validation tests #105360

Refactor kubelet config validation tests #105360

Conversation

shuheiktgw commented Sep 30, 2021

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.:

k8s-ci-robot commented Sep 30, 2021

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

Choose a reason for hiding this comment

pacoxu commented Sep 30, 2021 • edited

shuheiktgw commented Sep 30, 2021 • edited

shuheiktgw commented Oct 4, 2021

ehashman commented Oct 6, 2021

shuheiktgw commented Nov 17, 2021

shuheiktgw commented Nov 17, 2021

ehashman left a comment

Choose a reason for hiding this comment

ehashman commented Nov 18, 2021

shuheiktgw commented Nov 18, 2021

shuheiktgw commented Nov 19, 2021

ehashman left a comment

Choose a reason for hiding this comment

liggitt commented Nov 23, 2021

k8s-ci-robot commented Nov 23, 2021

k8s-triage-robot commented Nov 23, 2021

ehashman commented Nov 23, 2021

reylejano commented Nov 24, 2021

pacoxu commented Nov 24, 2021

ehashman commented Nov 24, 2021

pacoxu commented Sep 30, 2021 •

edited

shuheiktgw commented Sep 30, 2021 •

edited