-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
Added e2e test for HA master replicas in different zones. #34539
Conversation
Jenkins GCE e2e failed for commit 0056bf6. Full PR test history. The magic incantation to run this job again is |
@@ -16,6 +16,9 @@ | |||
|
|||
KUBE_ROOT=$(dirname "${BASH_SOURCE}")/../.. | |||
|
|||
if [[ ! -z "${1:-}" ]]; then | |||
export GCE_ZONE="${1}" |
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.
Variable GCE_ZONE
is only used (according to grep
) in test/e2e_node/jenkins
. Why are you setting it here? I don't understand.
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.
Fixed: changed to KUBE_GCE_ZONE
@@ -75,26 +76,61 @@ func verifyNumberOfMasterReplicas(expected int) { | |||
} | |||
} | |||
|
|||
func findRegionForZone(zone string) string { |
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.
This can be done easier:
$ gcloud compute zones list asia-east1-a --format="[no-heading](region)"
asia-east1
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.
done
return "" // unreachable | ||
} | ||
|
||
func findZonesForRegion(region string) []string { |
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.
Nit: instead of assuming that a zone is prefixed by region, you can explicitly filter by region:
$ gcloud compute zones list --filter="region=asia-east1" --format="[no-heading](name)"
asia-east1-a
asia-east1-b
asia-east1-c
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.
done
return zones | ||
} | ||
|
||
func removeZoneFromZones(zones []string, zone string) []string { |
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.
Instead of implementing this on your own you can also use sets.String
implemented in pkg/util/sets
or use a map instead where you could just delete a key.
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.
We cannot do it as additionalReplicaZones is a mult-map: if numAdditionalReplicas is larger then the number of remaining zones in the region, we create a few masters in the same zone and zone entry is repeated in additionalReplicaZones.
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.
Can you add such comment, that it can be a multimap. It was not clear when I was reading the code.
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.
Done.
0056bf6
to
3064f9b
Compare
@fgrzadkowski |
Jenkins GCI GKE smoke e2e failed for commit 3064f9b9009f05640158613eea6fcf4b2d7562af. Full PR test history. The magic incantation to run this job again is |
3064f9b
to
f3f76a7
Compare
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.
LGTM. Just add a small comment and feel free to apply LGTM label afterwards.
return zones | ||
} | ||
|
||
func removeZoneFromZones(zones []string, zone string) []string { |
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.
Can you add such comment, that it can be a multimap. It was not clear when I was reading the code.
f3f76a7
to
e0bb08e
Compare
Added e2e test for HA master replicas in different zones.
e0bb08e
to
8266f55
Compare
Jenkins unit/integration failed for commit 8266f55. Full PR test history. The magic incantation to run this job again is |
Jenkins GCE Node e2e failed for commit 8266f55. Full PR test history. The magic incantation to run this job again is |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
Part of kubernetes/enhancements#48 |
Added e2e test for HA master replicas in different zones.
This change is![Reviewable](https://camo.githubusercontent.com/2d899f4291d07d3cd2fa4aaae1e3b243f164c23fce87d30a589ace0d496a444c/68747470733a2f2f72657669657761626c652e6b756265726e657465732e696f2f7265766965775f627574746f6e2e737667)