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

Initial commit of metrics for scaling #119

Merged
merged 2 commits into from
Jul 24, 2019

Conversation

dklyle
Copy link
Contributor

@dklyle dklyle commented Jul 22, 2019

The beginning for #117

Copy link

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

This lgtm
Thanks for putting this together @dklyle . I ran it and got the results (json) file I expected.
There are obviously lots of things we want to clean up and improve, but as a first step this will allow us to put together and test the rest of the infra whilst we improve the test itself.
Thus, I'm happy if we merge this and then improve it with further PRs.

@@ -0,0 +1,339 @@
#!/bin/bash
#
# Copyright (c) 2017,2018 Intel Corporation

Choose a reason for hiding this comment

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

+2019 - and for other files as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

source ${LIB_DIR}/kata-common.bash
source ${LIB_DIR}/json.bash
source /etc/os-release || source /usr/lib/os-release
KATA_KSM_THROTTLER="${KATA_KSM_THROTTLER:-no}"

Choose a reason for hiding this comment

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

I suspect we can drop any notion of the throttler - afaik it is not installed by kata-deploy when adding kata to k8s installs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed and the associated function

KATA_KSM_THROTTLER="${KATA_KSM_THROTTLER:-no}"

# Set variables to reasonable defaults if unset or empty
DOCKER_EXE="${DOCKER_EXE:-docker}"

Choose a reason for hiding this comment

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

I think we are going to eradicate all docker stuff from this code, yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

punted on this for now, will work on once this is merged

local rpath="$(get_docker_kata_path $RUNTIME)"
local json="$(cat << EOF
"kata-env" :
$($rpath kata-env --json)

Choose a reason for hiding this comment

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

We'll need to fix up (and write anew) this section, as it is currently pretty hard wired to try and get docker info. We want k8s info.
A quick fix/hack is to add the output of kubectl get nodes -o=json, which is way better than nothing - but longer term I wonder if we can find/extract more info to add here about the DUT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

punted on this for now


KATA_HYPERVISOR="${KATA_HYPERVISOR:-qemu}"

die() {

Choose a reason for hiding this comment

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

we have die,warn,info in two of the lib files - we should be able to factor those out now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed from common.bash as in sources kata-common.bash

@@ -0,0 +1,33 @@
apiVersion: apps/v1

Choose a reason for hiding this comment

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

We could add a copyright here.
@krsna1729 @mcastelino - do you have a standard idiom you use for this repo? This code is being cloned from over in the Kata code base, so obviously it (and I ;-) ) come with some pre-conceptions, like adding a copyright header to every file that can take one - but, you have the top level LICENSE file that covers the whole repo with Apache2.0 - so, we are probably good just having that file and individual file over-rides where necessary. what say you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

info "And grab some stats"
# Tell mpstat to measure over a short period, not only so we get slightly de-noised data, but also
# if you don't tell it the period, you will get the avg since boot, which is not what we want.
local cpu_idle=$(kubectl exec -ti ${stats_pod} -- sh -c "mpstat -u 3 1 | tail -1 | awk '{print \$11}'" | sed 's/\r//')

Choose a reason for hiding this comment

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

heh, I'd forgotten I wrote that! ;-)

# FIXME - time taken in 'seconds' - we can do better than that, if we
# use the date nanosecond stamp, and steal the magic from the density test script.
# switch to 'date +%N'
local start_time=$(date +%s)

Choose a reason for hiding this comment

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

the json.sh lib file has ms and ns time functions we can now use here I reckon..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to milliseconds

# kubelet config - from 'kubectl describe node -o json' ?

# Launch our stats gathering pod
kubectl apply -f ${stats_pod}.yaml

Choose a reason for hiding this comment

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

I've just found, we might want to prepend the ${SCRIPT_PATH} to the front of the yaml files etc., as if you don't run the test from within its own directory right now it fails to find stats.yaml for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that will be true for bb.yaml.in too, it's mergeable without updating, but will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated for bb.yaml.in, generated.yaml, and stats.yaml

info "Testing replicas ${reqs} of ${NUM_PODS}"
# Generate the next yaml file

local runtime_command

Choose a reason for hiding this comment

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

just noticed, I think indentation is bust in this section - it is all in the for loop, so some of the lines need to be pushed in one tab.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should incorporate these, maybe as git pre-commit hooks?
https://github.com/mvdan/sh
https://github.com/koalaman/shellcheck

Choose a reason for hiding this comment

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

@krsna1729 eek. maybe.... I think we opted out of shellcheck over in kata land as there were some things it moaned about that were particularly hard to get around iirc...

Let's see if anybody finds the time to try these out on the code.... and, reserve the right to not inflict them if they pain barrier will be too high ;-)

@grahamwhaley
Copy link

Merging - we will update and clean some other bits in other PRs as we grow and improve the code base.

@grahamwhaley grahamwhaley merged commit 0f39518 into clearlinux:master Jul 24, 2019
@dklyle dklyle deleted the add-metrics branch July 26, 2019 15:26
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.

3 participants