Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test(e2e): validate that pulling from an acr registry attached to aks via --attach-acr with karpenter nodes works #457

Merged
merged 8 commits into from
Aug 22, 2024
Merged

test(e2e): validate that pulling from an acr registry attached to aks via --attach-acr with karpenter nodes works #457

merged 8 commits into from
Aug 22, 2024

Conversation

Bryce-Soghigian
Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian commented Aug 14, 2024 •
edited by charliedmcb
Loading

Fixes #

Description
As part of the efforts to fix 1.30 bootstrap we removed a flag that removes functionality for out of tree providers. One of the side affects of this is that ACR Pull will not work in the same way. We needed a way to validate this image pull works in a reproducible and consistent way.

This test suite is that. Currently it has to be ran locally to target specific k8s versions so a nice followup at lower priority would be to add the ability to run our e2es against a specific k8s version.
How was this change tested?

  • make az-e2etests
[251.679 seconds]
------------------------------

Ran 1 of 1 Specs in 251.874 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAcr (251.87s)
PASS
ok      github.com/Azure/karpenter-provider-azure/test/suites/acr       252.588s

Does this change impact docs?

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

Release Note

@Bryce-Soghigian Bryce-Soghigian self-assigned this Aug 14, 2024
Bryce-Soghigian
Bryce-Soghigian commented Aug 14, 2024
View reviewed changes
Copy link
Collaborator Author

@Bryce-Soghigian Bryce-Soghigian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/test

@coveralls
Copy link

coveralls commented Aug 14, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 10512106851

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.702%
Totals Coverage Status
Change from base Build 10503015105: 0.0%
Covered Lines: 36313
Relevant Lines: 37167
💛 - Coveralls

@Bryce-Soghigian
Copy link
Collaborator Author

https://github.com/Azure/karpenter-provider-azure/actions/runs/10392432538/job/28777647831

@Bryce-Soghigian Bryce-Soghigian added the area/e2e-testing Issues or PRs related to e2e testing label Aug 22, 2024
@Bryce-Soghigian
Copy link
Collaborator Author

Its failing remotely because the acr name passed in is wrong

cd test && CLUSTER_NAME=mc AZURE_ACR_NAME=karpenter go test
-p 1
-count 1
-timeout "3h"
-v
./suites/acr/...
--ginkgo.focus=""
--ginkgo.timeout="3h"
--ginkgo.grace-period=3m
--ginkgo.vv

@Bryce-Soghigian
Copy link
Collaborator Author

cd test && CLUSTER_NAME=mc AZURE_ACR_NAME=acracr1285317475 go test
-p 1
-count 1
-timeout "3h"
-v
./suites/acr/...
--ginkgo.focus=""
--ginkgo.timeout="3h"
--ginkgo.grace-period=3m
--ginkgo.vv

@Bryce-Soghigian
Copy link
Collaborator Author

https://github.com/Azure/karpenter-provider-azure/actions/runs/10503169793/job/29096028129 should pass now?

charliedmcb
charliedmcb previously approved these changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@charliedmcb charliedmcb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple nits/thoughts on the make commands, but looks good to me

Makefile-az.mk Show resolved Hide resolved
Makefile-az.mk Outdated Show resolved Hide resolved
Makefile-az.mk Show resolved Hide resolved
tallaxes
tallaxes previously approved these changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@tallaxes tallaxes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, minor feedback

.github/workflows/e2e-matrix.yaml Outdated Show resolved Hide resolved
.github/workflows/e2e-matrix.yaml Outdated Show resolved Hide resolved
Makefile-az.mk Outdated Show resolved Hide resolved
Makefile-az.mk Outdated Show resolved Hide resolved
test/suites/acr/suite_test.go Outdated Show resolved Hide resolved
aagusuab
aagusuab approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@aagusuab aagusuab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Bryce-Soghigian
Copy link
Collaborator Author

1m 55s
0s
Run goveralls -coverprofile=coverage.out -service=github
bad response status from coveralls: 405

<title>405 Not Allowed</title> 405 Not Allowed
nginx

Error: Process completed with exit code 1.

_--

CI test is failing due to coveralls outage. Just gonna merge as it is.

tallaxes
tallaxes reviewed Aug 22, 2024
View reviewed changes
Makefile-az.mk
@@ -41,14 +41,11 @@ az-mkacr: az-mkrg ## Create test ACR
az-acrimport: ## Imports an image to an acr registry
az acr import --name $(AZURE_ACR_NAME) --source "mcr.microsoft.com/oss/kubernetes/pause:3.6" --image "pause:3.6"

az-cleanenv:
az-cleanenv: az-rmnodeclaims-fin ## Deletes a few common karpenter testing resources(pods, nodepools, nodeclaims, aksnodeclasses)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To enforce the order that Charlie was suggesting, one can call make target explicitly in the right place, rather than making it a dependency

@Bryce-Soghigian Bryce-Soghigian merged commit 1797b30 into main Aug 22, 2024
11 of 13 checks passed
@Bryce-Soghigian Bryce-Soghigian deleted the bsoghigian/e2e-acr-pull branch August 22, 2024 22:17
Bryce-Soghigian added a commit that referenced this pull request Sep 12, 2024
@Bryce-Soghigian
test(e2e): validate that pulling from an acr registry attached to aks…
37a5bc5 
… via --attach-acr with karpenter nodes works (#457)

* test(e2e): validate that pulling from an acr registry attached to aks via --attach-acr with karpenter nodes works

* fix: ci

* test: ci

* fix: crd validation breaks on local so accidentally committed the change with it disabled

* fix: passing in azure acr name from env rather than using makefile default

* fix: ci again?

* fix: nit comments addressed

* test: only provisioning one pod
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallaxes tallaxes tallaxes left review comments

@aagusuab aagusuab aagusuab approved these changes

@charliedmcb charliedmcb Awaiting requested review from charliedmcb

Assignees

@Bryce-Soghigian Bryce-Soghigian

Labels
area/e2e-testing Issues or PRs related to e2e testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Bryce-Soghigian @coveralls @aagusuab @tallaxes @charliedmcb