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.

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

[YUNIKORN-2850] Watch configmap only in yunikorn's deployed namespace #942

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

400Ping
Copy link

@400Ping 400Ping commented Nov 29, 2024

What is this PR for?

Currently, Yunikorn uses configmap informer to handle configuration hot reload.

However, In current implementation informer watches all namespaces even only need to watch namespace in which yunikorn is deployed. It causes in efficient behavior when sync and cache configmap states. If there is too many unrelated configmap in other namespace cause long recovery time to list and memory presure to handle configmap caches which is redundant.

So, If we could replace configmap informer to namespace restricted one, it would improve startup / recovery time and reduce memory usage.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2850

@400Ping
Copy link
Author

400Ping commented Nov 29, 2024

Here is my make test results:
~/yunikorn/yunikorn-k8shim git:[YUNIKORN-2850]
make test
running unit tests
"go" clean -testcache
"go" test ./pkg/... -cover -race -tags deadlock -coverprofile="build/coverage.txt" -covermode=atomic
? github.com/apache/yunikorn-k8shim/pkg/admission/common [no test files]
github.com/apache/yunikorn-k8shim/pkg/cmd/admissioncontroller coverage: 0.0% of statements
ok github.com/apache/yunikorn-k8shim/pkg/admission 45.583s coverage: 78.0% of statements
ok github.com/apache/yunikorn-k8shim/pkg/admission/conf 4.441s coverage: 69.5% of statements
ok github.com/apache/yunikorn-k8shim/pkg/admission/metadata 13.545s coverage: 93.4% of statements
ok github.com/apache/yunikorn-k8shim/pkg/admission/pki 50.927s coverage: 69.6% of statements
ok github.com/apache/yunikorn-k8shim/pkg/cache 17.593s coverage: 82.0% of statements
ok github.com/apache/yunikorn-k8shim/pkg/cache/external 4.491s coverage: 68.3% of statements
ok github.com/apache/yunikorn-k8shim/pkg/client 9.061s coverage: 11.2% of statements
? github.com/apache/yunikorn-k8shim/pkg/common/constants [no test files]
github.com/apache/yunikorn-k8shim/pkg/cmd/schedulerplugin coverage: 0.0% of statements
github.com/apache/yunikorn-k8shim/pkg/cmd/shim coverage: 0.0% of statements
github.com/apache/yunikorn-k8shim/pkg/cmd/webtest coverage: 0.0% of statements
github.com/apache/yunikorn-k8shim/pkg/common/test coverage: 0.0% of statements
github.com/apache/yunikorn-k8shim/pkg/plugin coverage: 0.0% of statements
ok github.com/apache/yunikorn-k8shim/pkg/common 5.519s coverage: 100.0% of statements
ok github.com/apache/yunikorn-k8shim/pkg/common/events 4.315s coverage: 28.6% of statements
ok github.com/apache/yunikorn-k8shim/pkg/common/utils 8.628s coverage: 100.0% of statements
ok github.com/apache/yunikorn-k8shim/pkg/conf 5.240s coverage: 83.8% of statements
ok github.com/apache/yunikorn-k8shim/pkg/dispatcher 13.004s coverage: 82.5% of statements
ok github.com/apache/yunikorn-k8shim/pkg/locking 5.969s coverage: 100.0% of statements
ok github.com/apache/yunikorn-k8shim/pkg/log 21.850s coverage: 82.5% of statements
ok github.com/apache/yunikorn-k8shim/pkg/plugin/predicates 5.934s coverage: 83.1% of statements
ok github.com/apache/yunikorn-k8shim/pkg/plugin/support 7.255s coverage: 35.3% of statements
ok github.com/apache/yunikorn-k8shim/pkg/shim 32.819s coverage: 67.5% of statements
ok github.com/apache/yunikorn-k8shim/pkg/webtest 5.713s coverage: 77.8% of statements
"go" vet "github.com/apache/yunikorn-k8shim/pkg"...

@400Ping
Copy link
Author

400Ping commented Nov 30, 2024

I see, I accidentally changed the imports. I will change it back now.

Signed-off-by: 400Ping <[email protected]>
Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

I don't understand this PR in its current form. Besides the test cases, there are two main changes:

  1. informers.go: it's just reformatting
  2. main.go: bail out if namespace = "". But even the original code passes the namespace to admission.NewInformers().

So what's the fundamental change here? What am I missing?

I also disagree with the main.go modification because that would result in an inconsistent behavior: the admission controller would crash loop if namespace == "" but the scheduler wouldn't.

pkg/admission/informers.go Outdated Show resolved Hide resolved
@400Ping 400Ping marked this pull request as draft December 1, 2024 04:00
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.02%. Comparing base (e289196) to head (0c529a2).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/shim/scheduler.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #942      +/-   ##
==========================================
- Coverage   68.05%   68.02%   -0.04%     
==========================================
  Files          70       70              
  Lines        9194     9194              
==========================================
- Hits         6257     6254       -3     
- Misses       2730     2732       +2     
- Partials      207      208       +1     

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

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

-1. You cannot scope the entire informer factory to a single namespace. This is used by all predicate logic, and must be able to read Kubernetes objects from multiple namespaces.

@400Ping
Copy link
Author

400Ping commented Dec 9, 2024

I see, I am working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants