chore(role_assignment): make roleassignment reconcile async by sonasingh46 · Pull Request #1873 · kubernetes-sigs/cluster-api-provider-azure

sonasingh46 · 2021-11-17T09:06:04Z

Signed-off-by: Ashutosh Kumar sonasingh46@gmail.com

What type of PR is this?

What this PR does / why we need it:
Implementation of async service for role assignment.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1718

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Make roleassignment reconcile async

k8s-ci-robot · 2021-11-17T09:06:13Z

Hi @sonasingh46. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

mboersma · 2021-11-17T16:53:13Z

/ok-to-test

sonasingh46 · 2022-01-19T16:40:48Z

-	actual := NewService(mps, resourceskus.NewStaticCache(nil, ""))
-	g.Expect(actual).ToNot(BeNil())
-}
+// ToDo: Fix this test case


@CecileRobertMichon @shysank -- Any suggestions on fixing this test case. This test case requires to import scope package and hence causes import loop.

looking, I don't think we should be creating real scopes to test NewService(). NewService takes a ScaleSetScope, which we have a mock for, so we should be able to do:

mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := NewMockScaleSetScope(mockCtrl) actual := NewService(scopeMock, resourceskus.NewStaticCache(nil, "")) g.Expect(actual).ToNot(BeNil())

If that doesn't work, that test isn't testing much since the NewService func just sets the parameters into a struct and returns it, so worst case we can get rid of the test altogether.

I tried something like below:

scopeMock := mock_scalesets.NewMockScaleSetScope(mockCtrl) rec:=scopeMock.EXPECT() rec.SubscriptionID().Return("123") rec.BaseURI().Return("/dummy") rec.Authorizer().Return(scopeMock) actual := NewService(scopeMock, resourceskus.NewStaticCache(nil, "")) g.Expect(actual).ToNot(BeNil())

I added those mock Returns as they were expected but at the end I got error something like this:

wrong type of argument 0 to Return for *mock_scalesets.MockScaleSetScope.Authorizer: *mock_scalesets.MockScaleSetScope is not assignable to autorest.Authorizer [/Users/kuashutosh/go/src/sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesets/scalesets_test.go:66]

Jont828 · 2022-01-24T23:44:27Z

Hey @sonasingh46, how's it going with the role assignments PR? I worked on a lot of the async refactor PRs with @CecileRobertMichon, and I'd be happy to answer any questions or to do a code review.

sonasingh46 · 2022-03-21T17:54:15Z

+	if len(s.Scope.RoleAssignmentSpecs(to.StringPtr(""))) < 1 {
+		log.V(2).Info("no role assignment spec to reconcile")
+		return nil
+	}


This is a redundant call to role assignments spec that I had to make so that when we fetch principalID -- nil point exception does not occur.

When there is not role assignment spec, while fetching the vm details, some of fields are nil and looks more simpler to return early in case there is no role assignment spec.

So this first call passes an empty principal ID and validates if role assignment spec is present and then below finds the principal ID and calls this spec function once again to finally get the correct role assignment specs.
cc @CecileRobertMichon @Jont828

I'd suggest adding another method to scope that just returns true/false if the machine/machinepool has system assigned identity for better readability.

I'd suggest adding another method to scope that just returns true/false if the machine/machinepool has system assigned identity for better readability.

+1

sonasingh46 · 2022-03-24T15:05:46Z

/test pull-cluster-api-provider-azure-e2e

sonasingh46 · 2022-03-24T15:06:19Z

/test pull-cluster-api-provider-azure-e2e-windows-dockershim

k8s-ci-robot · 2022-03-24T15:07:34Z

@sonasingh46: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-azure-build
/test pull-cluster-api-provider-azure-e2e
/test pull-cluster-api-provider-azure-test
/test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-azure-apidiff
/test pull-cluster-api-provider-azure-apiversion-upgrade
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-ci-entrypoint
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-coverage
/test pull-cluster-api-provider-azure-e2e-exp
/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-e2e-workload-upgrade
/test pull-cluster-api-provider-azure-upstream-windows-dockershim
/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts
/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts-serial-slow

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-azure-apidiff
pull-cluster-api-provider-azure-build
pull-cluster-api-provider-azure-ci-entrypoint
pull-cluster-api-provider-azure-coverage
pull-cluster-api-provider-azure-e2e
pull-cluster-api-provider-azure-test
pull-cluster-api-provider-azure-verify

Details

In response to this:

/test pull-cluster-api-provider-azure-e2e-windows-dockershim

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.

CecileRobertMichon

/lgtm
/assign @shysank

CecileRobertMichon · 2022-03-25T16:51:07Z

@sonasingh46 can you please squash?

shysank · 2022-03-25T17:04:54Z

@sonasingh46 were you able to find the cause of failure for the vmss e2e test?

shysank · 2022-03-25T22:28:18Z

/retest

sonasingh46 · 2022-03-27T13:16:14Z

@shysank -- Not sure why it failed but when I rebased and pushed again the test passed.

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

sonasingh46 · 2022-03-28T06:30:01Z

/retest

CecileRobertMichon · 2022-03-28T16:27:39Z

/retest

k8s-ci-robot · 2022-03-28T16:29:18Z

@sonasingh46: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-e2e-windows-dockershim	`c488770`	link	true	`/test pull-cluster-api-provider-azure-e2e-windows-dockershim`
pull-cluster-api-provider-azure-apidiff	`0b74770`	link	false	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

shysank · 2022-03-28T19:51:00Z

/lgtm
/approve

k8s-ci-robot · 2022-03-28T19:51:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shysank

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [shysank]

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 added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 17, 2021

k8s-ci-robot requested review from CecileRobertMichon and mboersma November 17, 2021 09:06

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Nov 17, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 17, 2021

sonasingh46 force-pushed the async_roleassignment branch from 35426b5 to 082d85c Compare November 22, 2021 11:04

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 22, 2021

sonasingh46 commented Nov 22, 2021

View reviewed changes

Comment thread azure/scope/machine.go Outdated

sonasingh46 commented Nov 22, 2021

View reviewed changes

Comment thread azure/services/roleassignments/roleassignments.go

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

sonasingh46 force-pushed the async_roleassignment branch from 082d85c to 1bbe5b6 Compare January 12, 2022 14:19

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 12, 2022

sonasingh46 force-pushed the async_roleassignment branch 2 times, most recently from 5b5d1d9 to 3073807 Compare January 19, 2022 16:37

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 19, 2022

sonasingh46 commented Jan 19, 2022

View reviewed changes

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

sonasingh46 commented Mar 21, 2022

View reviewed changes

sonasingh46 force-pushed the async_roleassignment branch 2 times, most recently from 1ad6487 to b9fcc7f Compare March 24, 2022 12:17

CecileRobertMichon reviewed Mar 24, 2022

View reviewed changes

Comment thread azure/services/roleassignments/spec.go

CecileRobertMichon reviewed Mar 25, 2022

View reviewed changes

k8s-ci-robot assigned shysank and CecileRobertMichon Mar 25, 2022

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

shysank mentioned this pull request Mar 25, 2022

pull-cluster-api-provider-azure-e2e failed when collecting logs #2197

Closed

sonasingh46 force-pushed the async_roleassignment branch from b9fcc7f to 71ac260 Compare March 27, 2022 11:43

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2022

chore(roleassignment): make roleassignments service async

0b74770

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

sonasingh46 force-pushed the async_roleassignment branch from 71ac260 to 0b74770 Compare March 27, 2022 13:18

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

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

k8s-ci-robot merged commit 9075098 into kubernetes-sigs:main Mar 28, 2022

k8s-ci-robot added this to the v1.3 milestone Mar 28, 2022

jackfrancis mentioned this pull request Jan 31, 2024

Add Jonathan Tong to maintainers #4526

Merged

4 tasks

Conversation

sonasingh46 commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Nov 17, 2021

Uh oh!

mboersma commented Nov 17, 2021

Uh oh!

Uh oh!

Uh oh!

sonasingh46 Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

sonasingh46 Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

Jont828 commented Jan 24, 2022

Uh oh!

sonasingh46 Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

shysank Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

sonasingh46 Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

sonasingh46 commented Mar 24, 2022

Uh oh!

sonasingh46 commented Mar 24, 2022

Uh oh!

k8s-ci-robot commented Mar 24, 2022

Uh oh!

Uh oh!

CecileRobertMichon left a comment

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Mar 25, 2022

Uh oh!

shysank commented Mar 25, 2022

Uh oh!

shysank commented Mar 25, 2022

Uh oh!

sonasingh46 commented Mar 27, 2022

Uh oh!

sonasingh46 commented Mar 28, 2022

Uh oh!

CecileRobertMichon commented Mar 28, 2022

Uh oh!

k8s-ci-robot commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shysank commented Mar 28, 2022

Uh oh!

k8s-ci-robot commented Mar 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sonasingh46 commented Nov 17, 2021 •

edited

Loading

k8s-ci-robot commented Mar 28, 2022 •

edited

Loading