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

Optmize node watcher in daemon #634

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alan-kut
Copy link
Contributor

@alan-kut alan-kut commented Feb 13, 2024

Node watcher is very heavy when watching
all the nodes, not only the current one.
Nodes are regularly updated, e.g. by kubelet.

Watch only the current node all the time and
other nodes only when holding the drain lock.

Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

Node watcher is very heavy when watching
all the nodes, not only the current one.
Nodes are regularly updated, e.g. by kubelet.

Watch only the current node all the time and
other nodes only when holding the drain lock.

Signed-off-by: Alan Kutniewski <[email protected]>
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7889385247

Details

  • -38 of 50 (24.0%) changed or added relevant lines in 1 file are covered.
  • 8 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 30.011%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/daemon/daemon.go 12 50 24.0%
Files with Coverage Reduction New Missed Lines %
pkg/daemon/daemon.go 8 35.46%
Totals Coverage Status
Change from base Build 7875814354: -0.2%
Covered Lines: 3538
Relevant Lines: 11789

💛 - Coveralls

@alan-kut alan-kut marked this pull request as draft February 13, 2024 16:16
@alan-kut alan-kut marked this pull request as ready for review February 15, 2024 09:13
@alan-kut
Copy link
Contributor Author

/test-all

@adrianchiris
Copy link
Collaborator

adrianchiris commented Feb 15, 2024

@alan-kut currently there is work around moving the drain logic to a centralized controller[1]

I think that part of this work, we can limit the node informer to just the current node. WDYT ?

@SchSeba ^^ Thoughts?

[1] #555

@SchSeba
Copy link
Collaborator

SchSeba commented Feb 19, 2024

sure we can do it after we merge #555

@SchSeba
Copy link
Collaborator

SchSeba commented Feb 20, 2024

Hi @alan-kut thanks for the PR!

so the idea base on #555 is that we are not going to watch the node anymore the daemon will only watch the nodeState and in the future also the operator will not watch the nodes as we are going to move the drain request annotation to the nodeState only

@SchSeba
Copy link
Collaborator

SchSeba commented Apr 4, 2024

Hi @alan-kut can you take a look on the operator now that we remove the nodes update from the config daemon we have it only on the controller

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.

None yet

4 participants