kubetest: add AKS deployer #16452

alexeldeib · 2020-02-24T18:24:03Z

This PR breaks the single existing deployer for AKS into two Azure deployers, one using AKS-Engine and one for AKS. Currently they share a lot of flags, and I haven't gone into a deep effort to dedupe or shuffle some of these around since they're use somewhat deeply throughout kubetest. I wanted to open this as-is and start getting feedback/validation before I attempted any major cleanup.

I'll open a separate PR in k/k shortly with some of the even-less-polished bash to integrate with kubemark and clusterloader2 for scalability tests.

feiskyer · 2020-02-28T09:53:33Z

@alexeldeib is this ready for review?

alexeldeib · 2020-03-03T05:34:23Z

@feiskyer it's a bit rough still, but enough of the plumbing works to run conformance.

If you're open to reviewing as-is, we can work towards an initial merge, and then follow up with a periodic prowjob to run conformance and use that as signal for where to continue iterating?

alexeldeib · 2020-03-03T18:34:26Z

kubetest/aks.go

+		return fmt.Errorf("failed to read downloaded cluster template file: %v", err)
+	}
+
+	var model containerservice.ManagedCluster


i'd like to convert this to raw JSON and use a REST client so we can use any version, but for now I will do it with a specific type.

alexeldeib · 2020-03-04T21:55:29Z

@feiskyer please do take a pass on review on this. I think the main things we will want to change will be prepareAks() + flags to plumb for kubemark/scalability. For this cut, I'm focusing only on the kubetest deployer and targeting conformance. @marwanad is going to pick up this side: kubernetes/kubernetes#88477 and drive the kubemark integrations separately.

alexeldeib · 2020-03-04T21:55:50Z

/assign @feiskyer

marwanad

one question and a nit

kubetest/aks.go

marwanad · 2020-03-04T23:41:42Z

kubetest/aks.go

+	if err != nil {
+		return nil, fmt.Errorf("failed to get azure credentials: %v", err)
+	}
+	env, err := azure.EnvironmentFromName(*aksAzureEnv)


just double-checking, this is still in the aks-engine deployer and i guess this is fine for the PR purposes and the goal is to split into a common Azure or even just define them locally in aks.go based on what we need later?

I think i'd like to split the flags and share a common azure set. Since you'll never invoke with two deployers, I didn't think it was a high priority fix. Something frustrating is that the flags are mostly used directly from different parts of the code, not passed down, which makes it a slightly more involved change, but nothing crazy.

feiskyer · 2020-03-05T09:24:47Z

kubetest/aks.go

+		*aksResourceGroupName = *aksResourceName
+	}
+	if *aksDNSPrefix == "" {
+		*aksDNSPrefix = *aksResourceName


would dns prefix too long since uuid is used in the name?

The generated names with form kubetest-$UUID work okay in Azure. If the user provides one, I'd expect them to make sure it validates properly.

feiskyer · 2020-03-05T09:26:15Z

kubetest/aks.go

+	*(*model.LinuxProfile.SSH.PublicKeys)[0].KeyData = string(sshPublicKey)
+	model.ManagedClusterProperties.DNSPrefix = aksDNSPrefix
+	model.ManagedClusterProperties.ServicePrincipalProfile.ClientID = &a.azureCreds.ClientID
+	model.ManagedClusterProperties.ServicePrincipalProfile.Secret = &a.azureCreds.ClientSecret


I think it's better to also set region and kubernetes version here

fixed, parsing from the shared flags with aks-e currently

feiskyer · 2020-03-05T09:31:27Z

kubetest/e2e.go

+		if err != nil {
+			return fmt.Errorf("failed to get masterInternalIP: %v", err)
+		}
+	case "aks":


@nilo19 do we need those env for aks-engine deployer?

feiskyer · 2020-03-06T06:29:10Z

could you fix the bazel errors and squash the commits?

feiskyer

/lgtm
/approve

k8s-ci-robot · 2020-03-06T06:52:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeldeib, feiskyer

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:

~~kubetest/OWNERS~~ [feiskyer]

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

mborsz · 2020-03-11T10:36:28Z

kubetest/e2e.go

+	var masterIP, masterInternalIP []byte
+
+	switch o.deployment {
+	case "gce":


existing kubemark tests don't set --deployment=gce (it's not even correct value of deployment flag). We use --deployment=bash with --provider=gce

This breaks our kubermark tests in gce.

Apologies for the inconvenience @mborsz. I started a revert, maybe you could take a look: #16733.

Looking again at the default deployment path, is it safe to assume --deployment=bash only supports the gce provider currently? (i.e., this behavior is correct barring that i've put "gce" and not "bash" in the switch)

My understanding is that right now gce is an only supported provider in deplyment=bash mode, but I'm not sure if we want to use this assumption here (what if some day someone adds support for some other provider)?

Personally, I would go with

if deployment == "aks" { use logic specific for aks } else if deployment == "bash" and provider == "gce" { use logic for gce } else { fatal error }

What do you think about this solution?

Makes sense to me. I’m happy to make this change on top of the revert, assuming that’s the sequence you’d prefer.

I wonder if there is some CI automation we can improve to avoid this situation in the future?

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 24, 2020

k8s-ci-robot requested review from feiskyer and wojtek-t February 24, 2020 18:24

k8s-ci-robot added area/kubetest sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Feb 24, 2020

alexeldeib mentioned this pull request Feb 24, 2020

add: kubemark AKS integration kubernetes/kubernetes#88477

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2020

alexeldeib force-pushed the ace/aks branch from 143137a to 2072be3 Compare March 3, 2020 03:45

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2020

alexeldeib force-pushed the ace/aks branch from 6a0a341 to e3994c7 Compare March 3, 2020 04:49

alexeldeib commented Mar 3, 2020

View reviewed changes

k8s-ci-robot assigned feiskyer Mar 4, 2020

marwanad reviewed Mar 4, 2020

View reviewed changes

feiskyer reviewed Mar 5, 2020

View reviewed changes

alexeldeib force-pushed the ace/aks branch from bc33bd7 to 7e74a58 Compare March 6, 2020 06:32

add aks kubetest deployer

90d879c

alexeldeib force-pushed the ace/aks branch from 7e74a58 to 90d879c Compare March 6, 2020 06:39

feiskyer reviewed Mar 6, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2020

k8s-ci-robot merged commit eff5545 into kubernetes:master Mar 6, 2020

k8s-ci-robot added this to the v1.18 milestone Mar 6, 2020

mborsz mentioned this pull request Mar 11, 2020

Automated cherry pick of fix for #77733 (NodeLifecycleController is overloading kube-apiserver) into release-1.15 kubernetes/kubernetes#88962

Merged

mborsz reviewed Mar 11, 2020

View reviewed changes

mborsz mentioned this pull request Mar 11, 2020

Update prow to v20200305-062b3366c, and other images as necessary. #16639

Merged

alexeldeib mentioned this pull request Mar 12, 2020

add: fixes to aks kubetest deployer #16659

Closed

alexeldeib mentioned this pull request Mar 19, 2020

kubetest: re-merge aks deployer #16813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubetest: add AKS deployer #16452

kubetest: add AKS deployer #16452

alexeldeib commented Feb 24, 2020

feiskyer commented Feb 28, 2020

alexeldeib commented Mar 3, 2020 •

edited

Loading

alexeldeib Mar 3, 2020

alexeldeib commented Mar 4, 2020

alexeldeib commented Mar 4, 2020

marwanad left a comment

marwanad Mar 4, 2020

alexeldeib Mar 5, 2020

feiskyer Mar 5, 2020

alexeldeib Mar 5, 2020

feiskyer Mar 5, 2020

alexeldeib Mar 5, 2020

feiskyer Mar 5, 2020

feiskyer commented Mar 6, 2020

feiskyer left a comment

k8s-ci-robot commented Mar 6, 2020

mborsz Mar 11, 2020

alexeldeib Mar 11, 2020

mborsz Mar 11, 2020

alexeldeib Mar 11, 2020

kubetest: add AKS deployer #16452

kubetest: add AKS deployer #16452

Conversation

alexeldeib commented Feb 24, 2020

feiskyer commented Feb 28, 2020

alexeldeib commented Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

alexeldeib commented Mar 4, 2020

alexeldeib commented Mar 4, 2020

marwanad 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

feiskyer commented Mar 6, 2020

feiskyer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 6, 2020

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

alexeldeib commented Mar 3, 2020 •

edited

Loading