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

refactor: Prepare controller code for watching single namespace #987

Merged
merged 12 commits into from
Jun 4, 2024
Merged

refactor: Prepare controller code for watching single namespace #987

merged 12 commits into from
Jun 4, 2024

Conversation

akrejcir
Copy link
Collaborator

@akrejcir akrejcir commented May 23, 2024 •
edited
Loading

What this PR does / why we need it:
This is a refactoring PR before limiting the operator to watching SSPs only from a single namespace.
The main goal of this is to create controllers before creating the Manager. Then in a future PR, each controller will expose information needed to configure cache to only watch certain namespaces or labels. It will improve memory load of the operator.

Which issue(s) this PR fixes:
Jira: https://issues.redhat.com/browse/CNV-38130
Jira: https://issues.redhat.com/browse/CNV-41247

Release note:

None

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels May 23, 2024
@kubevirt-bot kubevirt-bot requested review from 0xFelix and opokornyy May 23, 2024 12:25
@akrejcir
Copy link
Collaborator Author

/cc @0xFelix @ksimon1 @codingben

@kubevirt-bot kubevirt-bot requested review from codingben and ksimon1 May 23, 2024 12:25
@akrejcir
Copy link
Collaborator Author

/retest

@codingben
Copy link
Member

It will improve memory load of the operator.

Do you have some data about it, how much it improved memory load of the operator? What it was before, and will be after this PR is merged?

@akrejcir
Copy link
Collaborator Author

akrejcir commented May 28, 2024 •
edited
Loading

It will improve memory load of the operator.

Do you have some data about it, how much it improved memory load of the operator? What it was before, and will be after this PR is merged?

I don't have much data, but it makes sense. Currently the operator is caching all objects of certain type that are in the cluster, like Deployments, ServiceAccounts, Services, etc. Even those that are created by users and not related to CNV.
This means that memory requirements of the operator scale with the cluster size, which is not needed.

The only data I have is in the linked jira ticket: https://issues.redhat.com/browse/CNV-38130
The graph shows that after creating 2000 user workloads (Deployments, Services, ...) the memory load is around 190 MB.

@akrejcir
Copy link
Collaborator Author

This PR is only a refactoring. I'm still working on the change that will limit which objects are cached.

0xFelix
0xFelix reviewed May 28, 2024
View reviewed changes
internal/controllers/services_controller.go Outdated Show resolved Hide resolved
internal/controllers/setup.go Outdated Show resolved Hide resolved
internal/controllers/setup.go Outdated Show resolved Hide resolved
internal/controllers/ssp_controller.go Show resolved Hide resolved
internal/controllers/ssp_controller.go
@@ -113,32 +122,55 @@ func (r *sspReconciler) setupController(mgr ctrl.Manager) error {
builder := ctrl.NewControllerManagedBy(mgr)
watchSspResource(builder)

r.areCrdsMissing = len(r.crdList.MissingCrds()) > 0
s.areCrdsMissing = len(s.crdList.MissingCrds()) > 0
Copy link
Member

Choose a reason for hiding this comment

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

This could be missing CRDs of other Controllers too, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, thanks for noticing. I will open a separate issue to fix this.
It does not break anything, but the resulting error message can be confusing.

Copy link
Member

Choose a reason for hiding this comment

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

ACK

internal/env/env.go Show resolved Hide resolved
internal/env/env.go Show resolved Hide resolved
internal/operands/vm-console-proxy/reconcile_test.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
tests/service_controller_test.go Outdated Show resolved Hide resolved
akrejcir added 12 commits May 29, 2024 16:09
@akrejcir
chore: move environment variables to a separate package
3c80f36 
Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
feat: Use environment variable to get operator's namespace
ef20715 
Exposing the pod's namespace as an environment variable is cleaner
than reading a file.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
chore: move "controllers" directory to "internal" directory
3d7f398 
Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
chore: rename file webhook_controller_test.go
27a9880 
Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
refactor: Define "Controller" interface and implement it.
a1773b3 
Defined "Controller" interface that will be useful in future
commits, and implemented this interface for VM controller.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
refactor: Implement "Controller" for webhook controller
92cfe36 
Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
refactor: Implement "Controller" for service controller
8821969 
Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
refactor: Implement "Controller" for SSP controller
18980d7 
Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
refactor: Add crdList parameter to Controller.AddToManager()
4bc4399 
Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
refactor: Create controllers before adding them to manager
fe11ed2 
Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
refactor: create Controllers before Manager
880496d 
This will be useful in a future PR, where we setup the cache
in client according to what controllers need.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir
refactor: Rename function CreateVmController() to NewVmController()
a58ace9 
Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@akrejcir akrejcir force-pushed the controller-refactor branch from b14ccc4 to a58ace9 Compare June 3, 2024 13:11
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jun 3, 2024

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

0xFelix
0xFelix reviewed Jun 4, 2024
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

/approve

Thanks!

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2024
codingben
codingben approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@codingben codingben left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2024
@kubevirt-bot kubevirt-bot merged commit 777e0e4 into kubevirt:main Jun 4, 2024
12 checks passed
@akrejcir akrejcir deleted the controller-refactor branch June 4, 2024 10:42
@akrejcir akrejcir mentioned this pull request Jun 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xFelix 0xFelix 0xFelix left review comments

@codingben codingben codingben approved these changes

@opokornyy opokornyy Awaiting requested review from opokornyy

@ksimon1 ksimon1 Awaiting requested review from ksimon1

Assignees

@codingben codingben

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@akrejcir @codingben @kubevirt-bot @0xFelix