perf-tests icon indicating copy to clipboard operation
perf-tests copied to clipboard

feat: improve network performance test to make it reusable and modifiable

Open ritwikranjan opened this issue 1 year ago • 17 comments

What type of PR is this?

/kind feature

What this PR does / why we need it:

This pull request includes significant updates to the network/benchmarks/netperf directory, focusing on improving the build process, enhancing test capabilities, and updating dependencies. The most important changes include modifying the Makefile to streamline Docker builds and test execution, updating the go.mod file with new dependencies, and adding new utility libraries for Kubernetes interactions.

Build and Test Improvements:

  • network/benchmarks/netperf/Makefile: Updated the Docker build process to use dynamic repository and image tags, added a test target for running Go tests, and introduced a localtest target for local test execution. [1] [2] [3]

Dependency Updates:

  • network/benchmarks/netperf/go.mod: Updated module path and dependencies to more recent versions, including k8s.io/api, k8s.io/apimachinery, and k8s.io/client-go.

New Utility Libraries:

  • network/benchmarks/netperf/lib/outputlib.go: Added functions for retrieving logs and data from Kubernetes pods and processing raw data into output files.
  • network/benchmarks/netperf/lib/testlib.go: Introduced a new library for handling test parameters, performing tests, and managing Kubernetes resources.
  • network/benchmarks/netperf/lib/utilslib.go: Added utility functions for setting up Kubernetes clients, creating services, and managing replication controllers.

Which issue(s) this PR fixes:

Fixes #2876

ritwikranjan avatar Oct 30 '24 15:10 ritwikranjan

Hi @ritwikranjan. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

k8s-ci-robot avatar Oct 30 '24 15:10 k8s-ci-robot

Hey @aojea, Can you take a look and let me know if it can be improved in any way?

ritwikranjan avatar Nov 04 '24 10:11 ritwikranjan

@wojtek-t do you know if this code is being used somewhere? I honestly can't remember and I will not have time until December to check on this, sorry

aojea avatar Nov 04 '24 13:11 aojea

Hi @aojea, Would you be able to take a look at these changes now?

ritwikranjan avatar Jan 13 '25 15:01 ritwikranjan

We need

  1. identify where are these tests running
  2. split the PR in independent commits, so it can be easy to review, the modification of the makefile should be independent of the commit that refactors the test code, ...
  3. explain how we probe these works (maybe we just need to know .1) and we merge and iterate, that is fine with me too

aojea avatar Jan 13 '25 15:01 aojea

Okay, sounds good to me, I'll go ahead and split into commits.

ritwikranjan avatar Jan 13 '25 15:01 ritwikranjan

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ritwikranjan Once this PR has been reviewed and has the lgtm label, please assign aojea for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 avatar Jan 13 '25 15:01 k8s-ci-robot

Hi @aojea, is there anything else I can do to better this?

ritwikranjan avatar Jan 22 '25 11:01 ritwikranjan

We need

  1. identify where are these tests running
  2. split the PR in independent commits, so it can be easy to review, the modification of the makefile should be independent of the commit that refactors the test code, ...
  3. explain how we probe these works (maybe we just need to know .1) and we merge and iterate, that is fine with me too

missing step .1 and 3. , how can we sure this does not break anything?

aojea avatar Jan 23 '25 09:01 aojea

@aojea For 1 & 3, I have not found any explicit place where these tests are used directly however I have made sure keep backward compatibility, all the targets in Makefile still give the same output and the older images of nptest also work in the same way. Whatever changes are made are either using extra flag (such as -json) or by keeping the api definitions intact. Now the proof that the changes work, we are currently using a fork Azure/perf-tests for testing an open source project Retina. Let me know if you want details of the implementations for the same. If you are aware of any other usage of this tool do let me know and i'll do test out those as well.

ritwikranjan avatar Jan 23 '25 14:01 ritwikranjan

/ok-to-test

do you think you can add a manual job in https://github.com/kubernetes/test-infra so we can at least now if it works?

aojea avatar Jan 23 '25 23:01 aojea

/retest

ritwikranjan avatar Jan 24 '25 13:01 ritwikranjan

@aojea I have addressed the failing linter tests.

I will add a couple of scripts in kubernetes/test-infra in experiment directory in order to verify both the cli as well the api. Thanks for taking the time to look into this.

ritwikranjan avatar Jan 24 '25 13:01 ritwikranjan

/retest

ritwikranjan avatar Jan 24 '25 14:01 ritwikranjan

Hi @aojea, anything else we want to consider here? Would like me to add the tests to test-infra first before merging or can I do it after?

ritwikranjan avatar Jan 27 '25 14:01 ritwikranjan

Would like me to add the tests to test-infra first before merging or can I do it after?

if we add after we don't know if it works, I really think it will be useful to have signal on this before merge if we can

aojea avatar Jan 27 '25 16:01 aojea

Hi @aojea , I have added a script: kubernetes/test-infra#34222

ritwikranjan avatar Jan 28 '25 14:01 ritwikranjan

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Apr 28 '25 14:04 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot avatar May 28 '25 15:05 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-triage-robot avatar Jun 27 '25 15:06 k8s-triage-robot

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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-sigs/prow repository.

k8s-ci-robot avatar Jun 27 '25 15:06 k8s-ci-robot