Reenable AzureFile tests for Windows on K8s 1.11.1, resolves #3439 #3496

PatrickLang · 2018-07-17T22:20:36Z

Kubernetes v1.11.1 is out, including the azurefiles fixes. This fixes the skipped test case to only skip v1.11.0 and run on other versions.

codecov · 2018-07-17T22:57:57Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3496      +/-   ##
==========================================
+ Coverage   55.47%   55.89%   +0.41%     
==========================================
  Files         105      105              
  Lines       16004    15943      -61     
==========================================
+ Hits         8879     8911      +32     
+ Misses       6375     6282      -93     
  Partials      750      750

PatrickLang · 2018-07-18T18:31:14Z

• Failure [1801.962 seconds]
Azure Container Cluster using the Kubernetes Orchestrator
/go/src/github.com/Azure/acs-engine/test/e2e/kubernetes/kubernetes_test.go:59
  with a windows agent pool
  /go/src/github.com/Azure/acs-engine/test/e2e/kubernetes/kubernetes_test.go:724
    should be able to attach azure file [It]
    /go/src/github.com/Azure/acs-engine/test/e2e/kubernetes/kubernetes_test.go:819

    Expected error:
        <*errors.fundamental | 0xc42000c7e0>: {
            msg: "Timeout exceeded (30m0s) while waiting for PersistentVolumeClaims (pvc-azurefile) to become ready",
            stack: [0x6e9414, 0x45bed1],
        }
        Timeout exceeded (30m0s) while waiting for PersistentVolumeClaims (pvc-azurefile) to become ready
    not to have occurred

The version check isn't working as expected. Will add unit tests next to figure it out. I would expect this test case to be skipped on v1.11.0

PatrickLang · 2018-07-20T02:07:11Z

@CecileRobertMichon thanks for helping me figure out the right way to check versions. Can you do a review?

CecileRobertMichon · 2018-07-20T17:16:09Z

pkg/api/common/versions.go

@@ -351,6 +351,13 @@ func IsKubernetesVersionGe(actualVersion, version string) bool {
 	return v1.GE(v2)
 }

+// IsKubernetesVersionEq returns true if actualVersion is equal to version
+func IsKubernetesVersionEq(actualVersion, version string) bool {


why not just compare the strings for this?

~~If you decide to keep it, it'd be an easy unit test to add in versions_test.go~~ --> you already did that nevermind!

Originally I was hoping the semantic versioning library could handle some cases like "1.11" == "1.11.0" . It doesn't handle that case so I should probably keep it simple and remove it.

CecileRobertMichon · 2018-07-20T17:18:21Z

test/e2e/kubernetes/kubernetes_test.go

-					// Failure in 1.11+ - https://github.com/kubernetes/kubernetes/issues/65845
-					Skip("Kubernetes 1.11 has a known issue creating Azure PersistentVolumeClaims")
-				} else if common.IsKubernetesVersionGe(eng.ClusterDefinition.ContainerService.Properties.OrchestratorProfile.OrchestratorVersion, "1.8") {
+				if common.IsKubernetesVersionEq(eng.ExpandedDefinition.Properties.OrchestratorProfile.OrchestratorVersion, "1.11.0") {


This could be if eng.ExpandedDefinition.Properties.OrchestratorProfile.OrchestratorVersion == "1.11.0" {

Agreed - I wasn't sure if OrchestratorVersion could be "1.11", but it's showing up as "1.11.0". The string comparison is sufficient here so I'll keep it simple

Simplifying to ==

PatrickLang · 2018-07-20T18:56:10Z

@CecileRobertMichon thanks. Simplified per feedback and rebased.

CecileRobertMichon · 2018-07-20T18:59:50Z

test/e2e/kubernetes/kubernetes_test.go

+				if eng.ExpandedDefinition.Properties.OrchestratorProfile.OrchestratorVersion == "1.11.0" {
+					// Failure in 1.11.0 - https://github.com/kubernetes/kubernetes/issues/65845, fixed in 1.11.1
+					Skip("Kubernetes 1.11.0 has a known issue creating Azure PersistentVolumeClaims")
+				} else if common.IsKubernetesVersionGe(eng.ExpandedDefinition.Properties.OrchestratorProfile.OrchestratorVersion, "1.8") {


I'm not sure "1.8" is a valid version, I think you need "1.8.0"

Ah yes you're right.

CecileRobertMichon

/lgtm

acs-bot · 2018-07-20T21:28:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, PatrickLang

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:

~~OWNERS~~ [CecileRobertMichon]

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

* 'master' of https://github.com/Azure/acs-engine: (59 commits) Docs: Update user guide list to include Windows, update description of clusters (Azure#3473) update to Azure CNI v1.0.10 (Azure#3551) Adding 'make dev' equivalent for Windows (Azure#3471) print out ubuntu ver in e2e (Azure#3555) fix an issue where networkPlugin was not defined correctly when using calico or cilium (Azure#3271) Bump ginkgo to a tagged release (Azure#3554) Reenable AzureFile tests for Windows on K8s 1.11.1, resolves Azure#3439 (Azure#3496) removing rbac error checking from merge fn (Azure#3530) Change dns healthcheck to look at external domain (Azure#3282) DOCUMENTATION: Fix Documented Default Value for clusterSubnet (Azure#3474) Document required manual calico 2.6.3 -> calico 3.1.1 upgrade when upgrading from < 0.17.0-provisioned clusters (Azure#3208) revert --image-pull-policy=IfNotPresent for win (Azure#3553) --image-pull-policy=IfNotPresent for kubectl run commands (Azure#3552) Kubernetes: --max-pods=30 should be Azure CNI-only (Azure#3543) disable Azure CNI network monitor addon default (Azure#3550) only do az vm list for k8s (Azure#3540) Retire Swarm E2E for PR test coverage (Azure#3539) retire Azure CDN for container image repository proxying (Azure#3535) removed datadisk to allow scale after upgrade (Azure#3482) Pump k8s-azure-kms version (Azure#3531) ...

* master: (59 commits) Docs: Update user guide list to include Windows, update description of clusters (Azure#3473) update to Azure CNI v1.0.10 (Azure#3551) Adding 'make dev' equivalent for Windows (Azure#3471) print out ubuntu ver in e2e (Azure#3555) fix an issue where networkPlugin was not defined correctly when using calico or cilium (Azure#3271) Bump ginkgo to a tagged release (Azure#3554) Reenable AzureFile tests for Windows on K8s 1.11.1, resolves Azure#3439 (Azure#3496) removing rbac error checking from merge fn (Azure#3530) Change dns healthcheck to look at external domain (Azure#3282) DOCUMENTATION: Fix Documented Default Value for clusterSubnet (Azure#3474) Document required manual calico 2.6.3 -> calico 3.1.1 upgrade when upgrading from < 0.17.0-provisioned clusters (Azure#3208) revert --image-pull-policy=IfNotPresent for win (Azure#3553) --image-pull-policy=IfNotPresent for kubectl run commands (Azure#3552) Kubernetes: --max-pods=30 should be Azure CNI-only (Azure#3543) disable Azure CNI network monitor addon default (Azure#3550) only do az vm list for k8s (Azure#3540) Retire Swarm E2E for PR test coverage (Azure#3539) retire Azure CDN for container image repository proxying (Azure#3535) removed datadisk to allow scale after upgrade (Azure#3482) Pump k8s-azure-kms version (Azure#3531) ...

ghost assigned PatrickLang Jul 17, 2018

ghost added the in progress label Jul 17, 2018

PatrickLang added the do-not-merge/hold label Jul 18, 2018

PatrickLang force-pushed the plang-azurefile-1.11.1 branch 4 times, most recently from 3c62d49 to b5c5474 Compare July 19, 2018 23:51

PatrickLang removed the do-not-merge/hold label Jul 19, 2018

CecileRobertMichon reviewed Jul 20, 2018

View reviewed changes

PatrickLang force-pushed the plang-azurefile-1.11.1 branch from b5c5474 to ccca36f Compare July 20, 2018 18:53

Reenable AzureFile tests for Windows on K8s 1.11.1, resolves Azure#3439

50534ba

Simplifying to ==

PatrickLang force-pushed the plang-azurefile-1.11.1 branch from ccca36f to 50534ba Compare July 20, 2018 18:55

CecileRobertMichon reviewed Jul 20, 2018

View reviewed changes

fix version 1.8 check

1507b38

acs-bot assigned CecileRobertMichon Jul 20, 2018

CecileRobertMichon approved these changes Jul 20, 2018

View reviewed changes

acs-bot added the lgtm label Jul 20, 2018

acs-bot added the approved label Jul 20, 2018

jackfrancis merged commit b2d20f9 into Azure:master Jul 25, 2018

ghost removed the in progress label Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reenable AzureFile tests for Windows on K8s 1.11.1, resolves #3439 #3496

Reenable AzureFile tests for Windows on K8s 1.11.1, resolves #3439 #3496

PatrickLang commented Jul 17, 2018

codecov bot commented Jul 17, 2018 •

edited

Loading

PatrickLang commented Jul 18, 2018 •

edited

Loading

PatrickLang commented Jul 20, 2018

CecileRobertMichon Jul 20, 2018 •

edited

Loading

PatrickLang Jul 20, 2018

CecileRobertMichon Jul 20, 2018

PatrickLang Jul 20, 2018

PatrickLang commented Jul 20, 2018

CecileRobertMichon Jul 20, 2018

PatrickLang Jul 20, 2018

CecileRobertMichon left a comment

acs-bot commented Jul 20, 2018

Reenable AzureFile tests for Windows on K8s 1.11.1, resolves #3439 #3496

Reenable AzureFile tests for Windows on K8s 1.11.1, resolves #3439 #3496

Conversation

PatrickLang commented Jul 17, 2018

codecov bot commented Jul 17, 2018 • edited Loading

Codecov Report

PatrickLang commented Jul 18, 2018 • edited Loading

PatrickLang commented Jul 20, 2018

CecileRobertMichon Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

PatrickLang Jul 20, 2018

Choose a reason for hiding this comment

CecileRobertMichon Jul 20, 2018

Choose a reason for hiding this comment

PatrickLang Jul 20, 2018

Choose a reason for hiding this comment

PatrickLang commented Jul 20, 2018

CecileRobertMichon Jul 20, 2018

Choose a reason for hiding this comment

PatrickLang Jul 20, 2018

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

acs-bot commented Jul 20, 2018

codecov bot commented Jul 17, 2018 •

edited

Loading

PatrickLang commented Jul 18, 2018 •

edited

Loading

CecileRobertMichon Jul 20, 2018 •

edited

Loading