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

Remove controller runtime dependency #847

Merged
merged 8 commits into from
Feb 2, 2024
Merged

Remove controller runtime dependency #847

merged 8 commits into from
Feb 2, 2024

Conversation

XudongLiuHarold
Copy link
Member

@XudongLiuHarold XudongLiuHarold commented Jan 26, 2024 •
edited
Loading

What this PR does / why we need it:

  • Remove controller runtime dependency

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

fixes #840

Special notes for your reviewer:

Release note:

NONE

generate vmop clientset code
58f38be 
Signed-off-by: Xudong Liu <xudongl@vmware.com>
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 26, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: XudongLiuHarold

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:

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 k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 26, 2024
@XudongLiuHarold
remove controller runtime dependency
bb8c76e 
Signed-off-by: Xudong Liu <xudongl@vmware.com>
@XudongLiuHarold XudongLiuHarold force-pushed the remove-controller-runtime-dependency branch from d6d4f9e to bb8c76e Compare January 27, 2024 07:00
lint
74bde04 
Signed-off-by: Xudong Liu <xudongl@vmware.com>
@XudongLiuHarold XudongLiuHarold force-pushed the remove-controller-runtime-dependency branch 2 times, most recently from 1136300 to 0915a50 Compare January 29, 2024 05:20
remove unused wrapper
d0ef60c 
Signed-off-by: Xudong Liu <xudongl@vmware.com>
@XudongLiuHarold XudongLiuHarold force-pushed the remove-controller-runtime-dependency branch 2 times, most recently from 58e7427 to 503ec08 Compare January 30, 2024 05:01
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 30, 2024
@XudongLiuHarold XudongLiuHarold force-pushed the remove-controller-runtime-dependency branch 2 times, most recently from 9699897 to c624418 Compare January 30, 2024 10:18
@XudongLiuHarold
change to use dynamic client
37441be 
Signed-off-by: Xudong Liu <xudongliuharold@gmail.com>
@XudongLiuHarold XudongLiuHarold force-pushed the remove-controller-runtime-dependency branch from c624418 to 37441be Compare January 30, 2024 12:07
@XudongLiuHarold
fix lint
7cc41d5 
Signed-off-by: Xudong Liu <xudongliuharold@gmail.com>
@XudongLiuHarold XudongLiuHarold force-pushed the remove-controller-runtime-dependency branch from 753b98b to 7cc41d5 Compare January 30, 2024 12:41
DanielXiao
DanielXiao reviewed Jan 31, 2024
View reviewed changes
pkg/cloudprovider/vsphereparavirtual/vmoperator/client/virtualmachineservice_client.go

func (v *virtualMachineServices) Get(ctx context.Context, name string, opts v1.GetOptions) (*vmopv1alpha1.VirtualMachineService, error) {
virtualMachineService := &vmopv1alpha1.VirtualMachineService{}
if obj, err := v.client.Resource(VirtualMachineServiceGVR).Namespace(v.ns).Get(ctx, name, opts); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will create an feature request to use dynamic informer for get and list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we don't need to watch virtualmachine & virtualmachineservcie resources, we may not need informer and lister here?

Copy link
Collaborator

@DanielXiao DanielXiao Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It depends on how frequently get/list is called. In general, a controller should call get/list from cache to reduce API calls on kube-apiserver even it does not watch those resources. Controller-runtime by default add the resource to informer cache at the first time get/list is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see! Yeah, we need a follow-up for this, thanks @DanielXiao !

DanielXiao
DanielXiao reviewed Jan 31, 2024
View reviewed changes
go.mod Show resolved Hide resolved
@DanielXiao
Copy link
Collaborator

@XudongLiuHarold Thanks and this is great. I would also invite @sbueringer @chrischdi to take a look.

add unit tests for client
1071d14 
Signed-off-by: Xudong Liu <xudongl@vmware.com>
@k8s-ci-robot k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 31, 2024
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 31, 2024
@XudongLiuHarold XudongLiuHarold changed the title WIP: Remove controller runtime dependency Remove controller runtime dependency Jan 31, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2024
@XudongLiuHarold XudongLiuHarold force-pushed the remove-controller-runtime-dependency branch from 017bdc7 to d861bcd Compare January 31, 2024 05:47
@XudongLiuHarold
Copy link
Member Author

/retest

@sbueringer
Copy link
Member

I'll try to find time tomorrow to take a look

sbueringer
sbueringer reviewed Feb 1, 2024
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Just a few nits. Looks pretty good

pkg/cloudprovider/vsphereparavirtual/vmoperator/client/virtualmachine_client_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/vsphereparavirtual/vmoperator/client/virtualmachine_client_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/vsphereparavirtual/vmoperator/client/virtualmachine_client_test.go Show resolved Hide resolved
resolve comment
1006123 
Signed-off-by: Xudong Liu <xudongl@vmware.com>
@XudongLiuHarold XudongLiuHarold force-pushed the remove-controller-runtime-dependency branch from d861bcd to 1006123 Compare February 2, 2024 05:31
@XudongLiuHarold
Copy link
Member Author

/retest

@YanzhaoLi
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2024
@k8s-ci-robot k8s-ci-robot merged commit 649791d into kubernetes:master Feb 2, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielXiao DanielXiao DanielXiao left review comments

@sbueringer sbueringer sbueringer left review comments

@imkin imkin Awaiting requested review from imkin

@maplain maplain Awaiting requested review from maplain

Assignees

@YanzhaoLi YanzhaoLi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove controller-runtime dependency
5 participants
@XudongLiuHarold @k8s-ci-robot @DanielXiao @sbueringer @YanzhaoLi