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

docs: add initial performance guide doc #13297

Merged
merged 1 commit into from Sep 25, 2020
Merged

docs: add initial performance guide doc #13297

merged 1 commit into from Sep 25, 2020

Conversation

borkmann
Copy link
Member

No description provided.

@borkmann borkmann added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.8 labels Sep 25, 2020
@borkmann borkmann requested a review from a team as a code owner September 25, 2020 20:40
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.4 Sep 25, 2020
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I have a scattering of minor wording fixups below, but LGTM overall.

We should add "higher is better" / "lower is better" wording to every graph. The results may not be so apparent to people who are less familiar with reading these graphs.

Documentation/spelling_wordlist.txt Outdated Show resolved Hide resolved
Documentation/concepts/performance/index.rst Outdated Show resolved Hide resolved
Documentation/concepts/performance/index.rst Outdated Show resolved Hide resolved
Documentation/concepts/performance/index.rst Outdated Show resolved Hide resolved
Documentation/concepts/performance/index.rst Outdated Show resolved Hide resolved
$ ansible-playbook -e mode=tunneling -i packet-hosts.ini playbooks/install-k8s-cilium.yaml
$ ansible-playbook -e conf=vxlan -i packet-hosts.ini playbooks/run-kubenetbench.yaml

The first command configures Cilium to use tunneling (``-e mode=tunneling``),
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add any additional commands in between the above to ensure that the machines are restarted & ready? Maybe it should be split into two code blocks to make sure that anyone trying to reproduce will stop to verify this first?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will leave this one to potentially address by either @pchaigno or @kkourt .

Copy link
Member Author

Choose a reason for hiding this comment

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

@kkourt could you also follow-up to include the "higher is better" / "lower is better" wording into every plot?

Copy link
Member

Choose a reason for hiding this comment

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

Why would we need to restart the machines? If we need to, we can do it as part of the playbook.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant to add this feedback for line 216 which talks about clearing out old state.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. Maybe we could just check that BPF programs are unloaded as part of the second playbook (playbooks/run-rawnetperf.yaml). That should just be a matter of running bpftool net. I'm guessing that would be our main concern, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIU, the ansible play will wait until the machine is rebooted so state should be clean after that. There are two reasons for the reboot: bpf programs (for which we could call cilium cleanup and check bpf progs), but also changes on the MTU of the NICs.

Documentation/concepts/performance/index.rst Outdated Show resolved Hide resolved
Initial guide with results and some basic tuning options for users.

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann borkmann merged commit 758539b into master Sep 25, 2020
@borkmann borkmann deleted the pr/perf-doc branch September 25, 2020 22:14
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from master in 1.8.4 Sep 25, 2020
@kkourt
Copy link
Contributor

kkourt commented Sep 25, 2020

"higher is better" / "lower is better"

Good point! I'll add proper wording.

@borkmann
Copy link
Member Author

(I'll readd the 1.8 backport once also the follow-ups are merged early next week so we can take in all at once w/o partial state.)

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.4 Sep 30, 2020
@joestringer joestringer removed this from Needs backport from master in 1.8.4 Sep 30, 2020
@joestringer joestringer added this to Needs backport from master in 1.8.5 Sep 30, 2020
@tklauser tklauser moved this from Needs backport from master to Backport done to v1.8 in 1.8.5 Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.5
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

None yet

5 participants