Adding network performance measurement testcases. (util-image changes) #1593

hanamantagoudvk · 2020-11-24T10:26:48Z

What type of PR is this?

/kind feature
/kind regression

What this PR does / why we need it:

This review is one of the changeset done as part of the enhancement/feature (Reference #1498)

This changeset has worker-pod implementation.

k8s-ci-robot · 2020-11-24T10:26:56Z

Hi @hanamantagoudvk. 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/test-infra repository.

hanamantagoudvk · 2020-11-24T14:08:26Z

/cc @aojea

wojtek-t · 2020-11-25T10:22:37Z

util-images/network/netperfbenchmark/Dockerfile

@@ -0,0 +1,31 @@
+FROM golang:1.15.2-alpine3.12 AS build-env


Why do you need alpine?

Can't you simply use golang:1.15.2?

There are frequent security vulneraibilities in those more complex images.

Changed build env to use golang image(for running we need to install tools hence that image isn't changed).

wojtek-t · 2020-11-25T10:24:06Z

util-images/network/netperfbenchmark/Dockerfile

@@ -0,0 +1,31 @@
+FROM golang:1.15.2-alpine3.12 AS build-env
+ARG gopkg=k8s.io/perf-tests/util-images/phases/netperfbenchmark


what is "phases" here and below?

Initial implementation was using phases and this is coming from there, will update appropriately.

wojtek-t · 2020-11-25T10:25:48Z

util-images/network/netperfbenchmark/Makefile

@@ -0,0 +1,17 @@
+PROJECT = k8s-testimages
+IMG = gcr.io/$(PROJECT)/netperfbenchmark
+TAG = v0.0.1


Will update the version.

wojtek-t · 2020-11-25T10:27:29Z

util-images/network/netperfbenchmark/cmd/main.go

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+/*


by convention we generally don't use /* */ style comments for anything else than the copyright

Please switch to:

// Package ...

BTW - this comment is useless - it's obvious what main is doing :)

Removed the comment.

wojtek-t · 2020-11-25T10:28:21Z

util-images/network/netperfbenchmark/go.mod

@@ -0,0 +1,7 @@
+module k8s.io/perf-tests/util-images/network/netperfbenchmark
+
+go 1.12


Can we go higher than 1.12 - it's already out of support window IIRC

Can we use 1.15?

Changed it to 1.15.

wojtek-t · 2020-11-25T10:35:48Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+}
+
+func startListening(port string, handlers map[string]func(http.ResponseWriter, *http.Request)) {
+	klog.Info("In StartHTTPServer")


This comment isn't meaningful...

wojtek-t · 2020-11-25T10:36:25Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+}
+
+//Metrics returns the metrics collected
+func Metrics(res http.ResponseWriter, req *http.Request) {


I'm also going to wait with further review until you rewrite it to be better structured (comment I added in line 42).

Have made the changes as suggested.

wojtek-t · 2020-11-25T10:39:29Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+		createResp(startWrkResponse{error: "missing/invalid required parameters"}, &res)
+		return
+	}
+	go schedule(protocolUDP, ts, dur, "iperf", []string{"-s", "-f", "K", "-u", "-e", "-i", dur, "-P", numcl})


There is significant code duplication - instead can you share the code and just (just call it with different parameters)

Modified the code as suggested.

wojtek-t · 2020-11-25T10:40:34Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	}
+}
+
+func parseTCP(result []string) []float64 {


Those parse* functions - can you move them to util.go file or sth like that

Modified as suggested.

wojtek-t · 2020-11-25T10:42:49Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	return sumResult
+}
+
+func parseUDP(result []string) []float64 {


There is huge code duplication between those parse methods.

Can we please reuse that more?

Modified the code for reusing logic.

aojea · 2020-11-26T07:48:48Z

util-images/network/netperfbenchmark/go.mod

+go 1.12
+
+require (
+	k8s.io/klog v0.2.0


Changed it to 1.0.0.

aojea · 2020-11-26T07:53:05Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+		return
+	}
+	go schedule(protocolTCP, ts, dur,
+		"iperf", []string{"-c", destIP, "-f", "K", "-l",


please add a comment explaining the parameters, something like this would work:

// -c destIP connect to destIP // -f K format Kbits // -l 20 read/write buffer size 20 butes // -b 1M bandwidth 1 Mbits // -i 1 repert stats every 1 sec // -t dur run durring dur seconds

The modified code has commands for all protocols declared as part of variable declaration together. Hence have put comment for each tool type with all options we have used. Please let me know if that would suffice.

aojea · 2020-11-26T07:53:55Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+		createResp(startWrkResponse{error: "missing/invalid required parameters"}, &res)
+		return
+	}
+	go schedule(protocolTCP, ts, dur, "iperf", []string{"-s", "-f", "K", "-i", dur, "-P", numcl})


add comment explaining the iperf commands please

Modified as described in previous comment.

aojea · 2020-11-26T07:57:36Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+		createResp(startWrkResponse{error: "missing/invalid required parameters"}, &res)
+		return
+	}
+	go schedule(protocolTCP, ts, dur, "iperf", []string{"-s", "-f", "K", "-i", dur, "-P", numcl})


also, this iperf -s will never die, I didn't go further in the code, sorry, how do you exit it?

We are giving option to wait for specified num of clients before closing, hence after the required number of clients have connected(and collected metrics), iperf would close the server.

aojea · 2020-11-26T08:01:40Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	resultStatus <- "OK"
+}
+
+func parseResult() ([]float64, error) {


just a question, should not be easier to parse the iperf output in csv format?

-y, --reportstyle C|c if set to C or c report results as CSV (comma separated values)

iperf2 documentation(https://iperf.fr/iperf-doc.php#doc) has not listed this option, hence didn't notice this option. This would have really helped reducing the parsing code. But after trying with -y looks like its not listing all the metrics, I was not able to see Latency metrics returned for UDP. Will see further on if I am missing something.

k8s-ci-robot · 2020-11-28T08:01:43Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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. I understand the commands that are listed here.

wojtek-t · 2020-11-30T08:26:51Z

util-images/network/netperfbenchmark/cmd/main.go

+)
+
+func main() {
+	klog.Infof("Worker Pod started")


Either make the comment more meaningful (what Pod etc.) or probably even better remove it.

wojtek-t · 2020-11-30T08:27:39Z

util-images/network/netperfbenchmark/pkg/util/util.go

+limitations under the License.
+*/
+
+// Package util contains utility methods for worker


nit: let's remove this comment

wojtek-t · 2020-11-30T08:28:04Z

util-images/network/netperfbenchmark/cmd/main.go

+
+func main() {
+	klog.Infof("Worker Pod started")
+	worker := worker.Worker{}


Let's maybe create a helper NewWorker method in worker pkg.

wojtek-t · 2020-11-30T08:28:49Z

util-images/network/netperfbenchmark/pkg/util/util.go

+
+// Number of metrics for each protocol
+const (
+	tcpMtrCnt   = 2


Please don't use abbreviations.

tcpMetricsCount

and so on

wojtek-t · 2020-11-30T08:30:41Z

util-images/network/netperfbenchmark/pkg/util/util.go

+	iperfTCPFn = []string{"", "", "", "Sum", "Avg"}
+)
+
+// ParseResult parses the response received for each protocol type


Those aren't universal utilities.

Let's move these to pkg/worker (those are helper functions for worker in fact).
Actually, let's move this whole file there

wojtek-t · 2020-11-30T08:50:17Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+
+// StartUDPServer starts iperf server for udp measurements
+func (w *Worker) StartUDPServer(res http.ResponseWriter, req *http.Request) {
+	klog.Info("In StartUDPServer")


Starting UDP Server

wojtek-t · 2020-11-30T08:51:45Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	w.startWork(&res, req, util.ProtocolUDP, "iperf", udpServerCmd)
+}
+
+// StartUDPClient starts iperf server for udp measurements


Can you please sort them somehow (first HTTP, TCP, UDP, server before client or whatever)?

wojtek-t · 2020-11-30T08:53:39Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+
+// Handler handles http requests for http measurements
+func (w *Worker) Handler(res http.ResponseWriter, req *http.Request) {
+	fmt.Fprintf(res, "hi\n")


Writing locally seems strange - you should write to response writer?

Have modified inline with other responses, but I thought Fprintf writes to the ResponseWriter itself in this case.

wojtek-t · 2020-11-30T08:54:05Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+}
+
+// StartTCPClient starts iperf client for tcp measurements
+func (w *Worker) StartTCPClient(res http.ResponseWriter, req *http.Request) {


Instead of calling it "res", let's call it "rw" (for ResponseWriter).
Same everywhere else

wojtek-t · 2020-11-30T08:57:18Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+func (w *Worker) createResponse(resp interface{}, wr *http.ResponseWriter) {
+	klog.Info("Inside create resp")
+	(*wr).Header().Set("Content-Type", "application/json")
+	(*wr).WriteHeader(http.StatusOK)


Actually, sending StatusOK if the response has an error seems wrong to me.

The createResponse shouldn't be type-agnostic and should send the correct http code.

wojtek-t · 2020-12-04T08:21:42Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+
+// startResponse is the response sent back to controller on starting a measurement
+type startWorkResponse struct {
+	error string


That doesn't address the question - you can serialize error to string.
[Though, I'm actually fine with leaving string here.]

That said, I would suggest redoing it to the following:

type MetricsResponse struct { Result []float64 // (the question is whether we don't want to be more explicit here, but we can leave it for now // Maybe also StartTime - but see my comment below first. } type Status struct { Message string }

And basically:

if you return non-200 code, then you simply return Status message

if you return 200 code, for start you don't return anything, for metrics you return MetricsResponse

wojtek-t · 2020-12-04T08:25:12Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+}
+
+func (w *Worker) listenToController() {
+	h := map[string]func(http.ResponseWriter, *http.Request){


nit:

type handlersMap map[string]func(http.ResponseWriter, *http.Request)

and use handlersMap here and below

wojtek-t · 2020-12-04T08:26:32Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	}
+	err := http.ListenAndServe(":"+port, nil)
+	if err != nil {
+		klog.Fatalf("Failed starting http server for port: %v, Error: %v", port, err)


nit:

Failed to start http server on port %v: %v", port, err)

wojtek-t · 2020-12-04T08:28:14Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	for urlPath, handler := range handlers {
+		http.HandleFunc(urlPath, handler)
+	}
+	err := http.ListenAndServe(":"+port, nil)


nit:

if err := ...; err != nil {

wojtek-t · 2020-12-04T08:34:08Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	var status string
+	select {
+	case status = <-w.resultStatus:
+		if status != "OK" {


This is mixing couple things in the same variable - the fact that we already finished processing and the result.

It would be much cleaner patter to have:

type Worker struct { result []float64 err error stopCh chan(struct{}) ...

And basically:

stopCh is a channel that defines you whether you already finished processing or not

result, err stores the result

So here you will have:

select { case <- w.stopCh: // send metrics default: // send metrics collection in progress

wojtek-t · 2020-12-04T08:49:17Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+
+// ParseResult parses the response received for each protocol type
+func ParseResult(protocol string, result []string, testDuration string) ([]float64, error) {
+	klog.Infof("Parsing response for %v", protocol)


This log doesn't bring any value.

wojtek-t · 2020-12-04T08:49:24Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+}
+
+func parseIperfResponse(result []string, testDuration string, protocol string, metricCount int, zeroFormat string, fractionZeroFormat string, operators []string) []float64 {
+	klog.Info("Parsing iperf response")


wojtek-t · 2020-12-04T08:51:05Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+			continue
+		}
+		split := strings.Split(formattedString, " ")
+		//for bug in iperf tcp


What bug? I don't understand what you're trying to achieve.

Here you really need comment what is the bug and what youre trying to achieve.

BTW - it should be done the other way - if this condition is not met, continue (to reduce nesting in the code).

Updated the comment and modified the code.

wojtek-t · 2020-12-04T08:53:27Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+				}
+				tmp, err := strconv.ParseFloat(v, 64)
+				if err != nil {
+					klog.Errorf("Conversion error %v", err)


should we really proceed here?

Added return with error

wojtek-t · 2020-12-04T09:06:58Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+	aggregatedResult := make([]float64, 0, httpMetricsCount)
+	multiSpaceRegex := regexp.MustCompile(`\s+`)
+	for _, op := range result {
+		if canAppend != true && strings.HasPrefix(op, "Transactions:") {


IIUC, you're trying to remove all lines before "Transactions:" and all after "Shortest transaction".

It would be much cleaner, to first trim the "result" value and do the actually parsing here.
Something like:

result = trimSiegeResponse(result)

trimSiegeResponse {
var beginIndex, endIndex int
for beginIndex := 0; beginIndex < len(result) && !strings.HasPrefix("Transactions":); beginIndex++ {}
if beginIndex == len(result) {
return nil
}
beingIndex++
for endIndex := beginIndex; endIndex < len(result) && !strings.hasPrefix("Short..."); endIndex++ {}
return result[beginIndex:endIndex]
}

wojtek-t · 2020-12-06T19:40:15Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+type Worker struct {
+	resultStatus chan string
+	result       []string
+	testDuration string


Yes - please make that duration.

The rationale is that in our tests we have conventions of passing duration (e.g. 1s, 2m, etc.). And conversions will allow you to use whatever format is needed below.

wojtek-t · 2020-12-06T19:42:19Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+			return
+		}
+		klog.Info("Metrics collected")
+		parsedResult, err := ParseResult(w.protocol, w.result, w.testDuration)


Please extract this parsing to a separate method.

And let's call it from schedule(), not from here.

wojtek-t · 2020-12-06T19:45:18Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	}
+}
+
+func (w *Worker) sendResponse(response interface{}, rw http.ResponseWriter, statusCode int) {


It would be more intuitive to switch order of parameters to:

rw, statusCode, response

[It's basically the order in which you use them]

wojtek-t · 2020-12-06T19:52:48Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	klog.Infof("About to wait for futuretime: %v", futureTimestamp)
+	klog.Infof("Current time: %v", time.Now().Unix())
+	time.Sleep(time.Duration(futureTimestamp-time.Now().Unix()) * time.Second)
+	w.startedAt = time.Now()


What exactly you would like to check with it?
I don't understand what this can be useful for.

wojtek-t · 2020-12-06T19:54:04Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	}
+	go w.schedule(protocol, tsint, valMap["duration"], command, arguments)
+	w.sendResponse(nil, rw, http.StatusOK)
+	return


remove - it's not needed

wojtek-t · 2020-12-06T20:03:38Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+		split := strings.Split(formattedString, " ")
+		// iperf gives aggregated result in a row with "SUM", but many fields are not getting aggregated
+		// hence not depending on iperf's aggregation
+		// if len(split) >= metricCount+1 && "SUM" != split[1] && split[2] == zeroFormat+"-"+testDuration+fractionZeroFormat {


util-images/network/netperfbenchmark/pkg/worker/util.go

wojtek-t · 2020-12-06T20:08:53Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+}
+
+// GetValuesFromURL returns a map with values parsed from http request,for attributes specified in paramsToSearch
+func GetValuesFromURL(request *http.Request, parametersToSearch []string) (map[string]string, error) {


Does it have to be public?

wojtek-t · 2020-12-06T20:10:54Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+	for beginIndex = 0; beginIndex < len(result) && !strings.HasPrefix(result[beginIndex], "Transactions:"); beginIndex++ {
+	}
+	if beginIndex == len(result) {
+		return nil, errors.New("Error parsing Siege response")


unexpected Siege response: lack of Transactions: line

wojtek-t · 2020-12-06T20:11:02Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+	for endIndex = beginIndex + 1; endIndex < len(result) && !strings.HasPrefix(result[endIndex], "Shortest transaction"); endIndex++ {
+	}
+	if endIndex == len(result) {
+		return nil, errors.New("Error parsing Siege response")


wojtek-t

I think we're getting relatively close with this PR. Hopefully 1-2 more passes.

wojtek-t · 2020-12-07T12:56:19Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	case <-w.stopCh:
+		if w.err != nil {
+			klog.Errorf("Error collecting metrics: %v", w.err)
+			w.sendResponse(rw, http.StatusInternalServerError, status{Message: "metrics collection failed: " + w.err.Error()})


nit:

message := fmt.Sprintf("metrics collection failed: %v", w.err)
w.sendResponse(rw, http.Statu..., status{Message: message})

wojtek-t · 2020-12-07T12:56:40Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+			w.sendResponse(rw, http.StatusInternalServerError, status{Message: "metrics collection failed: " + w.err.Error()})
+			return
+		}
+		klog.Info("Metrics collected")


nit: let's remove

wojtek-t · 2020-12-07T12:57:51Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+		var reply metricResponse
+		reply.Result = w.parsedResult
+		timeDifference := w.futureTime.Sub(w.startedAt)
+		reply.WorkerStartTime = "StartedAt: " + w.startedAt.String() + " FutureTime: " + w.futureTime.String() + " diffTime: " + timeDifference.String()


I still don't understand why do you need this. Why passing "FutureTime" (i.e. the time when real processing really started" isn't enough?

Can you answer this question?

We wanted the workers to start measuring at almost the same time, especially for the N:1 case. When value of N is high we suspect the time of starting measurement of 1st worker and nth worker could differ by many seconds.

So that I understand. And I buy that.

But that is why you pass the timestamp to the worker when it should start it's work. But that is future time.
[And I understand what it is for.]

I'm asking what do you need "startedAt" for.
This is the time at which worker received the request but didn't really do anything useful.
I don't understand what this one is for and you still didn't answer that.

wojtek-t · 2020-12-07T13:01:55Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+		if !strings.Contains(formattedString, zeroFormat+"-"+testDuration+fractionZeroFormat) {
+			continue
+		}
+		split := strings.Split(formattedString, " ")


Can you encapsulate the above 5 lines or so into a separate functIon?

Like parseIperfLine or sth like that?

wojtek-t · 2020-12-07T13:05:43Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+		for i, v := range split {
+			if i == 1 {
+				sessionID[v] = true
+				continue


sessionID[split[1]] = true

before the for loop

wojtek-t · 2020-12-07T13:07:07Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+				continue
+			}
+			//first index and hte last is ""
+			if i == 0 || i == 2 || i == metricCount+3 {


given this and the above, it would clear to have:

for i := 3; i < len(split); i++ { ... }

Also - how many items in split can be? Can there be more than "metricsCount+3"?

Modified the loop to avoid specific checks (metricsCount+3 would be the last element in split).

wojtek-t · 2020-12-07T13:07:48Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+			}
+			tmp, err := strconv.ParseFloat(v, 64)
+			if err != nil {
+				klog.Errorf("Conversion error %v", err)


Let's print what you were trying to parse - otherwise you will never debug it.

wojtek-t · 2020-12-07T13:10:11Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+	for _, op := range result {
+		formattedString := multiSpaceRegex.ReplaceAllString(op, " ")
+		split := strings.Split(formattedString, ":")
+		klog.Infof("Formatted: %v", formattedString)


wojtek-t · 2020-12-07T13:10:36Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+		formattedString := multiSpaceRegex.ReplaceAllString(op, " ")
+		split := strings.Split(formattedString, ":")
+		klog.Infof("Formatted: %v", formattedString)
+		if len(split) > 1 {


if len(split) <= 1 {
continue
}

to avoid nesting

wojtek-t · 2020-12-07T13:10:57Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+		split := strings.Split(formattedString, ":")
+		klog.Infof("Formatted: %v", formattedString)
+		if len(split) > 1 {
+			split := strings.Split(split[1], " ")


name collision - please rename to avoid confusion

wojtek-t · 2020-12-09T12:41:27Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	select {
+	case <-w.stopCh:
+		if w.err != nil {
+			klog.Errorf("Error collecting metrics: %v", w.err)


let's not log it here - instead, let's log it where you set w.err (in schedule function)

wojtek-t · 2020-12-09T12:42:20Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+			w.sendResponse(rw, http.StatusInternalServerError, status{Message: message})
+			return
+		}
+		var reply metricResponse


nit:

reply := metricResponse{ Result: w.parsedResult, WorkerDelay: w.workerDelay, }

wojtek-t · 2020-12-09T12:42:50Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+		reply.WorkerDelay = w.workerDelay
+		w.sendResponse(rw, http.StatusOK, reply)
+	default:
+		klog.Info("Metric collection in progress")


let's remove this log

Removed the log.

wojtek-t · 2020-12-09T12:43:00Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	rw.Header().Set("Content-Type", "application/json")
+	marshalledResponse, err := json.Marshal(response)
+	if err != nil {
+		klog.Infof("Error marshalling to json: %v", err)


util-images/network/netperfbenchmark/pkg/worker/worker.go

wojtek-t · 2020-12-09T13:00:48Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+			continue
+		}
+		sessionID[split[1]] = true
+		//Index 0,1,2 and last are not metrics


// The first three and the last items are not metrics in iperf summary.

wojtek-t · 2020-12-09T13:01:30Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+}
+
+func parseIperfLine(line string, zeroFormat string, fractionZeroFormat string, testDuration string) []string {
+	unitRegex := regexp.MustCompile(`%|\[\s+|\]\s+|KBytes\s+|KBytes/sec\s*|sec\s+|pps\s*|ms\s+|/|\(|\)\s+`)


make those file variables, instead of creating them in every call to the function

wojtek-t · 2020-12-09T13:03:47Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+			continue
+		}
+		parameter := argument[strings.Index(argument, "{")+1 : strings.Index(argument, "}")]
+		klog.Infof("arg with template: %v", parameter)


merge this log with the below one

wojtek-t · 2020-12-09T13:04:12Z

util-images/network/netperfbenchmark/Makefile

@@ -0,0 +1,17 @@
+PROJECT = k8s-testimages
+IMG = gcr.io/$(PROJECT)/netperfbenchmark
+TAG = 0.01


wojtek-t · 2020-12-09T13:04:48Z

util-images/network/netperfbenchmark/Makefile

+.PHONY: push
+push: build
+	docker push $(IMG):$(TAG)
+	docker push $(IMG):latest


Do we really need latest? Let's not push latest for now (same for build).

wojtek-t

Let me make another pass tomorrow - but I think we're pretty close with this PR now.

util-images/network/netperfbenchmark/pkg/worker/worker.go

wojtek-t

Couple more minor comments.

wojtek-t · 2020-12-10T15:59:58Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+}
+
+// metricResponse is the response sent back to controller after collecting measurement
+type metricResponse struct {


Given that it's actually kind-of API, let's make it public.

[Also fix comment then]

wojtek-t · 2020-12-10T16:00:05Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+}
+
+// status is the response sent back to controller when the request is unsuccessful
+type status struct {


Same here - make public

wojtek-t · 2020-12-10T16:00:49Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+// Start starts the worker
+func (w *Worker) Start() {
+	w.stopCh = make(chan struct{})
+	// TODO(#1631): make controller and worker communicate using k8s API(with CustomResources)


nit: whitespace after API (i.e. "... API (Custom...")

wojtek-t · 2020-12-10T16:02:05Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+
+// Handler handles http requests for http measurements
+func (w *Worker) Handler(rw http.ResponseWriter, request *http.Request) {
+	w.sendResponse(rw, http.StatusOK, "hi")


nit: let's change "hi" to "ok"

wojtek-t · 2020-12-10T16:02:31Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	w.sendResponse(rw, http.StatusOK, "hi")
+}
+
+func (w *Worker) startWork(rw http.ResponseWriter, request *http.Request, protocol string, command string, arguments []string) {


nit:

... protocol, command string,

wojtek-t · 2020-12-10T16:02:54Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	w.sendResponse(rw, http.StatusOK, nil)
+}
+
+func (w *Worker) schedule(startTimestamp int64, duration string, protocol string, command string, arguments []string) {


nit:

... protocol, command string,

wojtek-t · 2020-12-10T16:05:59Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+	return aggregatedResult, nil
+}
+
+func parseIperfLine(line string, zeroFormat string, fractionZeroFormat string, testDuration string) []string {


only line parameter is used

wojtek-t · 2020-12-10T16:06:39Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+	interval := zeroFormat + "-" + testDuration + fractionZeroFormat
+	for _, line := range result {
+		split := parseIperfLine(line, zeroFormat, fractionZeroFormat, testDuration)
+		// iperf gives aggregated result in a row with "SUM", but many fields are not getting aggregated


nit: please shorter the line (e.g break after not)

wojtek-t · 2020-12-10T16:08:34Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+}
+
+func parseIperfLine(line string, zeroFormat string, fractionZeroFormat string, testDuration string) []string {
+	formattedString := hyphenSpaceRegex.ReplaceAllString(multiSpaceRegex.ReplaceAllString(unitRegex.ReplaceAllString(line, " "), " "), "-")


nit: can you please shorten the line by splitting it?
e.g.

noUnitsLine := unitRegex.ReplaceAllString(line, " ") formatterLine := hyphenSpaceRegex.ReplaceAllString(multiSpaceRegex.ReplaceAllString(noUnitsLine, " "), "-")

wojtek-t · 2020-12-10T16:10:55Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+
+func trimSiegeResponse(result []string) ([]string, error) {
+	var beginIndex, endIndex int
+	for beginIndex = 0; beginIndex < len(result) && !strings.HasPrefix(result[beginIndex], "Transactions:"); beginIndex++ {


nit:

isBeginLine := func(index int) { return strings.HasPrefix(result[index], "Transactions:") } for beginIndex = 0; beginIndex < len(result) && !isBeginLine(beginIndex); beginIndex++ { }

same for end

wojtek-t

Some very minor comments.

Also please squash commits.

wojtek-t · 2020-12-10T17:41:03Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+	}
+}
+
+func parseIperfResponse(result []string, testDuration string, protocol string, metricCount int, zeroFormat string, fractionZeroFormat string, operators []string) ([]float64, error) {


result []string, testDuration string, metricCount int, protocol, zeroFormat, fractionZeroFormat string, operators []string

wojtek-t · 2020-12-10T17:42:57Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+	for _, searchString := range parametersToSearch {
+		val := values.Get(searchString)
+		if val == "" {
+			return nil, errors.New("missing required parameters")


what parameter?

fmt.Errorf("missing URL parameter: %s", param)

wojtek-t · 2020-12-10T17:43:05Z

util-images/network/netperfbenchmark/pkg/worker/util.go

+func getValuesFromURL(request *http.Request, parametersToSearch []string) (map[string]string, error) {
+	values := request.URL.Query()
+	paramMap := make(map[string]string)
+	for _, searchString := range parametersToSearch {


s/searchString/param/

util-images/network/netperfbenchmark/Dockerfile

wojtek-t · 2020-12-10T17:44:08Z

util-images/network/netperfbenchmark/Dockerfile

+RUN cd iperf-2.0.9 && ./configure --prefix=/usr/local --bindir /usr/local/bin && make && make install
+
+
+ENV VERSION=3.1.4


SIEGE_VERSION

wojtek-t · 2020-12-10T17:46:19Z

/ok-to-test

wojtek-t · 2020-12-10T17:56:26Z

@hanamantagoudvk - please also fix lint issues:
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/perf-tests/1593/pull-perf-tests-verify-lint/1337091296522670080

wojtek-t

Last few minor comments - once addressed I will approve.

wojtek-t · 2020-12-11T16:09:08Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	"k8s.io/klog"
+)
+
+// Worker hold data required for facilitating measurement


Please finish comments with a dot ("... measurement.")

Same everywhere else below and above.

wojtek-t · 2020-12-11T16:10:29Z

util-images/network/netperfbenchmark/pkg/worker/worker.go

+	}
+	timestamp, err := strconv.ParseInt(valMap["timestamp"], 10, 64)
+	if err != nil {
+		klog.Infof("Invalid timestamp: %v %v", valMap["timestamp"], err)


klog.Errorf("Invalid timestamp %v: %v", valMap["timestamp"], err)

wojtek-t · 2020-12-11T17:35:00Z

/lgtm
/approve

Thanks

k8s-ci-robot · 2020-12-11T17:35:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hanamantagoudvk, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aojea · 2020-12-11T18:09:34Z

awesome 👏

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 24, 2020

k8s-ci-robot requested review from gmarek and wojtek-t November 24, 2020 10:27

k8s-ci-robot requested a review from aojea November 24, 2020 14:08

wojtek-t reviewed Nov 25, 2020

View reviewed changes

aojea reviewed Nov 26, 2020

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 27, 2020

hanamantagoudvk changed the base branch from master to release-1.12 November 28, 2020 07:58

hanamantagoudvk changed the base branch from release-1.12 to master November 28, 2020 07:58

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 28, 2020

wojtek-t reviewed Nov 30, 2020

View reviewed changes

hanamantagoudvk changed the title ~~[WIP] Adding network performance measurement testcases. (util-image changes)~~ Adding network performance measurement testcases. (util-image changes) Nov 30, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2020

wojtek-t reviewed Dec 4, 2020

View reviewed changes

wojtek-t self-assigned this Dec 6, 2020

wojtek-t reviewed Dec 6, 2020

View reviewed changes

wojtek-t reviewed Dec 7, 2020

View reviewed changes

wojtek-t reviewed Dec 9, 2020

View reviewed changes

util-images/network/netperfbenchmark/pkg/worker/worker.go Show resolved Hide resolved

util-images/network/netperfbenchmark/pkg/worker/worker.go Show resolved Hide resolved

wojtek-t reviewed Dec 10, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 10, 2020

VivekThrivikraman-est force-pushed the netperf-msmt-utilImg branch 2 times, most recently from 7af3fb8 to 3618317 Compare December 11, 2020 14:17

wojtek-t reviewed Dec 11, 2020

View reviewed changes

Util-image changes consisting of worker-pod code

de11670

VivekThrivikraman-est force-pushed the netperf-msmt-utilImg branch from 3618317 to de11670 Compare December 11, 2020 17:15

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2020

k8s-ci-robot merged commit 7093fc4 into kubernetes:master Dec 11, 2020

		@@ -0,0 +1,31 @@
		FROM golang:1.15.2-alpine3.12 AS build-env
		ARG gopkg=k8s.io/perf-tests/util-images/phases/netperfbenchmark

		@@ -0,0 +1,7 @@
		module k8s.io/perf-tests/util-images/network/netperfbenchmark

		go 1.12

		RUN cd iperf-2.0.9 && ./configure --prefix=/usr/local --bindir /usr/local/bin && make && make install


		ENV VERSION=3.1.4

Adding network performance measurement testcases. (util-image changes) #1593

Adding network performance measurement testcases. (util-image changes) #1593

Conversation

hanamantagoudvk commented Nov 24, 2020

k8s-ci-robot commented Nov 24, 2020

hanamantagoudvk commented Nov 24, 2020

Choose a reason for hiding this comment

VivekThrivikraman-est Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VivekThrivikraman-est Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VivekThrivikraman-est Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VivekThrivikraman-est Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VivekThrivikraman-est Nov 25, 2020 •

edited

Loading

VivekThrivikraman-est Nov 25, 2020 •

edited

Loading

VivekThrivikraman-est Nov 30, 2020 •

edited

Loading

VivekThrivikraman-est Dec 4, 2020 •

edited

Loading