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.

Sign up for GitHub

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

Jump to bottom

Various changes to perf code #3037

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+57 −11
Open

Various changes to perf code #3037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
643546c
small fixes
Stevenjin8 Sep 12, 2024
2ee663f
Add ambient profile
Stevenjin8 Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion perf/benchmark/configs/istio/ext_authz/installation.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,4 @@ spec:
telemetry:
enabled: true
v2:
enabled: true
enabled: true
20 changes: 15 additions & 5 deletions perf/benchmark/graph_plotter/graph_plotter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ def plotter(args):

df = pd.read_csv(args.csv_filepath)
telemetry_modes_y_data = {}
if not args.telemetry_modes:
args.telemetry_modes = df["Labels"].unique()

metric_name = get_metric_name(args)
constructed_query_str = get_constructed_query_str(args)

Expand All @@ -41,6 +44,8 @@ def plotter(args):
fig = plt.figure(figsize=(1138 / dpi, 871 / dpi), dpi=dpi)
ax = fig.add_subplot(111)
ax.set_ylim(0, 1.0)
if args.title:
ax.set_title(args.title)
for key, val in telemetry_modes_y_data.items():
plot_key = key
match key:
Expand Down Expand Up @@ -107,15 +112,15 @@ def get_data_helper(df, query_list, query_str, telemetry_mode, metric_name):
try:
data[metric_name].head().empty
except KeyError as e:
y_series_data.append(None)
y_series_data.append(0)
else:
if not data[metric_name].head().empty:
if metric_name.startswith('cpu') or metric_name.startswith('mem'):
y_series_data.append(data[metric_name].head(1).values[0])
else:
y_series_data.append(data[metric_name].head(1).values[0] / data["ActualQPS"].head(1).values[0])
y_series_data.append(data[metric_name].head(1).values[0] / 1000)
Copy link
Member

Choose a reason for hiding this comment

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

not sure why it was using data["ActualQPS"] before but maybe comment on the change?
(also fortio data, in the json files, is in seconds not ms (even if the UI shows in ms for human consumption) so not sure what the 1000 is for either?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some digging I think this is correct (but a comment would help). Fortio reports data in seconds, but the script that pull data from the fortio client converts it to microseconds

tools/perf/benchmark/runner/fortio.py

Lines 60 to 64 in 2030a1d

p = h["Percentiles"]
for pp in p:
obj["p" + str(pp["Percentile"]).replace(".", "")
] = int(pp["Value"] * 10 ** 6)

so I have to divide by 1000 to get it back to milliseconds

else:
y_series_data.append(None)
y_series_data.append(0)

return y_series_data

Expand Down Expand Up @@ -174,15 +179,20 @@ def get_parser():
parser.add_argument(
"--query_str",
help="Specify the qps or conn query_str that will be used to query your y-axis data based on the CSV file."
"For example: conn_query_str=ActualQPS==1000, qps_query_str=NumThreads==16."
"For example: conn_query_str=ActualQPS==1000, qps_query_str=NumThreads==16.",
Copy link
Member

Choose a reason for hiding this comment

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

not sure what that example is but if ActualQPS is from fortio json, it's very unlikely to be a nice exact round number - if you ask for 1000 you're likely to get 999.97 or some such

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think my formatter might have caught this. Having the hard equals isn't great, I agree. But it gets rounded to an int, so if you run it for long enough, the actual qps ends up being the desired qps (most of the time).

default=""
)
parser.add_argument(
"--csv_filepath",
help="The path of the CSV file."
)
parser.add_argument(
"--graph_title",
help="The graph title."
help="Output path."
)
parser.add_argument(
"--title",
help="Visual title of graph."
)
return parser

Expand Down
2 changes: 1 addition & 1 deletion perf/benchmark/runner/fortio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ def sync_fortio(url, table, selector=None, promUrl="", csv=None, csv_output="",
if gd.get('errorPercent', 0) > 10:
print("... Run resulted in", gd['errorPercent'], "% errors")
continue
min_duration = METRICS_START_SKIP_DURATION + METRICS_END_SKIP_DURATION
min_duration = 5 # METRICS_START_SKIP_DURATION + METRICS_END_SKIP_DURATION
if min_duration > gd['ActualDuration']:
print("... {} duration={}s is less than minimum {}s".format(
gd["Labels"], gd['ActualDuration'], min_duration))
Expand Down
10 changes: 8 additions & 2 deletions perf/benchmark/setup_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ RBAC_ENABLED="false"
SERVER_REPLICA="${SERVER_REPLICA:-1}"
CLIENT_REPLICA="${CLIENT_REPLICA:-1}"
ISTIO_INJECT="${ISTIO_INJECT:-false}"
AMBIENT_ENABLED="${AMBIENT_ENABLED:-false}"
LINKERD_INJECT="${LINKERD_INJECT:-disabled}"
INTERCEPTION_MODE="${INTERCEPTION_MODE:-REDIRECT}"
FORTIO_SERVER_INGRESS_CERT_ENABLED="${FORTIO_SERVER_INGRESS_CERT_ENABLED:-false}"
Expand All @@ -55,12 +56,13 @@ function setup_test() {
--set rbac.enabled="${RBAC_ENABLED}" \
--set namespace="${NAMESPACE}" \
--set loadGenType="${LOAD_GEN_TYPE}" \
--set excludeOutboundIPRanges=$(pod_ip_range)\
--set includeOutboundIPRanges=$(svc_ip_range) \
Stevenjin8 marked this conversation as resolved.
Show resolved Hide resolved
--set server.replica="${SERVER_REPLICA}" \
--set client.replica="${CLIENT_REPLICA}" \
--set server.inject="${ISTIO_INJECT}" \
--set client.inject="${ISTIO_INJECT}" \
--set server.waypoint="${WAYPOINT_ENABLED}" \
--set server.ambient="${AMBIENT_ENABLED}" \
--set client.ambient="${AMBIENT_ENABLED}" \
--set server.injectL="${LINKERD_INJECT}" \
--set client.injectL="${LINKERD_INJECT}" \
--set domain="${DNS_DOMAIN}" \
Expand Down Expand Up @@ -92,6 +94,10 @@ then
kubectl label namespace "${NAMESPACE}" istio-injection=enabled --overwrite || true
fi

if [[ "$AMBIENT_ENABLED" == "true" ]]
then
kubectl label namespace "${NAMESPACE}" istio.io/dataplane-mode=ambient --overwrite || true
fi
if [[ "$LINKERD_INJECT" == "enabled" ]]
then
kubectl annotate namespace "${NAMESPACE}" linkerd.io/inject=enabled || true
Expand Down
25 changes: 24 additions & 1 deletion perf/benchmark/templates/fortio.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,32 @@ spec:
kind: ServiceRole
name: "{{ $ruleName }}"
{{- end }}

{{- define "fortio" }}

---
{{- if and .Values.server.waypoint .Values.server.ambient }}
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
name: {{ $.name }}-wp
labels:
istio.io/waypoint-for: service
spec:
gatewayClassName: istio-waypoint
listeners:
- name: mesh
port: 15008
protocol: HBONE
{{- end }}
---
apiVersion: v1
kind: Service
metadata:
name: {{ $.name }}
{{- if and .Values.server.waypoint .Values.server.ambient }}
labels:
istio.io/use-waypoint: {{ $.name }}-wp
{{- end }}
spec:
ports:
- name: http-echo
Expand Down Expand Up @@ -388,6 +407,7 @@ spec:
labels:
app: {{ $.name }}
spec:
terminationGracePeriodSeconds: 0
affinity:
podAntiAffinity:
requiredDuringSchedulingIgnoredDuringExecution:
Expand Down Expand Up @@ -509,6 +529,9 @@ spec:
- /var/lib/fortio/
{{- else }}
image: {{ $.Values.fortioImage }}
volumeMounts:
- name: shared-data
mountPath: /var/lib/fortio
args:
- report
- -http-port
Expand Down
3 changes: 3 additions & 0 deletions perf/benchmark/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ server: # server overrides
#tlsmode: DISABLE
tlsmode: ISTIO_MUTUAL
expose: false
waypoint: false
ambient: false
inject: "false"
injectL: "disabled" # "enabled" or "disabled"
replica: 1 # default value
Expand All @@ -39,6 +41,7 @@ client: # client overrides
#tlsmode: DISABLE
tlsmode: ISTIO_MUTUAL
expose: true
ambient: false
inject: "false"
injectL: "disabled" # "enabled" or "disabled"
replica: 1 # default value
Expand Down
2 changes: 1 addition & 1 deletion perf/istio-install/base/templates/prometheus-operator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,4 +175,4 @@ spec:
selector:
app.kubernetes.io/component: controller
app.kubernetes.io/name: prometheus-operator
{{- end }}
{{- end }}
4 changes: 4 additions & 0 deletions perf/istio-install/istioctl_profiles/ambient.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
apiVersion: install.istio.io/v1alpha1
kind: IstioOperator
spec:
profile: ambient