Adding capability for HA control plane testing with kops #6113

chrislovecnm · 2017-12-31T01:57:18Z

Added a new flag for kops-ha. Also, since we now may run in regions that
may not have m3 instances, updated the master type to c4 instances.

In order to test HA control plain a minimum of 3 AZs must exist.
Implemented a func that queries all regions, and then queries the list
of that regions AZ. A random region is then selected. If running in ha
mode, a region with a minimum of 3 AZ is selected. The new zone
capability can be run in both ha and non-ha modes. If --kops-zones is
not set, and the test run is on AWS, the new code is used. You can
override the new code by setting the zone as done previously.

Added a unit test, that requires a live AWS account for testing the new
random region selection. These tests are skipped since they require a
live AWS account.

chrislovecnm · 2017-12-31T01:58:05Z

/cc @geojaz @justinsb
/assign @krzyzacy

chrislovecnm · 2017-12-31T02:31:03Z

/hold

Need to do e2e test with this

krzyzacy · 2018-01-09T19:40:52Z

kubetest/kops.go

+	var zones []string
+
+	// if zones is set to zero and gcp project is not set then pick random aws zone
+	if *kopsZones == "" && gcpProject == "" {


why check for gcpProject? gce vs aws? maybe check for provider here

krzyzacy · 2018-01-09T19:41:19Z

kubetest/kops.go

 	}

+	if len(zones) == 0 {
+		return nil, fmt.Errorf("no zones found")


errors.New, same as below

krzyzacy · 2018-01-09T19:44:03Z

kubetest/kops.go

 		"--zones", strings.Join(k.zones, ","),
+		"--override", "'cluster.spec.etcdClusters[*].version=3.1.10'",


are we pinning etcd version for all the kops job now? (does etcd 3.1 work with older releases?)

good point, what versions are we testing?? Do I have access to the version number for the test run?

krzyzacy · 2018-01-09T19:52:49Z

kubetest/kops.go

+		}
+	}
+
+	// TODO get the number of ec2 instances in the region and ensure that there are not too many running


nit: TODO(chrislovecnm): blah blah

Oh - that would be cool. Having something like boskos that acts as a mutex. I wonder if we could "fake it" with tags or some cheap resource somehow...

krzyzacy · 2018-01-09T19:53:05Z

/lint

k8s-ci-robot

@krzyzacy: 0 warnings.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

justinsb · 2018-01-22T21:48:50Z

kubetest/kops.go

 	kopsDiskSize     = flag.Int("kops-disk-size", 48, "Disk size to use for nodes and masters")
+	kopsHA           = flag.Bool("kops-ha", false, "(kops only) Run HA control plane")


Make thiskops-master-count and have it pass through to --master-count ?

Or kops-multizone - as that's closer to what it is doing - picking distinct zones in the same region?

It is kops-master-count, not multizone :) Yes I can do that

justinsb · 2018-01-22T21:49:12Z

kubetest/kops.go

 	kopsDiskSize     = flag.Int("kops-disk-size", 48, "Disk size to use for nodes and masters")
+	kopsHA           = flag.Bool("kops-ha", false, "(kops only) Run HA control plane")
+	kopsEtcd         = flag.String("kops-etcd", "", "(kops only) Etcd Version")


Needed in this PR?

And perhaps rename to kops-etcd-version or should we just expose the override flag?

I can move this to another PR if you like, but we need it for HA testing in general

justinsb · 2018-01-22T21:50:14Z

kubetest/kops.go

+	ha bool
+
+	// kopsEtcd is the etcd version to run
+	kopsEtcd string


No need for kops prefix here (in struct kops). And I suggest etcdVersion

justinsb · 2018-01-22T21:50:47Z

kubetest/kops.go

+		zones = *z
+	} else {
+		zones = strings.Split(*kopsZones, ",")
+		if err := os.Setenv("ZONE", zones[0]); err != nil {


I take it this is no longer needed, as it isn't done in the other branch of the if statement. Remove (or add to the other if statement branch)?

I frankly have no bloody idea wth this is doing, I refactored it.

OK, then the problem is that it has to be done in both branches. Pull it out of the if statement?

justinsb · 2018-01-22T21:51:32Z

kubetest/kops.go

+
+	if len(zones) == 0 {
+		return nil, errors.New("no zones found")
+	} else if zones[0] == "" {


Validate all zones in slice, not just the first?

this is refactoring of old code. I can do that in another PR.

I don't see it in the diff? Can you point me to it?

justinsb · 2018-01-22T21:52:39Z

kubetest/kops.go

+		}
+		zones = *z
+	} else {
+		zones = strings.Split(*kopsZones, ",")


I think the validation error message should be "kops-zones must be specified when not running on AWS"

again old code, we can follow-up with another PR if you like.

justinsb · 2018-01-22T22:03:02Z

kubetest/kops.go

+		}
+	}
+
+	// TODO get the number of ec2 instances in the region and ensure that there are not too many running


Oh - that would be cool. Having something like boskos that acts as a mutex. I wonder if we could "fake it" with tags or some cheap resource somehow...

justinsb · 2018-01-22T22:03:24Z

kubetest/kops.go

+// getRandomAWSZones looks up all regions, and the availability zones for those regions.  A random
+// region is then chosen and the AZ's for that region is returned.  If HA is required a regions with 3
+// or more zones is returned.
+func getRandomAWSZones(isHA bool) (*[]string, error) {


Return []string, error

why? Would love to understand more.

justinsb · 2018-01-22T22:04:28Z

kubetest/kops.go

+		if err != nil {
+			return nil, err
+		}
+		zones = *z


I'd have getRandomAWSZones return []string, and just check that if len(zones) == 0) { return nil, errors.New("could not pick zones") } or similar

hrmmm I get what you are saying, but I would rather check for an error. That is why I made it a pointer. Can you live with it?

justinsb · 2018-01-22T22:05:34Z

kubetest/kops_test.go

+)
+
+func Test_GetHARandomRegion(t *testing.T) {
+	t.Skip("this test requires a valid aws account to run")


If you wanted to write a test, you could split getRandomAWSZones into two functions - one that builds the map, the other that returns a list of zones with at least N members

justinsb · 2018-01-22T22:05:56Z

kubetest/kops.go

+	// TODO get the number of ec2 instances in the region and ensure that there are not too many running
+	log.Printf("Launching cluster in region: %q", selectedRegion)
+
+	return &selectedZones, nil


What if selectedZones > 3 ?

Then we use all of them.

justinsb · 2018-01-22T22:08:47Z

kubetest/kops.go

+	}
+	if len(overrides) != 0 {
+		featureFlags = append(featureFlags, "SpecOverrideFlag")
+		createArgs = append(createArgs, "--override", strings.Join(overrides, ","))


Don't join I don't think - not sure it's valid

need context

justinsb · 2018-01-26T04:23:08Z

So I will counterpose something. How about we have a blacklist of regions, so that when US-Colorado opens we blacklist it? Also, when a region gets its third AZ it will automatically start running HA tests. I would rather know which regions are having issues. Just a thought.

I'm fine with that. There will be a lag when they announce a new region, but that's not a big deal.

justinsb · 2018-01-26T04:30:12Z

For the --overrides: I'm not sure we accept --override=a,b in kops; I think you have to do --override=a --override=b. Am I wrong?

justinsb · 2018-01-26T04:10:55Z

kubetest/kops.go

+
+	if len(zones) == 0 {
+		return nil, errors.New("no zones found")
+	} else if zones[0] == "" {


I don't see it in the diff? Can you point me to it?

justinsb · 2018-01-26T04:11:55Z

kubetest/kops.go

+		zones = *z
+	} else {
+		zones = strings.Split(*kopsZones, ",")
+		if err := os.Setenv("ZONE", zones[0]); err != nil {


OK, then the problem is that it has to be done in both branches. Pull it out of the if statement?

justinsb · 2018-01-26T04:12:53Z

kubetest/kops.go

@@ -273,17 +309,19 @@ func (k kops) Up() error {
 		"--node-count", strconv.Itoa(k.nodes),
 		"--node-volume-size", strconv.Itoa(k.diskSize),
 		"--master-volume-size", strconv.Itoa(k.diskSize),
+		// TODO chrislovecnm make this configurable
+		"--master-size", "c4.large",


We override this in some tests (the ENA test IIRC), so this should be a flag if you want to default it to c4.large

justinsb · 2018-01-26T04:14:44Z

kubetest/kops.go

+		// az for each of the regions. AWS Go API does not allow us to make a single call
+		zonesResults, err := ec2Session.DescribeAvailabilityZones(&ec2.DescribeAvailabilityZonesInput{})
+		if err != nil {
+			return nil, fmt.Errorf("unable to call aws api DescribeAvailabilityZones: %v", err)


I don't think building the session is going to fail, it's going to be on your first "real" API call - i.e. here

justinsb · 2018-01-26T04:15:07Z

kubetest/kops.go

+		}
+		// build a map of region and the regions az so that we can lookup AZ easily
+		for _, z := range zonesResults.AvailabilityZones {
+			existing, ok := regionMap[*z.RegionName]


This can replace the whole block of the for loop

justinsb · 2018-01-26T04:16:14Z

kubetest/kops.go

+	}
+
+	// initialize global pseudo random generator
+	rand.Seed(time.Now().Unix())


You normally only call rand.Seed once, at the very start of your program (i.e. in main). Many languages don't even require you to call seed at all. It's unusual to seed repeatedly.

So - why are you seeding repeatedly is more the question :-)

justinsb · 2018-01-26T04:17:34Z

kubetest/kops_test.go

+/*
+Per @justinsb
+TODO @chrislovecnm
+If you wanted to write a test, you could split getRandomAWSZones into two functions - one that builds the map, the other that returns a list of zones with at least N members


I just don't really see the point of these skipped test functions. We're going to find out PDQ if it doesn't work...

justinsb · 2018-01-26T04:18:39Z

kubetest/kops.go

 	kopsDiskSize     = flag.Int("kops-disk-size", 48, "Disk size to use for nodes and masters")
+	kopsMasterCount  = flag.Int("kops-master-count", 1, "(kops only) Number of masters to run")
+	kopsEtcdVersion  = flag.String("kops-etcd", "", "(kops only) Etcd Version")


Change flag to kops-etcd-version?

justinsb · 2018-01-26T04:20:46Z

kubetest/kops.go

+// getRandomAWSZones looks up all regions, and the availability zones for those regions.  A random
+// region is then chosen and the AZ's for that region is returned.  If HA is required a regions with 3
+// or more zones is returned.
+func getRandomAWSZones(masterCount int) (*[]string, error) {


I was unclear before - I suggest this should return ([]string, error), because []string is already a ref type, so it's already nullable.

justinsb · 2018-01-26T04:27:19Z

kubetest/kops.go

+	// initialize global pseudo random generator
+	rand.Seed(time.Now().Unix())
+
+	// select a random region and zones


You asked for more context on using rand.Perm.

Instead of looping through and removing the regions one by one, instead just use rand.Perm to get a list of indexes. Those are the random order in which you will consider the regions in the slice. Then you don't need to do the removal, and you only need one rand call.

for _, i := rand.Perm(n) { region := regions[i] ... } return nil, fmt.Errorf("unable to find region with %d zones", masterCount)

chrislovecnm · 2018-02-02T22:34:32Z

For the --overrides: I'm not sure we accept --override=a,b in kops; I think you have to do --override=a --override=b. Am I wrong?

I will test, but I think it is ok either way

Otherwise @justinsb PTAL

chrislovecnm · 2018-02-02T22:48:39Z

We cannot merge this until we get the AWS quotas addressed.

justinsb · 2018-02-03T02:20:20Z

kubetest/kops.go

 	kopsDiskSize     = flag.Int("kops-disk-size", 48, "Disk size to use for nodes and masters")
 	kopsPublish      = flag.String("kops-publish", "", "Publish kops version to the specified gs:// path on success")
+	kopsMasterType   = flag.String("kops-master-type", "c4.large", "(kops only) master instance type")
+	kopsMasterCount  = flag.Int("kops-master-count", 1, "(kops only) Number of masters to run")
+	kopsEtcdVersion  = flag.String("kops-etcd", "", "(kops only) Etcd Version")


Flag name should be kops-etcd-version I think

justinsb · 2018-02-03T02:21:20Z

kubetest/kops.go

+	if *kopsZones == "" && provider == "aws" {
+		// Initialize global pseudo random generator.
+		// We do not need to intialize it if kopsZones is passed in or we are running the other tests.
+		// That is why I did not put it in main func.


I think it should go into the main func, it's something that should be done once in every program and then we forget about it

justinsb · 2018-02-03T02:22:35Z

kubetest/kops.go

+	if len(zones) == 0 {
+		return nil, errors.New("no zones found")
+	} else if zones[0] == "" {
+		return nil, errors.New("zone cannot be a empty string")


Can we validate all the zones?

A really good idea, but do you mind if we do it in another PR? I am wondering if that work is outside the scope of this PR.

Also, my plan is to not pass in zones long term. Always pick a random zone or zones with the new func. Because of this zones would be chosen programmatically.

justinsb · 2018-02-03T02:23:05Z

kubetest/kops.go

 	kopsDiskSize     = flag.Int("kops-disk-size", 48, "Disk size to use for nodes and masters")
 	kopsPublish      = flag.String("kops-publish", "", "Publish kops version to the specified gs:// path on success")
+	kopsMasterType   = flag.String("kops-master-type", "c4.large", "(kops only) master instance type")


kops-master-size, for consistency with the master-size kops flag

I also think default should be empty, because otherwise you're changing the size that all tests run with.

Yah I understand that we are changing all tests, and it is a big change. My overall plan is to always select a random region and at least one random AZ. Because of that plan, we are going to hit far more regions that do not support m3 instance types. If we default it to c4.large it may help us with lack of m3 instances. What are your thoughts?

justinsb · 2018-02-03T02:24:25Z

kubetest/kops.go

+	etcdVersion string
+
+	// masterType is the EC2 instance type for the master
+	masterType string


masterSize, for consistency with the kops flag

justinsb · 2018-02-03T02:26:49Z

kubetest/kops.go

+
+// getRandomAWSZones looks up all regions, and the availability zones for those regions.  A random
+// region is then chosen and the AZ's for that region is returned.  If HA is required a regions with 3
+// or more zones is returned.


Maybe replace " If HA is required a regions with 3 or more zones is returned." with "At least masterCount zones will be returned, all in the same region" or something like that?

justinsb · 2018-02-03T02:27:18Z

kubetest/kops.go

+	for _, i := range rand.Perm(len(awsRegions)) {
+		ec2Session, err := getAWSEC2Session(awsRegions[i])
+		if err != nil {
+			return selectedZones, err


return nil, err would be less confusing

justinsb · 2018-02-03T02:27:31Z

kubetest/kops.go

+		// az for a region. AWS Go API does not allow us to make a single call
+		zoneResults, err := ec2Session.DescribeAvailabilityZones(&ec2.DescribeAvailabilityZonesInput{})
+		if err != nil {
+			return nil, fmt.Errorf("unable to call aws api DescribeAvailabilityZones: %v", err)


Include region in message

justinsb · 2018-02-03T02:28:17Z

kubetest/kops.go

+		}
+	}
+
+	return selectedZones, fmt.Errorf("unable to find region with %d zones", masterCount)


nil, fmt.Error... I think

justinsb · 2018-02-03T02:30:27Z

kubetest/kops_test.go

+import "testing"
+
+///
+// TODO:(@chrislovecnm) we should mock out the aws interface so we do not make a aws API call


I think we should do one of (1) just remove the skipped tests, (2) implement the mock, (3) make the function testable by splitting it up. I'm not sure there's much to test in option 3, TBH.

BenTheElder · 2018-02-06T16:55:47Z

/retest

chrislovecnm · 2018-02-06T18:25:12Z

@justinsb PTAL

I think there are two points that we may disagree on.

changing the default instance size to c4.large. m3.large are not available in a lot of regions and az, even one of our az in us-east-1. With the new code that selects from all regions, I think we need to change
validating the zones. I do not think we will need that anymore. My plan is to not pass in the zones for the most part and have the code choose random zones. I now have the capability of just choosing a single random zone in a random region. Also, I think it is kinda outside the scope of this PR.

I should have done all of your other updates, please let me know if I missed anything.

justinsb · 2018-02-08T00:46:36Z

LGTM

chrislovecnm · 2018-02-13T05:03:44Z

@krzyzacy do you have time to deploy this on Tuesday?

krzyzacy · 2018-02-13T18:26:02Z

@chrislovecnm can you squash?
/lgtm
/hold
/lint

k8s-ci-robot · 2018-02-13T18:26:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, krzyzacy

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 OWNERS Files:

~~OWNERS~~ [krzyzacy]

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

k8s-ci-robot

@krzyzacy: 0 warnings.

In response to this:

@chrislovecnm can you squash?
/lgtm
/hold
/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Added a new flag for kops-ha. Also, since we now may run in regions that may not have m3 instances, updated the master type to c4 instances. In order to test HA control plain a minimum of 3 AZs must exist. Implemented a func that queries all regions, and then queries the list of that regions AZ. A random region is then selected. If running in ha mode, a region with a minimum of 3 AZ is selected. The new zone capability can be run in both ha and non-ha modes. If --kops-zones is not set, and the test run is on AWS, the new code is used. You can override the new code by setting the zone as done previously. Added a unit test, that requires a live AWS account for testing the new random region selection. These tests are skipped since they require a live AWS account.

chrislovecnm · 2018-02-13T19:21:04Z

/hold cancel

chrislovecnm requested a review from krzyzacy as a code owner December 31, 2017 01:57

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 31, 2017

k8s-ci-robot assigned krzyzacy Dec 31, 2017

k8s-ci-robot requested review from geojaz and justinsb December 31, 2017 01:58

chrislovecnm force-pushed the kops-regions branch 3 times, most recently from b7af578 to cc29ef3 Compare December 31, 2017 02:28

chrislovecnm mentioned this pull request Dec 31, 2017

AWS HA Testing with kops #5916

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 31, 2017

chrislovecnm mentioned this pull request Jan 4, 2018

vendor AWS Go SDK #6101

Merged

chrislovecnm force-pushed the kops-regions branch 2 times, most recently from 28d98c2 to 442df92 Compare January 6, 2018 07:06

chrislovecnm mentioned this pull request Jan 6, 2018

aws go library is broken #6173

Closed

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 8, 2018

chrislovecnm force-pushed the kops-regions branch 2 times, most recently from b722f2f to 37f743c Compare January 9, 2018 01:07

krzyzacy reviewed Jan 9, 2018

View reviewed changes

k8s-ci-robot reviewed Jan 9, 2018

View reviewed changes

krzyzacy mentioned this pull request Jan 9, 2018

Add bazel rules for aws-janitor #6204

Closed

chrislovecnm force-pushed the kops-regions branch from 2b1fc28 to 20e7e6b Compare January 12, 2018 20:39

k8s-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 12, 2018

justinsb reviewed Jan 22, 2018

View reviewed changes

chrislovecnm force-pushed the kops-regions branch from 20e7e6b to 51116ab Compare January 26, 2018 02:41

justinsb reviewed Jan 26, 2018

View reviewed changes

chrislovecnm force-pushed the kops-regions branch 4 times, most recently from 5d444a7 to 66d96e9 Compare February 2, 2018 22:18

justinsb reviewed Feb 3, 2018

View reviewed changes

chrislovecnm force-pushed the kops-regions branch from 90dfa88 to bfe00e8 Compare February 6, 2018 18:03

chrislovecnm force-pushed the kops-regions branch from bfe00e8 to 94c4698 Compare February 13, 2018 05:03

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2018

k8s-ci-robot reviewed Feb 13, 2018

View reviewed changes

chrislovecnm force-pushed the kops-regions branch from 94c4698 to 75e5c46 Compare February 13, 2018 19:20

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 13, 2018

k8s-ci-robot merged commit ae4dc70 into kubernetes:master Feb 13, 2018

		"--zones", strings.Join(k.zones, ","),
		"--override", "'cluster.spec.etcdClusters[*].version=3.1.10'",

		kopsDiskSize = flag.Int("kops-disk-size", 48, "Disk size to use for nodes and masters")
		kopsHA = flag.Bool("kops-ha", false, "(kops only) Run HA control plane")

Adding capability for HA control plane testing with kops #6113

Adding capability for HA control plane testing with kops #6113

Conversation

chrislovecnm commented Dec 31, 2017

chrislovecnm commented Dec 31, 2017

chrislovecnm commented Dec 31, 2017

Choose a reason for hiding this comment

krzyzacy Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzyzacy commented Jan 9, 2018

k8s-ci-robot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Jan 26, 2018

justinsb commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Feb 2, 2018

chrislovecnm commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Feb 6, 2018

chrislovecnm commented Feb 6, 2018

justinsb commented Feb 8, 2018

chrislovecnm commented Feb 13, 2018

krzyzacy commented Feb 13, 2018

k8s-ci-robot commented Feb 13, 2018

k8s-ci-robot left a comment

Choose a reason for hiding this comment

chrislovecnm commented Feb 13, 2018

krzyzacy Jan 9, 2018 •

edited

Loading

chrislovecnm Jan 9, 2018 •

edited

Loading