Network performance measurement: Implementing worker using CRD #1645

VivekThrivikraman-est · 2021-01-06T09:26:39Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
In the current implementation of network performance measurement, controller and worker communicates with each other using http, this change is for making the controller and worker communicate using custom resources. This PR contains the changes for worker portion.
Which issue(s) this PR fixes:

Fixes #1631

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-01-06T09:26:47Z

Welcome @VivekThrivikraman-est!

It looks like this is your first PR to kubernetes/perf-tests 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/perf-tests has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-01-06T09:26:47Z

Hi @VivekThrivikraman-est. 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.

wojtek-t · 2021-01-08T12:41:43Z

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

 	for _, line := range result {
+		klog.V(3).Info(line)


V(4) maybe?

wojtek-t · 2021-01-08T12:42:40Z

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

+		if !isPresent {
+			return nil, fmt.Errorf("property %v not present in request", parameter)
+		}
+		if reflect.TypeOf(value).Kind() == reflect.Int64 {


why do we need this?

The properties contains mix of string and int, but all of them are required to be converted to string.

Hmm - I see now. I'm not huge fan of this pattern.

Can we change the requestProperties to really be of type map[string]string and does this parsing elsewhere (in a dedicated function that will be extracting properties)?

In other words, there are exactly 3 parameters that are used across all command. I would really prefer to have a function like:

func extractParameters(map[string]interface{} input) map[string]string { result := map[string]string{} for _, param := range []string{"duration", "numberOfClients", "serverPodIP"} { // if appropriate param exists, extract to result. } }

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

wojtek-t · 2021-01-08T12:47:46Z

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

 )

 var (
+	protocolCommandMap = map[string]string{ProtocolHTTP: "siege", ProtocolTCP: "iperf", ProtocolUDP: "iperf"}


nit: please split the lines, i.e.

protocolCommandMap = map[string]string{ ProtocolHTTP: "siege", ProtocolTCP: "iperf", ProtocolUDP: "iperf" }

wojtek-t · 2021-01-08T12:58:27Z

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

 		klog.Infof("Value after resolving template %v: %v", parameter, arguments[i])
 	}
 	return arguments, nil
 }
+
+func getDynamicResource(gvk schema.GroupVersionKind, namespace string) (dynamic.ResourceInterface, error) {


maybe let's call it getResourceInterface ?

This method is no more there, but modified the variable as mentioned.

wojtek-t · 2021-01-08T12:59:20Z

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

-	protocol     string
-	workerDelay  time.Duration
-	startTime    time.Time
+	dynamicResource dynamic.ResourceInterface


resourceInterface ?

wojtek-t · 2021-01-08T13:00:26Z

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

+	var isPodNameAvailable bool
+	w.podName, isPodNameAvailable = os.LookupEnv("POD_NAME")
+	if !isPodNameAvailable {
+		w.updateStatus(map[string]interface{}{"error": "Pod name not set as environment variable"})


can you create a helper function:

makeErrorStatus(err error)

?

wojtek-t · 2021-01-08T13:04:05Z

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

-			w.sendResponse(rw, http.StatusInternalServerError, Status{Message: message})
-			return
+func (w *Worker) watchForWork() {
+	watcher, err := w.getWatcher()


That's not going to work universally:

watch starts at now, so if the object was created earlier you will never watch it

you don't handle all corner cases, resuming watches on timeouts/errors etc.

Instead of that - you should simply use the informer that will do all of that for you (and just register callbacks).
Here you have an example how to do that for dynamic clients:

perf-tests/clusterloader2/pkg/measurement/util/informer/informer.go

Line 54 in a0288cb

func NewDynamicInformer(

Modified to use informer.

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

wojtek-t · 2021-01-14T12:58:39Z

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

+		if !isPresent {
+			return nil, fmt.Errorf("property %v not present in request", parameter)
+		}
+		if reflect.TypeOf(value).Kind() == reflect.Int64 {


Hmm - I see now. I'm not huge fan of this pattern.

Can we change the requestProperties to really be of type map[string]string and does this parsing elsewhere (in a dedicated function that will be extracting properties)?

In other words, there are exactly 3 parameters that are used across all command. I would really prefer to have a function like:

func extractParameters(map[string]interface{} input) map[string]string { result := map[string]string{} for _, param := range []string{"duration", "numberOfClients", "serverPodIP"} { // if appropriate param exists, extract to result. } }

wojtek-t · 2021-01-14T13:04:12Z

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

+	w.podName, isPodNameAvailable = os.LookupEnv("POD_NAME")
+	if !isPodNameAvailable {
+		err := errors.New("pod name not set as environment variable")
+		klog.Fatal(err)


nit: klog.Fatalf(errors.New(...))

(don't create err var)

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

wojtek-t · 2021-01-14T13:09:10Z

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

-	if _, err := rw.Write(marshalledResponse); err != nil {
-		klog.Errorf("Error writing response to ResponseWriter: %v", err)
+	klog.Info("Recevied add event for resource with spec:", w.work.resourceSpec)
+	if w.podName == w.work.resourceSpec.ClientPodName {


switch w.podName { case w.work.resourceSpec.ClientPodName: .. case w.work.resourceSpec.ServerPodName: .. default: .. }

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

wojtek-t · 2021-01-14T13:16:51Z

/ok-to-test

This looks great - it's pretty close to mergable state. Thanks!

wojtek-t

Couple more small comments, but this is pretty close - thanks!

wojtek-t · 2021-01-18T08:35:20Z

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

 )

+var gvk = schema.GroupVersionKind{Group: "clusterloader.io", Kind: "NetworkTestRequest", Version: "v1"}


we shouldn't go directly to v1 - let's start with v1alpha1

wojtek-t · 2021-01-18T08:35:46Z

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

-	httpPort         = "5301"
-)
+// http server listen port.
+const httpPort = "5301"


nit: let's do:

const ( httpPort... namespace )

wojtek-t · 2021-01-18T08:40:45Z

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

-	startTime    time.Time
+	k8sClient         dynamic.Interface
+	resourceInterface dynamic.ResourceInterface
+	gvr               schema.GroupVersionResource


let's remove this one (see my comment below)

wojtek-t · 2021-01-18T08:41:52Z

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

-	w.listenToController()
+	w.initialize()
+	w.populatePodName()
+	w.registerForCustomResourceEvents(w.handleCustomResource)


Let's change to :

w.populatePodName() w.initialize()

and merge initialize and registerForCustomResourceEvents to reduce the amount of params

wojtek-t · 2021-01-18T08:44:04Z

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

-	protocol     string
-	workerDelay  time.Duration
-	startTime    time.Time
+	k8sClient         dynamic.Interface


once you merge initialize() and registerForCustomResourceEvents as suggested below, this one also will no longer be needed

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

wojtek-t

Sorry - I added few more comment, but I think it will make it more reliable and easier to follow.
But we are pretty close with this one.

wojtek-t · 2021-01-20T07:15:01Z

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

+type work struct {
+	resourceContent map[string]interface{}
+	workType        string
+	resourceSpecMap map[string]interface{}


Actually - let's remove this one - and just keep "resourceSpec" field below (it contains the same information).

It means that you need to change extractParameters method to take resourceSpec as input, but it's actually fine (and probably even easier to reason about).

wojtek-t · 2021-01-20T07:19:05Z

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

-		rw.WriteHeader(http.StatusInternalServerError)
+		err = fmt.Errorf("populating resource spec failed: %v", err)
+		w.updateStatus(makeErrorStatus(err))
+		klog.Error(err)


actually - it seems to be used in couple places, so let's add a helper method:

func (w *Worker) handleError(err error) { klog.Errorf(err) if err := w.updateStatus(...) { klog.Errorf(err) } }

and use it everywehre

and call it everywhere (

wojtek-t · 2021-01-20T07:19:22Z

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

+		w.work.workType = "server"
+	default:
+		klog.Error(errors.New("pod name not set as client or server"))
+		w.updateStatus(makeErrorStatus(err))


you're reporting the wrong error here - see also comment above

wojtek-t · 2021-01-20T07:21:21Z

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

-	w.startWork(rw, request, ProtocolHTTP, "siege", httpClientArguments)
+func (w *Worker) populateResourceSpec(object runtime.Object) error {
+	var err error
+	w.work.resourceContent, err = runtime.DefaultUnstructuredConverter.ToUnstructured(object)


Let's not store resourceContent in the "work" item - see my comment below.

wojtek-t · 2021-01-20T07:22:23Z

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

+func (w *Worker) updateStatus(status map[string]interface{}) {
+	w.work.resourceContent["status"] = status
+	var unstructuredRes unstructured.Unstructured
+	unstructuredRes.SetUnstructuredContent(w.work.resourceContent)


Instead of modifying resourceContent etc., let's simply use the
UpdateStatus method, instead of Update:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/dynamic/interface.go#L36

As informed ,have issues with UpdateStatus,hence for now not changing this.

I guess I wasn't clear offline. What I said is that, instead of storing "resourceContent" explicit in the Worker just do
(syntax to fix):

obj, err := w.resourceInterface.Get(context.TODO(), name)
// handle err
// transform to map[string]interface{}
// do what you do here

This let's you remove resourceContent from Worker struct too (without doing UpdateStatus).

wojtek-t · 2021-01-20T07:23:12Z

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

+	// TODO(@VivekThrivikraman-est): add retries.
+	_, err := w.resourceInterface.Update(context.TODO(), &unstructuredRes, metav1.UpdateOptions{})
+	if err != nil {
+		klog.Errorf("Error updating status: %v", err)


Actually - let's change this method to return this error and don't log anything here.

wojtek-t · 2021-02-08T12:21:42Z

LGTM

@VivekThrivikraman-est - please squash and I will approve

wojtek-t · 2021-02-10T08:27:17Z

/lgtm
/approve

k8s-ci-robot · 2021-02-10T08:27:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VivekThrivikraman-est, 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

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 6, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 6, 2021

k8s-ci-robot requested review from mm4tt and wojtek-t January 6, 2021 09:27

VivekThrivikraman-est changed the title ~~Implementing worker using CRD~~ Network performance measurement: Implementing worker using CRD Jan 6, 2021

wojtek-t reviewed Jan 8, 2021

View reviewed changes

wojtek-t reviewed Jan 14, 2021

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 Jan 14, 2021

wojtek-t reviewed Jan 18, 2021

View reviewed changes

wojtek-t reviewed Jan 20, 2021

View reviewed changes

Implementing worker using CRD

cda4b87

VivekThrivikraman-est force-pushed the netperf-msmt-utilImg branch from f17bb8f to cda4b87 Compare February 8, 2021 15:00

k8s-ci-robot assigned wojtek-t Feb 10, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2021

k8s-ci-robot merged commit f17c6d8 into kubernetes:master Feb 10, 2021

VivekThrivikraman-est mentioned this pull request Dec 7, 2021

REQUEST: New membership for VivekThrivikraman-est kubernetes/org#3135

Closed

7 tasks

		)

		var gvk = schema.GroupVersionKind{Group: "clusterloader.io", Kind: "NetworkTestRequest", Version: "v1"}

Network performance measurement: Implementing worker using CRD #1645

Network performance measurement: Implementing worker using CRD #1645

Conversation

VivekThrivikraman-est commented Jan 6, 2021

k8s-ci-robot commented Jan 6, 2021

k8s-ci-robot commented Jan 6, 2021

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

wojtek-t commented Jan 14, 2021

wojtek-t left a 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

wojtek-t left a 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

wojtek-t commented Feb 8, 2021

wojtek-t commented Feb 10, 2021

k8s-ci-robot commented Feb 10, 2021