test: remove windows-specific tests, use windows node pools #2349
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2349 +/- ##
======================================
Coverage 71.9% 71.9%
======================================
Files 130 130
Lines 23395 23395
======================================
Hits 16823 16823
Misses 5546 5546
Partials 1026 1026 |
Could we merge #2303 instead? It's just better hygiene to have 1 PR (commit) == 1 concern, plus it was here first. |
| - template: e2e-job-template.yaml | ||
| parameters: | ||
| name: 'k8s_1_13_release_e2e' | ||
| k8sRelease: '1.13' | ||
| apimodel: 'examples/e2e-tests/kubernetes/release/default/definition.json' | ||
| createVNET: true | ||
| stabilityIterations: '3' |
There was a problem hiding this comment.
So we've been using 3 for the "linux" tests, which are occasionally flaky :(
We were skipping for "windows" tests.
The most cautious way to converge the two was to stop running these "stability" tests on PR tests for now.
(That's my thinking, at least.)
| @@ -1891,11 +1891,11 @@ var _ = Describe("Azure Container Cluster using the Kubernetes Orchestrator", fu | |||
| if clusterAutoscalerEngaged { | |||
| cpuTarget = 50 | |||
| for _, profile := range eng.ExpandedDefinition.Properties.AgentPoolProfiles { | |||
| maxPods, _ := strconv.Atoi(profile.KubernetesConfig.KubeletConfig["--max-pods"]) | |||
| var n []node.Node | |||
| n, err = node.GetByRegexWithRetry(fmt.Sprintf("^k8s-%s", profile.Name), 3*time.Minute, cfg.Timeout) | |||
There was a problem hiding this comment.
So now we're just using profile.Count instead of actually fetching nodes to find the maxTotalPods. Is this because of Windows, and was there value in making the API call to get the nodes?
There was a problem hiding this comment.
Right, it's because the prior regex would fail (Windows nodes have a different name).
The purpose of this enumeration is to set an appropriate number of pod replicas that exceeds the pods-per nodes setting across all nodes (so that we induce quick scale up by creating pods that are unable to be scheduled). Doing the node count by calling the k8s api at runtime is more strictly accurate, but with Windows nodes in the mix, got more complicated: basically correlating the nodes in the cluster with a particular pool is trickier. All that being said, the theoretical benefits of doing this over simply taking the Count value in the api model seemed to be not worth wrangling this complexity.
Yep |
jackfrancis
force-pushed
the
e2e-merge-tests
branch
from
0e47211
to
17eb407
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, marosset 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:
Approvers can indicate their approval by writing |
Reason for Change:
Rather than test Windows by creating entirely new clusters, let's just add a Windows node pool to every cluster configuration we test across Kubernetes versions.
This PR adds a Windows node pool to the "standard" cluster configuration we use for our current "Linux" Kubernetes version tests, effectively making them OS-agnostic (or more appropriately: OS-generic, as they will cover both OS flavors).
This PR also removes 1.11 and 1.12 from our PR test matrix to reflect that 1.13-->1.17 are the versions of Kubernetes that are actively under development upstream.
Fewer tests for the same amount of coverage!
Issue Fixed:
Requirements:
Notes: