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

Add scalability tests #2239

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Add scalability tests #2239

wants to merge 32 commits into from

Conversation

d-uzlov
Copy link
Contributor

@d-uzlov d-uzlov commented Jul 16, 2021

Issue

#1015

Requirements

These tests use few programs that may be not present in your environment:
gnuplot using container for this instead of direct installation
styx
Also, these tests depend on icmp-responder having support for multiple network services: networkservicemesh/cmd-nse-icmp-responder#246 merged.

How to run

There is one 4 scenarios with configurable parameters.
It's possible to create several tests, one for each set of parameters, but since parameters are arbitrary, it would be inconvenient to have 4×M×N×K×R×Q test setups, so it's probably better to use one setup and set the params before running it.

func setTestParams(netsvc, nse, nsc int, remote bool) string {
	filename := "../deployments-k8s/examples/scalability/cases/set_params.sh"
	filecontent := `#!/bin/bash
TEST_NS_COUNT=` + strconv.Itoa(netsvc) + `
TEST_NSE_COUNT=` + strconv.Itoa(nse) + `
TEST_NSC_COUNT=` + strconv.Itoa(nsc) + `
TEST_REMOTE_CASE=` + strconv.FormatBool(remote)
	err := ioutil.WriteFile(filename, []byte(filecontent), 0777)
	if err != nil {
		panic(err)
	}
	testName := "ns=" + strconv.Itoa(netsvc) + ",nse=" + strconv.Itoa(nse) + ",nsc=" + strconv.Itoa(nsc) + ",remote=" + strconv.FormatBool(remote)
	return testName
}

func TestScalability(t *testing.T) {
	t.Run(setTestParams(1, 1, 15, true), func(t *testing.T) { suite.Run(t, &scalability.Suite{})})
}

@d-uzlov d-uzlov marked this pull request as ready for review July 19, 2021 11:32
@edwarnicke
Copy link
Member

@d-uzlov Could you look into pure Go plotting alternatives to gnuplot? I'd like to have a pure Go solution here, so we don't start the accumulation of new commands things for our tests...

@d-uzlov
Copy link
Contributor Author

d-uzlov commented Jul 19, 2021

@edwarnicke sure. I'll have to research it, but from what I see there are plenty of golang frameworks for plotting, so it shouldn't be hard to draw something nice from Go.

@d-uzlov
Copy link
Contributor Author

d-uzlov commented Jul 20, 2021

@edwarnicke
Question: which dependencies can we accept, and which do we want to avoid?
Let me clarify what I mean.

We have several options for plot drawing to choose from.
For example, gonum/plot, vdobler/chart, wcharczuk/go-chart, go-echarts, etc.
Also we have an option of Arafatk/glot, which is just a wrapper over the gnuplot.

The last one got me thinking.

It would be most easy to transfer what I already have to to glot, for obvious reasons, but it wouldn't really be a "pure Go" solution. But the only visible difference would be that we would still need to do a sudo apt-get install gnuplot call before tests.

For example, go-echarts generates html files with interactive charts. It also uses an external dependency, however, its a JS library dependency that is hidden from us by the web browser. It's arguably more complex and less common dependency than gnuplot. It's still wouldn't be a "pure Go" solution but it doesn't have any visible dependencies.

The gnuplot dependency is not that uncommon. Using gnuplot is the first solution that you could find in google for drawing charts.

So, would, for example go-echarts be an acceptable dependency?
And would it be a better dependency than gnuplot, or not?

@edwarnicke
Copy link
Member

@d-uzlov sudo apt-get install gnuplot is what we are trying to avoid...

On go-echarts... does it produce results that can be embedded into a Github Issue etc as an image? Or does it require full on JS machinery?

@d-uzlov
Copy link
Contributor Author

d-uzlov commented Jul 20, 2021

@edwarnicke
From what I understand, go-echarts is created specifically to generate interactive JS-based charts, and I don't think it's capable of drawing images on itself.
It could be interesting to get an interactive chart, where you can hide some of the lines on the fly, but I mainly used it as an example of another type of dependency. There are alternatives that are designed to draw images, some of which I listed in my previous comment. I'll probably just pick one of them.

Also, another question on dependencies. Please, consider this just as my curiosity, for the sake of choosing dependencies in the future.
Does any of the following change your opinion on the usage of gnuplot?

  1. During the community meeting call you said that you want to avoid apt-get stuff because this way we assume that user at least have apt-get, which is often not the case.
    While such commands are obviously unacceptable for tests that should "just work", I don't think any such thing is intended to be part of tests. It must be a part of setup, like golang installation, kubectl installation, which is usually platform-dependent.
  2. It's possible to run gnuplot in a docker container (that is, if we have docker available on the test machine). This way we wouldn't need to install anything at all.

d-uzlov added 13 commits July 26, 2021 17:05
Signed-off-by: Danil Uzlov <[email protected]>
Signed-off-by: Danil Uzlov <[email protected]>
Signed-off-by: Danil Uzlov <[email protected]>
Signed-off-by: Danil Uzlov <[email protected]>
Signed-off-by: Danil Uzlov <[email protected]>
Signed-off-by: Danil Uzlov <[email protected]>
Signed-off-by: Danil Uzlov <[email protected]>
Signed-off-by: Danil Uzlov <[email protected]>
@edwarnicke
Copy link
Member

It's possible to run gnuplot in a docker container (that is, if we have docker available on the test machine). This way we wouldn't need to install anything at all.

This seems like a good potential solution to the problem.

d-uzlov added 5 commits July 28, 2021 16:42
Signed-off-by: Danil Uzlov <[email protected]>
Signed-off-by: Danil Uzlov <[email protected]>
Signed-off-by: Danil Uzlov <[email protected]>
Signed-off-by: Danil Uzlov <[email protected]>
@d-uzlov d-uzlov marked this pull request as draft July 28, 2021 12:45
@d-uzlov
Copy link
Contributor Author

d-uzlov commented Jul 28, 2021

Marking this as draft until I solve issues with measuring time on remote clusters with high connection delay.

@d-uzlov d-uzlov marked this pull request as ready for review July 29, 2021 04:55
@d-uzlov
Copy link
Contributor Author

d-uzlov commented Jul 29, 2021

I believe that generally this PR is ready. However, this issue makes troubles:

Scalability tests should work regardless, but until CPU issue if fixed, the results will be pretty ugly.

EVENT_TEXT_CONNECTIONS_1_READY="Conns 1 ready"
```
```bash
sleep 15
Copy link

@Bolodya1997 Bolodya1997 Jul 29, 2021

Choose a reason for hiding this comment

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

Why do we need these sleeps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I added sleeps into tests, my main intent was to capture performance after different phases of the test. For example, to make sure, that when we capture that request/heal finishes, it no longer consumes CPU.

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.

4 participants