Fix compiler and test failures with latest version of sigs.k8s.io/controller-runtime #7979

brooke-hamilton · 2024-10-06T20:50:04Z

Description

A recent change to the controller-runtime package caused Radius compilation errors and test failures, as referenced in #7882. Breaking changes in controller-runtime include the removal of a configuration option, and a new validation that prevents duplicate controller names. This PR makes changes to update the packages referenced, fix the compilation errors, and address the test failures.

Changes include:

Updates to go.mod and go.sum.
Fixed compiler errors by moving the warning suppression configuration from the controller-runtime/pkg/client options to client-go/rest/Config, as required by the changes in controller-runtime.
Fixed broken tests by adding the SkipNameValidation configuration parameter to unit tests for creating new controllers.
Some refactoring of commonly shared test functions and constants in cli/controller/reconciler into shared-test.go.

NOTE: The compilation fix in this PR did not change the behavior of the rad cli--only the tests were changed. The assumption is that rad does not ever create controllers with duplicate names, so suppressing the duplicate controller name validation is not necessary, even though it is necessary in the automated tests.

Type of change

This pull request fixes a bug in Radius and has an approved issue (issue link required).

Fixes: #7882

Contributor checklist

Please verify that the PR meets the following requirements, where applicable:

An overview of proposed schema changes is included in a linked GitHub issue.
A design document PR is created in the design-notes repository, if new APIs are being introduced.
If applicable, design document has been reviewed and approved by Radius maintainers/approvers.
A PR for the samples repository is created, if existing samples are affected by the changes in this PR.
A PR for the documentation repository is created, if the changes in this PR affect the documentation or any user facing updates are made.
A PR for the recipes repository is created, if existing recipes are affected by the changes in this PR.

Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>

rynowak · 2024-10-06T22:52:41Z

pkg/controller/reconciler/deployment_reconciler_test.go

@@ -74,7 +77,7 @@ func SetupDeploymentTest(t *testing.T) (*mockRadiusClient, client.Client) {
 	err = (&DeploymentReconciler{
 		Client:        mgr.GetClient(),
 		Scheme:        mgr.GetScheme(),
-		EventRecorder: mgr.GetEventRecorderFor("recipe-controller"),
+		EventRecorder: mgr.GetEventRecorderFor("deployment-controller"),


rynowak · 2024-10-06T22:55:52Z

Looks like make generate needs to run again. Your other changes probably updated a code generator we're using for the K8s controllers.

Fixed broken tests by adding the SkipNameValidation configuration parameter to unit tests for creating new controllers.

Do you know if SkipNameValidation is a new kind of warning they added to controller-runtime?

I agree with suppressing the warning. It's probably being triggered because our tests will register a new instance of the same controller for each test.

rynowak

Changes in this PR so far look right 👍. Will look again after you resolve build issues.

brooke-hamilton · 2024-10-07T02:55:08Z

Looks like make generate needs to run again. Your other changes probably updated a code generator we're using for the K8s controllers.

Fixed broken tests by adding the SkipNameValidation configuration parameter to unit tests for creating new controllers.

Do you know if SkipNameValidation is a new kind of warning they added to controller-runtime?

I agree with suppressing the warning. It's probably being triggered because our tests will register a new instance of the same controller for each test.

I'm having an issue with the make generate-controller target. I will follow up.

Validation to prevent duplication of controller names was added in PR2902, and a few days later PR2918 added the SkipNameValidation configuration option which bypasses the validation for duplicate names.

Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>

rynowak · 2024-10-07T15:59:57Z

Thanks. Based on this comment, we're in exactly the same test scenario as the user here: kubernetes-sigs/controller-runtime#2902 (comment)

Sounds like it's intended we use SkipNameValidation.

brooke-hamilton · 2024-10-07T16:01:43Z

@rynowak I needed to bump the controller-gen version to align with the current version of k8s.io/*. https://github.com/kubernetes-sigs/controller-tools

codecov · 2024-10-07T16:56:34Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 60.62%. Comparing base (d2870a7) to head (c9c3f41).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/ucp/dataprovider/factory.go	0.00%	1 Missing ⚠️
pkg/ucp/queue/provider/factory.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7979   +/-   ##
=======================================
  Coverage   60.61%   60.62%           
=======================================
  Files         534      534           
  Lines       28488    28484    -4     
=======================================
  Hits        17269    17269           
+ Misses       9702     9698    -4     
  Partials     1517     1517

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

radius-functional-tests · 2024-10-07T17:27:01Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	brooke-hamilton/radius
Commit ref	`baa66de`
Unique ID	funcf735b768d7
Image tag	pr-funcf735b768d7

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcf735b768d7
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcf735b768d7
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcf735b768d7
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcf735b768d7
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

pkg/controller/reconciler/recipe_reconciler_test.go

pkg/ucp/queue/provider/factory.go

Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>

radius-functional-tests · 2024-10-07T19:13:00Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	brooke-hamilton/radius
Commit ref	`c9c3f41`
Unique ID	func715cd02c94
Image tag	pr-func715cd02c94

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func715cd02c94
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func715cd02c94
controller test image location: ghcr.io/radius-project/dev/controller:pr-func715cd02c94
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func715cd02c94
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

Copied over from #7980. This is needed because original dependabot PR has conflicts now due to the manual changes that were made in #7979. Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

test fixes

eab43ad

Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>

brooke-hamilton requested review from a team as code owners October 6, 2024 20:50

rynowak reviewed Oct 6, 2024

View reviewed changes

brooke-hamilton added 2 commits October 7, 2024 03:00

add -y to check for npx

6e374d1

Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>

rev controller-gen to v0.16.0

4b54293

Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>

Merge branch 'main' into update-controller-runtime

baa66de

brooke-hamilton temporarily deployed to functional-tests October 7, 2024 17:26 — with GitHub Actions Inactive

lakshmimsft reviewed Oct 7, 2024

View reviewed changes

pkg/controller/reconciler/recipe_reconciler_test.go Show resolved Hide resolved

lakshmimsft reviewed Oct 7, 2024

View reviewed changes

pkg/ucp/queue/provider/factory.go Outdated Show resolved Hide resolved

code formatting

c9c3f41

Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>

brooke-hamilton temporarily deployed to functional-tests October 7, 2024 19:11 — with GitHub Actions Inactive

lakshmimsft approved these changes Oct 7, 2024

View reviewed changes

brooke-hamilton merged commit 3829f35 into radius-project:main Oct 7, 2024
28 checks passed

brooke-hamilton deleted the update-controller-runtime branch October 7, 2024 19:55

kachawla mentioned this pull request Oct 8, 2024

Bump the all group across 1 directory with 38 updates #7981

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compiler and test failures with latest version of sigs.k8s.io/controller-runtime #7979

Fix compiler and test failures with latest version of sigs.k8s.io/controller-runtime #7979

brooke-hamilton commented Oct 6, 2024

rynowak Oct 6, 2024

rynowak commented Oct 6, 2024

rynowak left a comment

brooke-hamilton commented Oct 7, 2024

rynowak commented Oct 7, 2024

brooke-hamilton commented Oct 7, 2024

codecov bot commented Oct 7, 2024 •

edited

Loading

radius-functional-tests bot commented Oct 7, 2024 •

edited

Loading

radius-functional-tests bot commented Oct 7, 2024 •

edited

Loading

Fix compiler and test failures with latest version of sigs.k8s.io/controller-runtime #7979

Fix compiler and test failures with latest version of sigs.k8s.io/controller-runtime #7979

Conversation

brooke-hamilton commented Oct 6, 2024

Description

Type of change

Contributor checklist

rynowak Oct 6, 2024

Choose a reason for hiding this comment

rynowak commented Oct 6, 2024

rynowak left a comment

Choose a reason for hiding this comment

brooke-hamilton commented Oct 7, 2024

rynowak commented Oct 7, 2024

brooke-hamilton commented Oct 7, 2024

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

radius-functional-tests bot commented Oct 7, 2024 • edited Loading

Radius functional test overview

Test Status

radius-functional-tests bot commented Oct 7, 2024 • edited Loading

Radius functional test overview

Test Status

codecov bot commented Oct 7, 2024 •

edited

Loading

radius-functional-tests bot commented Oct 7, 2024 •

edited

Loading

radius-functional-tests bot commented Oct 7, 2024 •

edited

Loading