Api availability measurement

[vamossagar12]

This PR is for #1096 which created a new measurement to track the availability of api-server by probing the /healthz end point

[k8s-ci-robot]

Hi @vamossagar12. 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.

[mm4tt]

/ok-to-test

Thanks, @vamossagar12!

I'll try to take a look tomorrow. Adding Wojtek in case he has capacity to take a look today
/assign @wojtek-t

[oxddr]

/uncc

[wojtek-t]

@vamossagar12 - sorry for delay, I'm pretty overloaded. I will try to look into it today or tomorrow

[wojtek-t]

@vamossagar12 - made a pass on it, sorry for long delay

[vamossagar12]

Thanks @wojtek-t I have more or less implemented the changes you suggested. Just had a couple of questions post which I can push the changes.

[vamossagar12]

@wojtek-t i made the changes but not able to push them. Keep getting this error after rebasing with the latest from master:

.git/hooks/pre-push: line 11: ./verify/verify-boilerplate.sh: No such file or directory

I see some changes have been made recently in the /verify folder but not able to figure out what the problem is.

[wojtek-t]

@vamossagar12 - the verify-boilerplace.sh script still exists and wasn't touched:
https://github.com/kubernetes/perf-tests/blob/master/verify/verify-boilerplate.sh

BTW - the hooks must be something in your local environment - I don't have them set up.

[vamossagar12]

@vamossagar12 - the verify-boilerplace.sh script still exists and wasn't touched:
https://github.com/kubernetes/perf-tests/blob/master/verify/verify-boilerplate.sh

BTW - the hooks must be something in your local environment - I don't have them set up.

Well, actually I set the hooks up since they were mentioned in the README of perf-tests. If it isn't needed, I can remove it. Would it have any implications though?

[wojtek-t]

Well, actually I set the hooks up since they were mentioned in the README of perf-tests. If it isn't needed, I can remove it. Would it have any implications though?

We have presubmits anyway - so these should be caught by them anyway.

[wojtek-t]

Also please squash commits

[vamossagar12]

Also please squash commits

Done. Had a couple of questions.

[wojtek-t]

Sorry for delay - I was on vacation for last 2.5 weeks.

I added some comments, but i'm mostly fine with what is now. Please change the PR out of Draft and let's proceed with it.

[vamossagar12]

Sorry for delay - I was on vacation for last 2.5 weeks.

I added some comments, but i'm mostly fine with what is now. Please change the PR out of Draft and let's proceed with it.

No worries :) I have made the changes and also removed the PR out of draft.

[wojtek-t]

Please fix gofmt:
https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/perf-tests/1162/pull-perf-tests-verify-lint/1285548209413820419/build-log.txt

[both here and in the second file]

[vamossagar12]

I ran gofmt -s for these 2 files but this error still got thrown. Is that the right command?

[wojtek-t]

gofmt -w -s

[vamossagar12]

thank you.. ran the command and pushed again.

[wojtek-t]

Sorry - I should have caught that earlier.

Actually I would really prefer to have a single summary rather than N summaries for every master and cluster separately (see my comment below).

[wojtek-t]

This isn't yet addressed.

[wojtek-t]

Actually, I would prefer the output to be:

type apiAvailabilitySummary struct {
  IP                                           string
  AvailabilityPercentage           float32
  LongestUnavailableDuration time.Duration
}

type apiAvailabilityOutput struct {
  ClusterMetrics apiAvailabilitySummary
  MasterMetrics []apiAvailabilitySummary
}

[vamossagar12]

hi @wojtek-t , slight confusion here. The final output for MasterMetrics is an array here. If you see the output of the measurement that would be returned, all outputs would be consolidated into 1.

https://github.com/kubernetes/perf-tests/pull/1162/files#diff-769e85d1bdfa456cbbe72288f2ba2ab5R54-R55

Do you think this isn't similar to what you prefer?

[wojtek-t]

I care about 'availability' summary as a whole. Yes - I want a single summary, instead of N separate ones.
Alternatively, we may consider two measurements: one for the cluster, second for all masters.

[vamossagar12]

I see. I might want to make some tweaks to the apiAvailabilityMeasurement struct in that case. Currently it looks like this:

type apiAvailabilityMeasurement struct {
	isRunning           bool
	stopCh              chan struct{}
	hosts               []string
	summaries           []measurement.Summary
	clusterLevelMetrics *apiAvailabilityMetrics
	hostLevelMetrics    map[string]*apiAvailabilityMetrics
	wg                  sync.WaitGroup
}

So, if I understand correctly, instead of an array of summaries, you want 1 cluster level summary and array of MasterLevel summaries in a single object. I would also need to remove the clusterLevelMetrics and hostLevelMetrics from the above struct.

Regarding 2 separate measurements, do we expose them as separate measurements- which seems slightly confusing to me. With a unified measurement, a user would be able to get all availability metrics in one summary.

[wojtek-t]

Sorry - I meant "two summaries" - one for the cluster, second for all masters.
But, I think having a single summary is cleaner.

[vamossagar12]

ok. so in that case, i will make the code changes to have one summary in the format you explained above.

[vamossagar12]

@wojtek-t I have made the code changes to expose only 1 summary for both ClusterLevel and MasterMetrics. Plz review when you get the chance..

[wojtek-t]

This isn't yet addressed.

[wojtek-t]

I care about 'availability' summary as a whole. Yes - I want a single summary, instead of N separate ones.
Alternatively, we may consider two measurements: one for the cluster, second for all masters.

[wojtek-t]

@vamossagar12 - thanks, it's pretty close now; added two more comments

[vamossagar12]

Thanks @wojtek-t , I made those changes as well.

[wojtek-t]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

• clusterloader2/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