E2E: rationalize node check + kube-system check, no kms#33
E2E: rationalize node check + kube-system check, no kms#33jackfrancis merged 20 commits intoAzure:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #33 +/- ##
==========================================
- Coverage 47.55% 46.89% -0.67%
==========================================
Files 91 91
Lines 13974 13800 -174
==========================================
- Hits 6646 6471 -175
Misses 6721 6721
- Partials 607 608 +1 |
|
/lgtm |
test/e2e/config/config.go
Outdated
| UseDeployCommand bool `envconfig:"USE_DEPLOY_COMMAND"` | ||
| GinkgoFocus string `envconfig:"GINKGO_FOCUS"` | ||
| GinkgoSkip string `envconfig:"GINKGO_SKIP"` | ||
| NewCluster bool `envconfig:"NEW_CLUSTER" default:"false"` // track when we create a new cluster via E2E |
There was a problem hiding this comment.
isn't this equivalent to Name == ""?
There was a problem hiding this comment.
nevermind, this is needed because you are using it after Name is set. I don't think we should make it an envconfig var though since its value will be set in https://github.com/Azure/aks-engine/pull/33/files#diff-881ae33659acba7c8c55e6efbf5edd3dR107 with os.Setenv("NEW_CLUSTER", "true"). This isn't a configurable input but a variable determined by other values.
There was a problem hiding this comment.
I'm pretty sure that's the only way that the env config parser works. It has a whitelist of env vars to parse and turn into a typed struct.
There was a problem hiding this comment.
it can follow the same pattern as CurrentWorkingDir
| "containers": [ | ||
| { | ||
| "name": "tiller", | ||
| "image": "gcr.io/kubernetes-helm/tiller:v2.8.1", |
There was a problem hiding this comment.
Curious. Why are we doing this? Defaults should populate the exact same thing
jackfrancis
changed the title
| } | ||
|
|
||
| if config.EnableKMSEncryption && config.ClientObjectID != "" { | ||
| if prop.OrchestratorProfile.KubernetesConfig == nil { |
There was a problem hiding this comment.
this is a nil panic vector in e2e runner if no kubernetesConfig object is present in input api model
jackfrancis
changed the title
|
Filed #49 to mark KMS as buggy |
| @@ -74,14 +74,22 @@ type List struct { | |||
| // AreAllReady returns a bool depending on cluster state | |||
| func AreAllReady(nodeCount int) bool { | |||
There was a problem hiding this comment.
The purpose of these changes is to make this func more scrupulous: instead of "looking for specific failure conditions and returning true otherwise" we are now "looking for specific success conditions and returning false otherwise".
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon, jackfrancis, tariq1890 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
What this PR does / why we need it: An active "intermittent zero byte
/etc/kubernetes/azure.jsonon master nodes issue" reveals that we aren't properly (1) testing for the integrity of this cloudprovider file, and (2) our E2E tests are implemented in a way that masks this.This PR re-arranges node readiness checks so that they more clearly communicate the state of nodes before testing, and during the E2E run.
We haven't root caused the azure.json issue, but we have repro'd it in isolation w/ KMS encryption enabled, so we're disabling that feature on standard cluster definitions.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Testing:
Release note: