Windows dns connectivity - e2e tests

[jsturtevant]

What this PR does / why we need it:
adds unit tests for e2e scenarios in windows containers. From issue #3632, specifically:

• Windows to Windows network connectivity via DNS
• Windows to Linux network connectivity via DNS
• Linux to Windows network connectivity via DNS

Special notes for your reviewer:
I am currently getting error:: I was getting this error due to kubectl version (I had 1.11)

 Error trying to deploy iis-dns-kubernetes-eastus-55793-65288 [microsoft/iis:windowsservercore-1803] in namespace default:error: v1beta1.Deployment is not suitable for converting to "apps/v1beta1"

@PatrickLang This appears to be related to 1.11. Is it ok to move from apiVersion": "extensions/v1beta1 in the windows deploy script to a newer apiversion?

If applicable:

Release note:

[codecov]

Codecov Report

Merging #3760 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3760      +/-   ##
==========================================
+ Coverage   55.66%   55.71%   +0.04%     
==========================================
  Files         109      109              
  Lines       16207    16207              
==========================================
+ Hits         9022     9030       +8     
+ Misses       6399     6386      -13     
- Partials      786      791       +5

[jsturtevant]

The line of code that has the older version set is:

acs-engine/test/e2e/kubernetes/deployment/deployment.go

Line 104 in f019949

overrides := `{ "apiVersion": "extensions/v1beta1", "spec":{"template":{"spec": {"nodeSelector":{"beta.kubernetes.io/os":"windows"}}}}}`

I found that the kubectl version affects if the test will pass. I was running 1.11 yesterday when I got the error above, on my other dev box I was running kubectl 1.9 and do not have the error.

I assume at some point we will run into this during the e2e tests as we move up versions. I found that I didn't need to pass the schema when creating a pod when writing tests for this pr here: https://github.com/jsturtevant/acs-engine/blob/1b5dd2353ef94350e7cc41fea887e08a0f8074b0/test/e2e/kubernetes/pod/pod.go#L141.

Removing the schema might be easy way to make the tests work across versions.

@jackfrancis @CecileRobertMichon any thoughts on the version for the deployments?

[jsturtevant]

A few notes for this PR:

Stability iterations

STABILITY_ITERATIONS does not seem to be set on the CI builds. Looking at the last few builds in Circle CI I am seeing this set to 0. When I enabled it locally I found a bug on the DNS checks recently added by @jackfrancis. I've fixed and used the same pattern for checking the DNS on windows. During that I refactored some common code (the RunCommandMultipleTimes) as it was being used in 4+ places.

@jackfrancis I can add the STABILITY_ITERATIONS to the CircleCI config.yml file. What is the target you would like?

Api versions removed

I've also removed apiversion checks on in the deployment.go file to fix kubectl errors mentioned above. This should resolve flaky tests based on kubectl version as mentioned above.

Windows DNS notes

@PatrickLang During testing I ran into two interesting things in windows DNS:

1. nslookup <svc-name> does not resolve but nslookup <svc-name>.default.svc.cluster.local does. I ended up using powershell iwr as it was used elsewhere and i figured it more likely covers developer use case of calling HTTP call on service.
2. Using curl in 1803 image as a command would fail. powershell iwr works. For instance:

# doesn't work
kubectl run windows-1803-curl -n default --image microsoft/windowsservercore:1803 --image-pull-policy=IfNotPresent --restart=Never --overrides '{ "spec": {"nodeSelector":{"beta.kubernetes.io/os":"windows"}}}' --command -- curl nginx-dns-kubernetes-eastus-21376-93545

# works
kubectl run windows-1803-ps -n default --image microsoft/windowsservercore:1803 --image-pull-policy=IfNotPresent --restart=Never --overrides '{ "spec": {"nodeSelector":{"beta.kubernetes.io/os":"windows"}}}' --command -- powershell iwr -UseBasicParsing -TimeoutSec 60 nginx-dns-kubernetes-eastus-21376-93545

[jackfrancis]

@jsturtevant thanks for this!

How many iterations did you need to fall into the bug you mentioned? It was set to zero intentionally for PR E2E tests so as not to suddenly introduce already existing transient flakiness that would block PR signal. But if the number is low (10?) we can consider turning that on in the near term.

[jsturtevant]

@jackfrancis The bug was related to having windows nodes in the cluster. The linux only tests where being scheduled onto the windows node and failing.

If STABILITY_ITERATIONS is disabled for PRs that sounds reasonable. Though, I didn't see the STABILITY_ITERATIONS being set for build_and_test_master test runs either. Is the expectation it is being run there?

I think a low number (5? or 10?) would be fine to start with and we could adjust as needed.

[jsturtevant]

@PatrickLang Could you give a review? Thanks!

[PatrickLang]

Overall I think this is a good improvement. I had a few small suggestions - feel free to make them if you agree, otherwise we can merge without them.

Can you do a quick rebase? I'm sorry I didn't get to this before someone checked in a conflicting change.

[PatrickLang]

any reason radial/busyboxplus is needed? wget could be used instead with _/busybox

[jsturtevant]

I've updated back to using the alpine images the rest of tests use with wget.

[PatrickLang]

if you're suppressing output for I > 1 at line 200, do you want to suppress it here too?

[jsturtevant]

This was from the original implementation. I could go either way. Seems like we might want to see details if it fails and grabbing logs each round keeps the code simpler. I'll leave it for now until we find it is to much

[PatrickLang]

I think this would be good to defer logging the summary to consistently log how many iterations were attempted, desired, and succeeded even on failure. It would be good to know if there's a pattern that leads to failing at N of M attempts. On a failed run it won't say how many attempts were made, just that one of them failed

[jsturtevant]

Great idea. I have updated it to defer and log the successful attempts, actual attempts, and desired attempts

[jsturtevant]

@PatrickLang I did a rebase to pull in the changes and addressed your questions/comments. I think this is ready for final review.

One final question I have is the setting for STABILITY_ITERATIONS. It does not appear to be set for any of the builds. If we don't get that set then none of this will be called. Starting out with a small number seems to make sense to see how often we run into flaky tests. I have been using a setting of 5 for my tests and haven't had any trouble.

[PatrickLang]

Sorry for the delay here - have been behind the last few days working on plans for kubernetes v1.13 + Windows. Looks good, launching last tests

[jsturtevant]

@PatrickLang it looks like it failed on the should autoscale test. I think it is a flaky test?

[PatrickLang]

/LGTM

I restarted the failed test. I think it's flaky

[PatrickLang]

@jackfrancis - is this a known test flake?

Azure Container Cluster using the Kubernetes Orchestrator
/go/src/github.com/Azure/acs-engine/test/e2e/kubernetes/kubernetes_test.go:63
  with a linux agent pool
  /go/src/github.com/Azure/acs-engine/test/e2e/kubernetes/kubernetes_test.go:581
    should be able to autoscale [It]
    /go/src/github.com/Azure/acs-engine/test/e2e/kubernetes/kubernetes_test.go:624

    Expected error:
        <*errors.fundamental | 0xc0000fc5a0>: {
            msg: "Timeout exceeded (30m0s) while waiting for 2 Pod replicas from Deployment php-apache-kubernetes-southeastasia-31965-41004",
            stack: [0x70c22b, 0x45da91],
        }
        Timeout exceeded (30m0s) while waiting for 2 Pod replicas from Deployment php-apache-kubernetes-southeastasia-31965-41004
    not to have occurred

[jackfrancis]

@PatrickLang we do see HPA being the highest rate of flake. I assume there's something there, just haven't root caused it yet.

[jsturtevant]

@PatrickLang did the test run again or is that the failure from before?

[PatrickLang]

/approve

[PatrickLang]

/assign @tariq1890

Can you merge? I restarted the failed test but don't think its related.

[tariq1890]

@PatrickLang Can you check the latest failures? Pretty sure they are flakes, but i'd like to confirm. Also deferring this one to @jackfrancis as e2e's are not my forte :)

[jsturtevant]

The failure on 1.9 is the cse timeout. I believe that is a flake issue.

The failure on 1.11 is a timeout on provisioning external the IP addresses. I haven't seen that one before. It looks like it failed for both the windows and linux tests. Are there any known issues in 1.11 for provisioning external IP's?

[jackfrancis]

Both are possible flakes, re-running.

[jsturtevant]

Looks like they were flakes and this is ready to go 👍

[tariq1890]

/lgtm

[acs-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsturtevant, PatrickLang, tariq1890

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [tariq1890]

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

[jackfrancis]

Thanks @jsturtevant!