test: adding testing trigger for running our e2e suite against the NodeAutoProvisioning Preview #178

Bryce-Soghigian · 2024-03-04T22:19:53Z

Fixes #195

Description
This PR adds additional functionality to our testing toolkit to run the e2e suites on node auto provisioning, alongside the consolidation e2e suite which only works against node auto provisioning and not fully properly with unmanaged karpenter.

How was this change tested?

ran the suites on nap
See Runs:
https://github.com/Azure/karpenter-provider-azure/actions/runs/8193216788/job/22475958956
https://github.com/Azure/karpenter-provider-azure/actions/runs/8150047908/job/22275681008

Then runs validating the standard e2es functionality haven't been tarnished: https://github.com/Azure/karpenter-provider-azure/actions/runs/8232044776
Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: #
No

Release Note

update fix: fix blah test: nap suite consolidation test no need for acr fix: adding preview cli fix: removing make az-perm as we are using nap which should set this up

coveralls · 2024-03-04T22:25:55Z

Pull Request Test Coverage Report for Build 8232173689

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 97.691%

Totals
Change from base Build 8194965275:	0.0%
Covered Lines:	35586
Relevant Lines:	36427

💛 - Coveralls

bruh

Bryce-Soghigian

/test-nap

Bryce-Soghigian · 2024-03-05T02:50:41Z

https://github.com/Azure/karpenter-provider-azure/actions/runs/8149980061
https://github.com/Azure/karpenter-provider-azure/actions/runs/8150047908/job/22275681008

Bryce-Soghigian

/test-nap

Bryce-Soghigian

/nap

Bryce-Soghigian

/nap

Bryce-Soghigian

/test

Bryce-Soghigian

/nap

…because it's always true

Bryce-Soghigian · 2024-03-11T11:29:56Z

See the tests pass for nap https://github.com/Azure/karpenter-provider-azure/actions/runs/8193216788/job/22475958956

Bryce-Soghigian

/test

Bryce-Soghigian · 2024-03-11T11:30:35Z

Validating standard tests wtill work and standard workflow triggers still work

Bryce-Soghigian · 2024-03-11T11:49:00Z

running bash hack/boilerplate.sh is not generating the right configuration for the consolidation header for some reason

charliedmcb

Initial review. Didn't get to go over the files within .github yet. Will be completing a second review for that.

Overall looking good, but I'd like a couple small adjustments, the ability to signal running the E2E tests for NAP or not via input params, and I think for cleanliness/focus it makes sense to split this into 3 PRs:

workflow change, and required supporting Makefile//test changes
new consolidation test suite
general unrelated cleanup to the e2e test files.

charliedmcb · 2024-03-26T22:44:22Z

Makefile-az.mk

@@ -3,17 +3,19 @@ ifeq ($(CODESPACES),true)
 AZURE_RESOURCE_GROUP ?= $(CODESPACE_NAME)
 AZURE_ACR_NAME ?= $(subst -,,$(CODESPACE_NAME))
 else
- AZURE_RESOURCE_GROUP ?= karpenter
+ AZURE_RESOURCE_GROUP ?= karpeeer 


why changing karpenter to karpeeer?

not on purpose :O

Makefile-az.mk

charliedmcb · 2024-03-26T22:47:07Z

Makefile-az.mk

@@ -36,6 +38,16 @@ az-mkaks: az-mkacr ## Create test AKS cluster (with --vm-set-type AvailabilitySe
 az aks get-credentials --name $(AZURE_CLUSTER_NAME) --resource-group $(AZURE_RESOURCE_GROUP) --overwrite-existing
 skaffold config set default-repo $(AZURE_ACR_NAME).azurecr.io/karpenter

+az-mkaks-cilium-nap:
+ az extension add --name aks-preview


I slightly question auto-adding this. It should be a pre-requirement the user has for running this command in my opinion.

Having the side effect of running this make command be that we add the aks-preview extension to the users cli seems questionable.

We need it because there is no way to registere specific versions of the preview cli in the github action from what I could find. Added a makefile command following the existing pattern used for creating the cluster.

I'd still be wary on adding it to this make command. Separating it out as an az-apply-nap-prerequirements make command along with the feature registration as I mention in the below comment I think would be good. That way in the action we can call az-apply-nap-prerequirements, but anyone doing local work, or such can avoid having the prerequirements side effects.

charliedmcb · 2024-03-26T22:50:41Z

Makefile-az.mk

@@ -36,6 +38,16 @@ az-mkaks: az-mkacr ## Create test AKS cluster (with --vm-set-type AvailabilitySe
 az aks get-credentials --name $(AZURE_CLUSTER_NAME) --resource-group $(AZURE_RESOURCE_GROUP) --overwrite-existing
 skaffold config set default-repo $(AZURE_ACR_NAME).azurecr.io/karpenter

+az-mkaks-cilium-nap:
+ az extension add --name aks-preview
+ az feature register --namespace "Microsoft.ContainerService" --name "NodeAutoProvisioningPreview"


I also highly question having this side effect as well. I think again this is registering NodeAutoProvisioningPreview feature for the subscription.

A possible solution here would be to have an az-apply-nap-prerequirements with a note on it that it will update the cli to have the aks-preview extension, plus register the sub.

This way it can be something that users only have to apply once, but if it comes up in some pipeline, etc can always be called as a prerequirement.

Sub registration could be ran once, but we need the add of the aks-preview extension to be updated to the latest preview cli

charliedmcb · 2024-03-26T22:54:49Z

Makefile-az.mk

+ --enable-oidc-issuer --enable-managed-identity --node-provisioning-mode Auto
+
+ az aks get-credentials --name $(AZURE_CLUSTER_NAME) --resource-group $(AZURE_RESOURCE_GROUP) --overwrite-existing
+ kubectl taint nodes CriticalAddonsOnly=true:NoSchedule --all


I wouldn't add this taint here. If you are wanting this within the e2e pipeline, put it into the workflows instead.

The makefiles I think should be a place for general commands, and if a user were to create a NAP cluster should they actually be applying this taint?

They should ALWAYS taint the systempool. NAP should create all of the userpool nodes imo.

Wonder if we should add something about it to the docs than?

https://learn.microsoft.com/en-us/azure/aks/node-autoprovision?tabs=azure-cli

So far tainting has been something we've done within the e2e test workflow themselves not the make commands.

However, if it is a best practice we want to recommend than I could see adding it to the make command, and/or documentation.

Thoughts?

I agree we should! Maybe referencing the existing systempool documentation makes sense and explaining a bit more about the role of the system-surge pool

test/pkg/environment/common/expectations.go

test/suites/consolidation/suite_test.go

test/pkg/environment/azure/setup.go

test/pkg/environment/azure/environment.go

charliedmcb

Little mixed on how much to duplicate the workflow files, but I think at least a few of them can definitely be merged with the regular flow:

approval-comment-nap.yaml (merge with regular)
cleanup-nap/action.yaml (merge with regular)

I'd try and merge based on some form of flag:

e2e-matrix-trigger-nap.yaml
e2e-matrix-nap.yaml

I think that e2e-nap.yaml might be able to be consolidated, but I'm wondering if its right to as it somewhat seems right that a "nap e2e test is different". I think at least through the point of we are selecting the matrix of tests to run, and how to run them seems reasonable.

I think keeping a separate create action for nap does make sense though.

Not certain on all the functionality available for getting some sort of flag for this, and if it'd be too messy in the code, but those are my general thoughts on reducing duplication for now.

.github/workflows/approval-comment-nap.yaml

charliedmcb · 2024-03-27T17:45:18Z

.github/workflows/e2e-matrix-trigger.yaml

@@ -32,4 +32,4 @@ jobs:
 secrets:
 E2E_CLIENT_ID: ${{ secrets.E2E_CLIENT_ID }}
 E2E_TENANT_ID: ${{ secrets.E2E_TENANT_ID }}
- E2E_SUBSCRIPTION_ID: ${{ secrets.E2E_SUBSCRIPTION_ID }}


Assuming this was an accidental change?

If not, than add it to the general e2e cleanup PR to keep files being touched focused?

.github/workflows/README.md

.github/workflows/e2e-matrix-trigger-nap.yaml

.github/workflows/e2e-matrix-nap.yaml

.github/workflows/e2e-nap.yaml

.github/actions/e2e/create-nap-cluster/action.yaml

charliedmcb · 2024-03-27T18:38:12Z

.github/actions/e2e/cleanup-nap/action.yaml

+ run: az account set --subscription ${{ inputs.subscription-id }}
+ - name: delete cluster ${{ inputs.cluster_name }}
+ shell: bash
+ run: az aks delete --name ${{ inputs.cluster_name }} --resource-group ${{ inputs.resource_group }} --yes --no-wait


Just doesn't have the delete acr? set. Could simply remove that from the general cleanup since resource group delete should be enough. I think that's worth removing the need for an almost duplicate file.

* revert changes to non .github/ files * remove new test suite * Revert "remove new test suite" This reverts commit 0e7f3ab. * Revert "revert changes to non .github/ files" This reverts commit 40a6377. * update nap triggers to be based on same approval comment --------- Co-authored-by: Charlie McBride <[email protected]>

Bryce-Soghigian · 2024-03-29T07:37:11Z

cleanup prs separated out:
#241
#240

Bryce-Soghigian · 2024-04-16T16:50:37Z

test ping @Azure/aks-karpenter

Bryce-Soghigian added 2 commits March 4, 2024 14:15

test: adding nap e2e matrix trigger

4f3be53

update fix: fix blah test: nap suite consolidation test no need for acr fix: adding preview cli fix: removing make az-perm as we are using nap which should set this up

test: adding new workflow trigger

8757474

Bryce-Soghigian changed the title ~~Bsoghigian/running nap clean~~ test: adding testing trigger for running our e2e suite against the NodeAutoProvisioning Preview Mar 4, 2024

fix: fix cleanup failure due missing acr for nap case

90c2ea4

bruh

Bryce-Soghigian force-pushed the bsoghigian/running-nap-clean branch from c10c0e9 to 90c2ea4 Compare March 4, 2024 22:29

Bryce-Soghigian added 5 commits March 4, 2024 14:35

fix: removing get karpenter deployment step

a7f2293

fix: tainting all the nodes

950606c

fix: modifying expect statements to work with nap

8cc40ad

fix: default pool

b59d84d

fix:drift

50e0e37

Bryce-Soghigian commented Mar 5, 2024

View reviewed changes

Bryce-Soghigian added 2 commits March 4, 2024 18:14

fix: removing branch trigger and using real approval comment

6d328fa

fix: removing branch

4162a24

Bryce-Soghigian commented Mar 5, 2024

View reviewed changes

fix: updating suites

5b6aec3

Bryce-Soghigian commented Mar 5, 2024

View reviewed changes

fix: default testing suite

e66b4e4

Bryce-Soghigian commented Mar 5, 2024

View reviewed changes

Bryce-Soghigian added 5 commits March 5, 2024 01:52

trigger workflow

f8faca4

fix: fixing scale down helper to be aware of system-surge-pool

163957d

fix: removing expect statement for empty pools in the else statement …

48d6b5f

…because it's always true

style(ci): removing unused settings references in the tests

28473b9

fix: removing trigger on branch push

4dbab58

Bryce-Soghigian commented Mar 11, 2024

View reviewed changes

Merge branch 'main' into bsoghigian/running-nap-clean

951013e

Bryce-Soghigian requested review from charliedmcb and rakechill March 11, 2024 11:41

Bryce-Soghigian marked this pull request as ready for review March 11, 2024 11:49

Bryce-Soghigian self-assigned this Mar 11, 2024

Bryce-Soghigian added the area/e2e-test-framework Issues or PRs related to refactoring the e2e test framework label Mar 11, 2024

charliedmcb requested changes Mar 26, 2024

View reviewed changes

charliedmcb requested changes Mar 27, 2024

View reviewed changes

charliedmcb mentioned this pull request Mar 27, 2024

test(e2e): running e2e on nap #233

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: adding testing trigger for running our e2e suite against the NodeAutoProvisioning Preview #178

test: adding testing trigger for running our e2e suite against the NodeAutoProvisioning Preview #178

Bryce-Soghigian commented Mar 4, 2024 •

edited

coveralls commented Mar 4, 2024 •

edited

Bryce-Soghigian left a comment

Bryce-Soghigian commented Mar 5, 2024

Bryce-Soghigian left a comment

Bryce-Soghigian left a comment

Bryce-Soghigian left a comment

Bryce-Soghigian left a comment

Bryce-Soghigian left a comment

Bryce-Soghigian commented Mar 11, 2024

Bryce-Soghigian left a comment

Bryce-Soghigian commented Mar 11, 2024

Bryce-Soghigian commented Mar 11, 2024

charliedmcb left a comment

charliedmcb Mar 26, 2024

Bryce-Soghigian Mar 26, 2024

charliedmcb Mar 26, 2024

Bryce-Soghigian Mar 26, 2024 •

edited

charliedmcb Mar 27, 2024 •

edited

charliedmcb Mar 26, 2024

Bryce-Soghigian Mar 27, 2024

charliedmcb Mar 26, 2024

Bryce-Soghigian Mar 27, 2024

charliedmcb Mar 27, 2024 •

edited

Bryce-Soghigian Mar 27, 2024

charliedmcb left a comment

charliedmcb Mar 27, 2024

charliedmcb Mar 27, 2024

charliedmcb Mar 27, 2024

Bryce-Soghigian commented Mar 29, 2024

Bryce-Soghigian commented Apr 16, 2024

test: adding testing trigger for running our e2e suite against the NodeAutoProvisioning Preview #178

Are you sure you want to change the base?

test: adding testing trigger for running our e2e suite against the NodeAutoProvisioning Preview #178

Conversation

Bryce-Soghigian commented Mar 4, 2024 • edited

coveralls commented Mar 4, 2024 • edited

Pull Request Test Coverage Report for Build 8232173689

Details

💛 - Coveralls

Bryce-Soghigian left a comment

Choose a reason for hiding this comment

Bryce-Soghigian commented Mar 5, 2024

Bryce-Soghigian left a comment

Choose a reason for hiding this comment

Bryce-Soghigian left a comment

Choose a reason for hiding this comment

Bryce-Soghigian left a comment

Choose a reason for hiding this comment

Bryce-Soghigian left a comment

Choose a reason for hiding this comment

Bryce-Soghigian left a comment

Choose a reason for hiding this comment

Bryce-Soghigian commented Mar 11, 2024

Bryce-Soghigian left a comment

Choose a reason for hiding this comment

Bryce-Soghigian commented Mar 11, 2024

Bryce-Soghigian commented Mar 11, 2024

charliedmcb 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

Bryce-Soghigian Mar 26, 2024 • edited

Choose a reason for hiding this comment

charliedmcb Mar 27, 2024 • edited

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

charliedmcb Mar 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliedmcb 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

Bryce-Soghigian commented Mar 29, 2024

Bryce-Soghigian commented Apr 16, 2024

Bryce-Soghigian commented Mar 4, 2024 •

edited

coveralls commented Mar 4, 2024 •

edited

Bryce-Soghigian Mar 26, 2024 •

edited

charliedmcb Mar 27, 2024 •

edited

charliedmcb Mar 27, 2024 •

edited