unittests: Fixes unit tests for Windows (part 3) #110403

claudiubelu · 2022-06-06T12:18:06Z

What type of PR is this?

/kind failing-test

/sig windows
/sig testing

What this PR does / why we need it:

Currently, there are some unit tests that are failing on Windows due to various reasons:

DeadlineExceeded can occur when trying to read data from an UDP socket. This can be used to detect whether the port was closed or not.
In Windows, with long file name support enabled, file names can have up to 32,767 characters. In this case, the error windows.ERROR_FILENAME_EXCED_RANGE will be encountered instead.
files not closed, which means that they cannot be removed / renamed.
time.Now() is not as precise on Windows, which means that 2 consecutive calls may return the same timestamp.
path.Base() will return the same path. filepath.Base() should be used instead.

Which issue(s) this PR fixes:

Related: #51540

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

claudiubelu · 2022-06-14T15:49:00Z

/test pull-kubernetes-e2e-capz-windows-containerd

claudiubelu · 2022-06-14T15:51:25Z

/priority important-soon

SergeyKanzhelev · 2022-06-15T17:22:57Z

/priority important-soon

I'd let sig windows triage it

SergeyKanzhelev · 2022-06-15T17:23:52Z

@endocrimes since you are looking at other windows unit tests PR, do you want this one (feel free to unassign)?

/assign @endocrimes

claudiubelu · 2022-09-16T14:39:35Z

/retest

msau42

In the future, it would make things easier to review if the test fixes were split up into separate PRs based on type of change, for example, one PR to fix file paths, one PR to fix error codes, one PR to fix timestamps, etc.

msau42 · 2022-10-18T02:42:28Z

pkg/credentialprovider/gcp/metadata_test.go

@@ -47,6 +47,9 @@ func createProductNameFile() (string, error) {
 // referenced by gceProductNameFile being removed, which is the opposite of
 // the other tests
 func TestMetadata(t *testing.T) {
+	if !onGCEVM() {


This looks unrelated to windows tests. Can we move this to another PR?

Done: #113138

msau42 · 2022-10-18T02:44:37Z

pkg/kubelet/cm/devicemanager/plugin/v1beta1/stub.go

@@ -205,7 +205,7 @@ func (m *Stub) Register(kubeletEndpoint, resourceName string, pluginSockDir stri
 	client := pluginapi.NewRegistrationClient(conn)
 	reqt := &pluginapi.RegisterRequest{
 		Version:      pluginapi.Version,
-		Endpoint:     path.Base(m.socket),
+		Endpoint:     filepath.Base(m.socket),


This looks like it is changing some product code. Is there a corresponding unit test for this? Maybe it is better to move this to a separate PR.

Will separate this is a different PR.

msau42 · 2022-10-18T02:44:59Z

pkg/kubelet/kubelet_pods.go

@@ -122,7 +121,7 @@ func (kl *Kubelet) makeBlockVolumes(pod *v1.Pod, container *v1.Container, podVol
 		}
 		// Get a symbolic link associated to a block device under pod device path
 		dirPath, volName := vol.BlockVolumeMapper.GetPodDeviceMapPath()
-		symlinkPath := path.Join(dirPath, volName)
+		symlinkPath := filepath.Join(dirPath, volName)


Similar question. Any unit tests to cover this? May be better as a separate PR.

Will move to a separate PR.

msau42 · 2022-10-18T02:49:23Z

pkg/kubelet/pluginmanager/pluginwatcher/plugin_watcher_test.go

@@ -44,18 +43,17 @@ func init() {
 	flag.Set("alsologtostderr", fmt.Sprintf("%t", true))


The init() routine in general seems odd. I wonder if we can remove it (in a separate PR).

IMO, it is pretty useful. It increases the log verbosity, which would make debugging there plugin tests easier, there would be more useful logs collected, especially since there were some issues with plugins before.

msau42 · 2022-10-18T02:57:25Z

pkg/kubelet/pluginmanager/reconciler/reconciler_test.go

 	pluginName := fmt.Sprintf("example-plugin")
 	p := pluginwatcher.NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...)
 	require.NoError(t, p.Serve("v1beta1", "v1beta2"))
-	timestampBeforeRegistration := time.Now()
+	// On Windows, time.Now() is not as precise, which means that 2 consecutive calls may


Do you think this could result in real problems in the code? It looks like some timestamp comparisons are done in the reconcier: https://github.com/kubernetes/kubernetes/blob/bd39672559f332d37fb335025d972ea2f2811dc2/pkg/kubelet/pluginmanager/reconciler/reconciler.go#L128

Indeed, the clock resolution is not as fine grained on Windows as on Linux. Because of this, if a plugin has been reregsitered within a 1-15ms window, the reregistration workflow won't be applied, it won't be detected as being reregistered.

I've moved this to a separate PR, where I'll looking more into it: #113253

msau42 · 2022-10-18T03:20:06Z

pkg/proxy/winkernel/proxier_test.go

+		isDSR:                 false,
+		hns:                   newFakeHNS(),
+		endPointsRefCount:     make(endPointsReferenceCountMap),
+		forwardHealthCheckVip: true,


It looks like the fowardHealthCheckVip option is configurable. Is it that there are test cases that require this flag to be set, or that this is a required field for all cases?

msau42 · 2022-10-18T03:22:40Z

pkg/scheduler/framework/runtime/framework_test.go

@@ -168,6 +168,10 @@ func (pl *TestPlugin) Less(*framework.QueuedPodInfo, *framework.QueuedPodInfo) b
 }

 func (pl *TestPlugin) Score(ctx context.Context, state *framework.CycleState, p *v1.Pod, nodeName string) (int64, *framework.Status) {
+	// NOTE: The tests expect some non-zero latency to occur while running these methods. On Windows, time.Now()


Could this also result in product issues?

Separated into a different PR: #113253

pkg/volume/util/fs/fs_windows_test.go

thockin · 2022-10-19T18:10:05Z

needs a rebase

Currently, there are some unit tests that are failing on Windows due to various reasons: - paths not properly joined (filepath.Join should be used). - Proxy Mode IPVS not supported on Windows. - DeadlineExceeded can occur when trying to read data from an UDP socket. This can be used to detect whether the port was closed or not. - In Windows, with long file name support enabled, file names can have up to 32,767 characters. In this case, the error windows.ERROR_FILENAME_EXCED_RANGE will be encountered instead. - files not closed, which means that they cannot be removed / renamed. - time.Now() is not as precise on Windows, which means that 2 consecutive calls may return the same timestamp. - path.Base() will return the same path. filepath.Base() should be used instead. - path.Join() will always join the paths with a / instead of the OS specific separator. filepath.Join() should be used instead.

claudiubelu · 2022-10-24T20:31:33Z

needs a rebase

Done

msau42

/lgtm

msau42 · 2022-10-25T00:45:26Z

pkg/proxy/winkernel/proxier_test.go

+		isDSR:                 false,
+		hns:                   newFakeHNS(),
+		endPointsRefCount:     make(endPointsReferenceCountMap),
+		forwardHealthCheckVip: true,


We end up setting this to True for all test cases. Is there any value in testing with it set to False?

msau42 · 2022-10-25T00:47:02Z

/approve

claudiubelu · 2022-10-26T06:51:02Z

@thockin, can you take another look? Ty!

thockin · 2022-10-31T20:59:07Z

/approve

k8s-ci-robot · 2022-10-31T20:59:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: claudiubelu, endocrimes, marosset, mrunalp, msau42, thockin

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/cm/OWNERS~~ [mrunalp,thockin]
~~pkg/kubelet/pluginmanager/OWNERS~~ [mrunalp,thockin]
~~pkg/proxy/apis/config/OWNERS~~ [thockin]
~~pkg/proxy/winkernel/OWNERS~~ [thockin]
~~pkg/routes/OWNERS~~ [thockin]
~~pkg/scheduler/OWNERS~~ [thockin]
~~pkg/util/removeall/OWNERS~~ [msau42,thockin]
~~pkg/volume/OWNERS~~ [msau42,thockin]

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

k8s-ci-robot requested review from caesarxuchao and caseydavenport June 6, 2022 12:20

claudiubelu force-pushed the unittests-3 branch 2 times, most recently from 46a99f2 to ca9690a Compare June 8, 2022 10:09

claudiubelu force-pushed the unittests-3 branch from ca9690a to dfb2c1b Compare June 13, 2022 22:17

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 13, 2022

claudiubelu force-pushed the unittests-3 branch from dfb2c1b to 8d50ac5 Compare June 13, 2022 22:28

claudiubelu force-pushed the unittests-3 branch from 8d50ac5 to 542db86 Compare June 14, 2022 08:52

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 15, 2022

k8s-ci-robot assigned endocrimes Jun 15, 2022

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

claudiubelu force-pushed the unittests-3 branch from ea8ec70 to 4ce9d54 Compare September 19, 2022 07:59

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2022

claudiubelu force-pushed the unittests-3 branch 2 times, most recently from 1e6f14c to bd39672 Compare September 19, 2022 10:02

mrunalp approved these changes Sep 27, 2022

View reviewed changes

thockin assigned msau42 Oct 14, 2022

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

msau42 reviewed Oct 18, 2022

View reviewed changes

claudiubelu mentioned this pull request Oct 18, 2022

unittests: Skip test if not on GCE #113138

Merged

claudiubelu mentioned this pull request Oct 21, 2022

WIP: unittests: Fixes unit tests for Windows (time) #113253

Closed

claudiubelu force-pushed the unittests-3 branch from bd39672 to 17e8ade Compare October 21, 2022 12:36

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2022

claudiubelu force-pushed the unittests-3 branch from 17e8ade to 9824feb Compare October 21, 2022 12:42

claudiubelu force-pushed the unittests-3 branch from 9824feb to 9f95b7b Compare October 21, 2022 16:26

msau42 reviewed Oct 25, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2022

k8s-ci-robot merged commit 4c657e5 into kubernetes:master Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unittests: Fixes unit tests for Windows (part 3) #110403

unittests: Fixes unit tests for Windows (part 3) #110403

claudiubelu commented Jun 6, 2022 •

edited

Loading

claudiubelu commented Jun 14, 2022

claudiubelu commented Jun 14, 2022

SergeyKanzhelev commented Jun 15, 2022

SergeyKanzhelev commented Jun 15, 2022

claudiubelu commented Sep 16, 2022

msau42 left a comment

msau42 Oct 18, 2022

claudiubelu Oct 18, 2022

msau42 Oct 18, 2022

claudiubelu Oct 18, 2022

msau42 Oct 18, 2022

claudiubelu Oct 18, 2022

msau42 Oct 18, 2022

claudiubelu Oct 18, 2022

msau42 Oct 18, 2022

claudiubelu Oct 21, 2022

msau42 Oct 18, 2022

msau42 Oct 18, 2022

claudiubelu Oct 21, 2022

thockin commented Oct 19, 2022

claudiubelu commented Oct 24, 2022

msau42 left a comment

msau42 Oct 25, 2022

msau42 commented Oct 25, 2022

claudiubelu commented Oct 26, 2022

thockin commented Oct 31, 2022

k8s-ci-robot commented Oct 31, 2022

		@@ -44,18 +43,17 @@ func init() {
		flag.Set("alsologtostderr", fmt.Sprintf("%t", true))

unittests: Fixes unit tests for Windows (part 3) #110403

unittests: Fixes unit tests for Windows (part 3) #110403

Conversation

claudiubelu commented Jun 6, 2022 • edited Loading

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

claudiubelu commented Jun 14, 2022

claudiubelu commented Jun 14, 2022

SergeyKanzhelev commented Jun 15, 2022

SergeyKanzhelev commented Jun 15, 2022

claudiubelu commented Sep 16, 2022

msau42 left a 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

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

thockin commented Oct 19, 2022

claudiubelu commented Oct 24, 2022

msau42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Oct 25, 2022

claudiubelu commented Oct 26, 2022

thockin commented Oct 31, 2022

k8s-ci-robot commented Oct 31, 2022

claudiubelu commented Jun 6, 2022 •

edited

Loading