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.
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
unittests: Fixes unit tests for Windows (part 3) #110403
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ import ( | |
"flag" | ||
"fmt" | ||
"os" | ||
"path/filepath" | ||
"sync" | ||
"testing" | ||
"time" | ||
|
@@ -32,8 +33,6 @@ import ( | |
) | ||
|
||
var ( | ||
socketDir string | ||
|
||
supportedVersions = []string{"v1beta1", "v1beta2"} | ||
) | ||
|
||
|
@@ -44,18 +43,17 @@ func init() { | |
flag.Set("alsologtostderr", fmt.Sprintf("%t", true)) | ||
flag.StringVar(&logLevel, "logLevel", "6", "test") | ||
flag.Lookup("v").Value.Set(logLevel) | ||
} | ||
|
||
func initTempDir(t *testing.T) string { | ||
// Creating a different directory. os.RemoveAll is not atomic enough; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any documentation about why this happens? - Can we reference it here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have any documentation about why this happens, just empirical evidence. Basically, the tests would fail with something like this:
Basically, it failed while trying to run
With the above, the failure now becomes:
But, if we introduce a
Plus, having separate folders per test would be useful if we do need to run tests in parallel. |
||
// os.MkdirAll can get into an "Access Denied" error on Windows. | ||
d, err := os.MkdirTemp("", "plugin_test") | ||
if err != nil { | ||
panic(fmt.Sprintf("Could not create a temp directory: %s", d)) | ||
t.Fatalf("Could not create a temp directory %s: %v", d, err) | ||
} | ||
|
||
socketDir = d | ||
} | ||
|
||
func cleanup(t *testing.T) { | ||
require.NoError(t, os.RemoveAll(socketDir)) | ||
os.MkdirAll(socketDir, 0755) | ||
return d | ||
} | ||
|
||
func waitForRegistration( | ||
|
@@ -106,13 +104,14 @@ func retryWithExponentialBackOff(initialDuration time.Duration, fn wait.Conditio | |
} | ||
|
||
func TestPluginRegistration(t *testing.T) { | ||
defer cleanup(t) | ||
socketDir := initTempDir(t) | ||
defer os.RemoveAll(socketDir) | ||
|
||
dsw := cache.NewDesiredStateOfWorld() | ||
newWatcher(t, dsw, wait.NeverStop) | ||
newWatcher(t, socketDir, dsw, wait.NeverStop) | ||
|
||
for i := 0; i < 10; i++ { | ||
socketPath := fmt.Sprintf("%s/plugin-%d.sock", socketDir, i) | ||
socketPath := filepath.Join(socketDir, fmt.Sprintf("plugin-%d.sock", i)) | ||
pluginName := fmt.Sprintf("example-plugin-%d", i) | ||
|
||
p := NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...) | ||
|
@@ -139,16 +138,17 @@ func TestPluginRegistration(t *testing.T) { | |
} | ||
|
||
func TestPluginRegistrationSameName(t *testing.T) { | ||
defer cleanup(t) | ||
socketDir := initTempDir(t) | ||
defer os.RemoveAll(socketDir) | ||
|
||
dsw := cache.NewDesiredStateOfWorld() | ||
newWatcher(t, dsw, wait.NeverStop) | ||
newWatcher(t, socketDir, dsw, wait.NeverStop) | ||
|
||
// Make 10 plugins with the same name and same type but different socket path; | ||
// all 10 should be in desired state of world cache | ||
pluginName := "dep-example-plugin" | ||
for i := 0; i < 10; i++ { | ||
socketPath := fmt.Sprintf("%s/plugin-%d.sock", socketDir, i) | ||
socketPath := filepath.Join(socketDir, fmt.Sprintf("plugin-%d.sock", i)) | ||
p := NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...) | ||
require.NoError(t, p.Serve("v1beta1", "v1beta2")) | ||
|
||
|
@@ -164,14 +164,15 @@ func TestPluginRegistrationSameName(t *testing.T) { | |
} | ||
|
||
func TestPluginReRegistration(t *testing.T) { | ||
defer cleanup(t) | ||
socketDir := initTempDir(t) | ||
defer os.RemoveAll(socketDir) | ||
|
||
dsw := cache.NewDesiredStateOfWorld() | ||
newWatcher(t, dsw, wait.NeverStop) | ||
newWatcher(t, socketDir, dsw, wait.NeverStop) | ||
|
||
// Create a plugin first, we are then going to remove the plugin, update the plugin with a different name | ||
// and recreate it. | ||
socketPath := fmt.Sprintf("%s/plugin-reregistration.sock", socketDir) | ||
socketPath := filepath.Join(socketDir, "plugin-reregistration.sock") | ||
pluginName := "reregister-plugin" | ||
p := NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...) | ||
require.NoError(t, p.Serve("v1beta1", "v1beta2")) | ||
|
@@ -206,12 +207,13 @@ func TestPluginReRegistration(t *testing.T) { | |
} | ||
|
||
func TestPluginRegistrationAtKubeletStart(t *testing.T) { | ||
defer cleanup(t) | ||
socketDir := initTempDir(t) | ||
defer os.RemoveAll(socketDir) | ||
|
||
plugins := make([]*examplePlugin, 10) | ||
|
||
for i := 0; i < len(plugins); i++ { | ||
socketPath := fmt.Sprintf("%s/plugin-%d.sock", socketDir, i) | ||
socketPath := filepath.Join(socketDir, fmt.Sprintf("plugin-%d.sock", i)) | ||
pluginName := fmt.Sprintf("example-plugin-%d", i) | ||
|
||
p := NewTestExamplePlugin(pluginName, registerapi.DevicePlugin, socketPath, supportedVersions...) | ||
|
@@ -224,7 +226,7 @@ func TestPluginRegistrationAtKubeletStart(t *testing.T) { | |
} | ||
|
||
dsw := cache.NewDesiredStateOfWorld() | ||
newWatcher(t, dsw, wait.NeverStop) | ||
newWatcher(t, socketDir, dsw, wait.NeverStop) | ||
|
||
var wg sync.WaitGroup | ||
for i := 0; i < len(plugins); i++ { | ||
|
@@ -252,7 +254,7 @@ func TestPluginRegistrationAtKubeletStart(t *testing.T) { | |
} | ||
} | ||
|
||
func newWatcher(t *testing.T, desiredStateOfWorldCache cache.DesiredStateOfWorld, stopCh <-chan struct{}) *Watcher { | ||
func newWatcher(t *testing.T, socketDir string, desiredStateOfWorldCache cache.DesiredStateOfWorld, stopCh <-chan struct{}) *Watcher { | ||
w := NewWatcher(socketDir, desiredStateOfWorldCache) | ||
require.NoError(t, w.Start(stopCh)) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,18 +138,19 @@ func NewFakeProxier(syncPeriod time.Duration, minSyncPeriod time.Duration, clust | |
networkType: networkType, | ||
} | ||
proxier := &Proxier{ | ||
serviceMap: make(proxy.ServiceMap), | ||
endpointsMap: make(proxy.EndpointsMap), | ||
clusterCIDR: clusterCIDR, | ||
hostname: testHostName, | ||
nodeIP: nodeIP, | ||
serviceHealthServer: healthcheck.NewFakeServiceHealthServer(), | ||
network: *hnsNetworkInfo, | ||
sourceVip: sourceVip, | ||
hostMac: macAddress, | ||
isDSR: false, | ||
hns: newFakeHNS(), | ||
endPointsRefCount: make(endPointsReferenceCountMap), | ||
serviceMap: make(proxy.ServiceMap), | ||
endpointsMap: make(proxy.EndpointsMap), | ||
clusterCIDR: clusterCIDR, | ||
hostname: testHostName, | ||
nodeIP: nodeIP, | ||
serviceHealthServer: healthcheck.NewFakeServiceHealthServer(), | ||
network: *hnsNetworkInfo, | ||
sourceVip: sourceVip, | ||
hostMac: macAddress, | ||
isDSR: false, | ||
hns: newFakeHNS(), | ||
endPointsRefCount: make(endPointsReferenceCountMap), | ||
forwardHealthCheckVip: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a test that requires this field set to True:
Setting it to true results in the test passing. With that flag, an HNS Health Check LoadBalancer resource is created. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We end up setting this to True for all test cases. Is there any value in testing with it set to False? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as |
||
} | ||
|
||
serviceChanges := proxy.NewServiceChangeTracker(proxier.newServiceInfo, v1.IPv4Protocol, nil, proxier.serviceMapChange) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
//go:build !windows | ||
// +build !windows | ||
|
||
/* | ||
Copyright 2022 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package routes | ||
|
||
import "syscall" | ||
|
||
const ( | ||
fileNameTooLong = syscall.ENAMETOOLONG | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/* | ||
Copyright 2022 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package routes | ||
|
||
import "golang.org/x/sys/windows" | ||
|
||
const ( | ||
fileNameTooLong = windows.ERROR_FILENAME_EXCED_RANGE | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init() routine in general seems odd. I wonder if we can remove it (in a separate PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.