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

tracker: improve the kubelet test coverage #109717

Open
ffromani opened this issue Apr 29, 2022 · 33 comments
Open

tracker: improve the kubelet test coverage #109717

ffromani opened this issue Apr 29, 2022 · 33 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/node Categorizes an issue or PR as relevant to SIG Node.

Comments

@ffromani
Copy link
Contributor

There is a strong and growing consensus in sig-node about the need of increasing the test coverage in the kubelet, improving the current testsuite about coverage and reliability. See for example the April 26 sig-node meeting notes and the April 27 sig-node CI subgroup meeting notes.

This work has already begun and there are already some PR posted (see: #108024 (review)). This is a great start. There are more areas in the kubelet, especially in the container manager and resource manager area, that can use better unit test coverage.

Besides the obvious benefits of documenting and preserving the current behaviour, adding tests is meant to lower the barrier for future work and contributions.

Examples of some areas which can benefit of more tests:

  • k/k/pkg/kubelet/cm/internal_container_lifecycle.go
  • k/k/pkg/kubelet/cm/internal_container_lifecycle_linux.go
  • k/k/pkg/kubelet/cm/pod_container_manager_linux.go
  • k/k/pkg/kubelet/cm/qos_container_manager_linux.go
  • everything coverage-driven (check test code coverage and build from there)
  • error paths in general
  • e2e tests in general
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 29, 2022
@k8s-ci-robot
Copy link
Contributor

@fromanirh: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@ffromani
Copy link
Contributor Author

/sig node
/good-first-issue
/help

@k8s-ci-robot
Copy link
Contributor

@fromanirh:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/sig node
/good-first-issue
/help

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.

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 29, 2022
@ffromani
Copy link
Contributor Author

@endocrimes @SergeyKanzhelev I think this can help us as tracking issue and to involve more contributors

@ffromani
Copy link
Contributor Author

@prakharporwal I think this can be a very nice way to start contributing in the node area (xref: #109595 (comment))

@prakharporwal
Copy link

Sure can you assign this to me will start working on this.

@prakharporwal
Copy link

/assign

@ffromani
Copy link
Contributor Author

/assign @prakharporwal

@STRRL
Copy link
Contributor

STRRL commented May 2, 2022

Hi @fromanirh, @prakharporwal I am also interested in this issue, and I saw that there several sub-tasks. Maybe I could also participate in resolving this?

@ffromani
Copy link
Contributor Author

ffromani commented May 2, 2022

@STRRL hi! Indeed there are several subtasks and I think there is ample space for collaboration.

@STRRL
Copy link
Contributor

STRRL commented May 2, 2022

/assign

@prakharporwal
Copy link

Hi ! Sure @STRRL. I am working on

  • k/k/pkg/kubelet/cm/internal_container_lifecycle.go
  • k/k/pkg/kubelet/cm/internal_container_lifecycle_linux.go
    right now.

@fromanirh I raised a draft PR please you take a look. I created 1 test for internal container lifecycle PreStartContainer. I am a little confused onto what to assert on once the PreStartContainer function runs. What will change when this function runs ? What do we want to test for ?

@STRRL
Copy link
Contributor

STRRL commented May 2, 2022

Hi ! Sure @STRRL. I am working on

k/k/pkg/kubelet/cm/internal_container_lifecycle.go
k/k/pkg/kubelet/cm/internal_container_lifecycle_linux.go
right now.

I would take a look at the rest files. :)

@yuv00191
Copy link

yuv00191 commented May 2, 2022

Hi @fromanirh, @prakharporwal, @STRRL I am also interested in this issue, and I saw that there several sub-tasks. Maybe I could also participate in resolving this?

l

@wqld
Copy link
Contributor

wqld commented May 6, 2022

Hi @prakharporwal @STRRL @yuv00191

  • k/k/pkg/kubelet/cm/qos_container_manager_linux.go

If no one is working it yet, can I do the work related to that?

@rusik69
Copy link
Contributor

rusik69 commented May 8, 2022

/assign

@MorrisLaw
Copy link
Member

@JayKayy this may have some room for another contributor, if you're interested.

@ffromani
Copy link
Contributor Author

@JayKayy this may have some room for another contributor, if you're interested.

To give readers more context: basically any go source code with missing or insufficient unit test coverage is a good candidate. Some source files are harder than others to unit-test, and we will probably need to create or improve mocks/fakes. But we should start this process anyway, so totally help is welcome in this area.

@JayKayy
Copy link
Contributor

JayKayy commented May 12, 2022

Sure I would like to get involved. I start looking at the coverage

@zabrox
Copy link

zabrox commented May 23, 2022

/assign

@halfrost
Copy link

/assign

@browdues
Copy link

browdues commented Jul 11, 2022

Hello all. I'm looking to begin contributing to this project and I saw this designated as good first issue. Before I dive in, is it safe to begin looking at coverage for pod_manager.go or is someone else investigating?

@loktev-d
Copy link
Contributor

loktev-d commented Sep 5, 2022

/assign

@varksvader
Copy link

Hi! I was wondering if you guys needed more help on test coverage here.

@PauloGoncalvesLima
Copy link
Contributor

Hi @fromanirh 😄,

I would like to contribute to this issue, there is anything that I could do?

@DeeptanshuDas
Copy link

/assign

@sourcelliu
Copy link
Contributor

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/node Categorizes an issue or PR as relevant to SIG Node.
Projects
None yet