NETOBSERV-1564: do not force flushing maps when rb is triggered #348

jotak · 2024-06-14T15:20:29Z

Description

Flushing (without throttling) has a harmful effect in high stressed scenario, generating a lot of evictions from maps, resulting in many more flows generated.

High stressed scenarios should rather rely on rb+accounter, which better handles the number of generated flows, than trying to force using maps this way

Also, use errno as the reason for the metric

With this change + high stress scenario I'm seeing better CPU but more memory slightly increased:

Patch applied at 16:55

overall, -50% CPU and +10% memory

This should be tested against cluster-density-v2

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

openshift-ci-robot · 2024-06-14T15:20:32Z

@jotak: This pull request references NETOBSERV-1564 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.17.0" version, but no target version was set.

In response to this:

Description

Flushing (without throttling) has a nefast effect in high stressed scenario, generating a lot of evictions from maps, resulting in many more flows generated.

High stressed scenarios should rather rely on rb+accounter, which better handles the number of generated flows.

Also, use errno as the reason for the metric

With this change + high stress scenario I'm seeing better CPU but more memory slightly increased:

overall, -50% CPU and +10% memory

This should be tested against cluster-density-v2

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

Will this change affect NetObserv / Network Observability operator? If not, you can ignore the rest of this checklist.

Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).

Does this PR require product documentation?

If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.

Does this PR require a product release notes entry?

If so, fill in "Release Note Text" in the JIRA.

Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.

If so, make sure it is described in the JIRA ticket.

QE requirements (check 1 from the list):

Standard QE validation, with pre-merge tests unless stated otherwise.

Regression tests only (e.g. refactoring with no user-facing change).

No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-06-14T15:22:05Z

@jotak: This pull request references NETOBSERV-1564 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.17.0" version, but no target version was set.

In response to this:

Description

Flushing (without throttling) has a nefast effect in high stressed scenario, generating a lot of evictions from maps, resulting in many more flows generated.

High stressed scenarios should rather rely on rb+accounter, which better handles the number of generated flows.

Also, use errno as the reason for the metric

With this change + high stress scenario I'm seeing better CPU but more memory slightly increased:

Patch applied at 16:55

overall, -50% CPU and +10% memory

This should be tested against cluster-density-v2

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

Will this change affect NetObserv / Network Observability operator? If not, you can ignore the rest of this checklist.

Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).

Does this PR require product documentation?

If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.

Does this PR require a product release notes entry?

If so, fill in "Release Note Text" in the JIRA.

Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.

If so, make sure it is described in the JIRA ticket.

QE requirements (check 1 from the list):

Standard QE validation, with pre-merge tests unless stated otherwise.

Regression tests only (e.g. refactoring with no user-facing change).

No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-06-14T15:35:22Z

@jotak: This pull request references NETOBSERV-1564 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

Description

Flushing (without throttling) has a harmful effect in high stressed scenario, generating a lot of evictions from maps, resulting in many more flows generated.

High stressed scenarios should rather rely on rb+accounter, which better handles the number of generated flows.

Also, use errno as the reason for the metric

With this change + high stress scenario I'm seeing better CPU but more memory slightly increased:

Patch applied at 16:55

overall, -50% CPU and +10% memory

This should be tested against cluster-density-v2

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

Will this change affect NetObserv / Network Observability operator? If not, you can ignore the rest of this checklist.

Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).

Does this PR require product documentation?

If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.

Does this PR require a product release notes entry?

If so, fill in "Release Note Text" in the JIRA.

Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.

If so, make sure it is described in the JIRA ticket.

QE requirements (check 1 from the list):

Standard QE validation, with pre-merge tests unless stated otherwise.

Regression tests only (e.g. refactoring with no user-facing change).

No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-06-14T15:36:05Z

@jotak: This pull request references NETOBSERV-1564 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

Description

Flushing (without throttling) has a harmful effect in high stressed scenario, generating a lot of evictions from maps, resulting in many more flows generated.

High stressed scenarios should rather rely on rb+accounter, which better handles the number of generated flows, than trying to force using maps this way

Also, use errno as the reason for the metric

With this change + high stress scenario I'm seeing better CPU but more memory slightly increased:

Patch applied at 16:55

overall, -50% CPU and +10% memory

This should be tested against cluster-density-v2

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

Will this change affect NetObserv / Network Observability operator? If not, you can ignore the rest of this checklist.

Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).

Does this PR require product documentation?

If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.

Does this PR require a product release notes entry?

If so, fill in "Release Note Text" in the JIRA.

Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.

If so, make sure it is described in the JIRA ticket.

QE requirements (check 1 from the list):

Standard QE validation, with pre-merge tests unless stated otherwise.

Regression tests only (e.g. refactoring with no user-facing change).

No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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 openshift-eng/jira-lifecycle-plugin repository.

Flushing (without throttling) has a nefast effect in high stressed scenario, generating a lot of evictions from maps, resulting in many more flows generated. High stressed scenarios should rather rely on rb+accounter, which better handles the number of generated flows. Also, use errno as the reason for the metric

github-actions · 2024-06-17T10:34:13Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:81e0239

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=81e0239 make set-agent-image

codecov · 2024-06-17T10:38:26Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@88136bd). Learn more about missing BASE report.
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #348   +/-   ##
=======================================
  Coverage        ?   33.33%           
=======================================
  Files           ?       48           
  Lines           ?     3489           
  Branches        ?        0           
=======================================
  Hits            ?     1163           
  Misses          ?     2229           
  Partials        ?       97

Flag	Coverage Δ
unittests	`33.33% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
pkg/flow/tracer_ringbuf.go	`25.00% <0.00%> (ø)`

msherif1234 · 2024-06-24T11:37:04Z

pkg/flow/tracer_ringbuf.go

@@ -91,15 +91,9 @@ func (m *RingBufTracer) listenAndForwardRingBuffer(debugging bool, forwardCh cha
 	if debugging {
 		m.stats.logRingBufferFlows(mapFullError)
 	}
-	// if the flow was received due to lack of space in the eBPF map


wouldn't removing this logic means we will stay in hmap full condition now for longer and possibly dropping flows ? if the concern here is doing many map flush because system is under stress can we trigger a flush every 1s or so hopping to free up some hmap space ?

I agree that having a fixed-rate flush could be a better solution, but actually there's already a flush every 5s (the cacheActiveTimeout setting) ; users can set it to 1s if that works better for them

or we introduce a new "stress timer" (could be automatically set to cacheActiveTimeout / something) that only starts when stress is detected (ie. maps are full) ? but that's a bit complex, it has to be worth it

by the way: when you said "this logic means we will stay in hmap full condition now for longer and possibly dropping flows" I don't think this is true; when maps are full, flows are not dropped, they're moved to the ring buffer. Dropped flows are only for busy maps when there isn't a new flow creation, so not something that can lead the map being full (we're updating an entry in that case, not adding a new one, so no incidence on hmap size)

we use rb for new flows and it send 1 pkt at a time to userspace so it can't handle burst of traffic either

it still handle that much better than an uncontrolled hot-loop of flush events. Look at the -50% CPU that I'm showing in the PR description

I'll run perf-scale tests to check the diff

/LGTM if perf scale shows improvements

ingress-perf shows slightly improved stats: -4% memory, -8% CPU
(https://docs.google.com/spreadsheets/d/1EN12dogz-0_H_5tSoV24T4iHa9a3wSgoF396YyOqavA/edit?gid=93930639#gid=93930639 / diff: https://docs.google.com/spreadsheets/d/1q_XCJ48h2Q78JapxgNB37nJe-4lTYVlMx7xVrn7ztck/edit?gid=696044201#gid=696044201)

Now running cluster-density. But I'm not sure to see something very different here, as the RB isn't involved a lot anyway.

Problem is that, none of those test really stress the agents. So I may not see something better than this -8% here. The tests that I did above DO stress the agents much more, and I had -50%

@msherif1234 , @Amoghrd did run cluster-density, with also slightly positive results: https://issues.redhat.com/browse/NETOBSERV-1564?focusedId=24995474&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-24995474

Amoghrd · 2024-06-25T17:40:23Z

/label qe-approved

openshift-ci-robot · 2024-06-25T17:40:28Z

@jotak: This pull request references NETOBSERV-1564 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

Description

Flushing (without throttling) has a harmful effect in high stressed scenario, generating a lot of evictions from maps, resulting in many more flows generated.

High stressed scenarios should rather rely on rb+accounter, which better handles the number of generated flows, than trying to force using maps this way

Also, use errno as the reason for the metric

With this change + high stress scenario I'm seeing better CPU but more memory slightly increased:

Patch applied at 16:55

overall, -50% CPU and +10% memory

This should be tested against cluster-density-v2

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

Will this change affect NetObserv / Network Observability operator? If not, you can ignore the rest of this checklist.

Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).

Does this PR require product documentation?

If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.

Does this PR require a product release notes entry?

If so, fill in "Release Note Text" in the JIRA.

Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.

If so, make sure it is described in the JIRA ticket.

QE requirements (check 1 from the list):

Standard QE validation, with pre-merge tests unless stated otherwise.

Regression tests only (e.g. refactoring with no user-facing change).

No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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 openshift-eng/jira-lifecycle-plugin repository.

msherif1234 · 2024-06-26T12:25:51Z

/lgtm

jotak · 2024-06-26T12:37:45Z

thanks @msherif1234
/approve

openshift-ci · 2024-06-26T12:37:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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~~ [jotak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the jira/valid-reference label Jun 14, 2024

jotak force-pushed the rb-no-flush branch from a5723dd to 3e4eb71 Compare June 17, 2024 10:30

jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 17, 2024

msherif1234 reviewed Jun 24, 2024

View reviewed changes

openshift-ci bot added the qe-approved QE has approved this pull request label Jun 25, 2024

jotak requested a review from msherif1234 June 26, 2024 06:25

openshift-ci bot assigned msherif1234 Jun 26, 2024

openshift-ci bot added the lgtm label Jun 26, 2024

openshift-ci bot added the approved label Jun 26, 2024

openshift-merge-bot bot merged commit fdebe3f into netobserv:main Jun 26, 2024
13 checks passed

NETOBSERV-1564: do not force flushing maps when rb is triggered #348

NETOBSERV-1564: do not force flushing maps when rb is triggered #348

Conversation

jotak commented Jun 14, 2024 • edited Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Jun 14, 2024 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Jun 14, 2024 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Jun 14, 2024 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Jun 14, 2024 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

github-actions bot commented Jun 17, 2024

codecov bot commented Jun 17, 2024 • edited Loading

Codecov Report

msherif1234 Jun 24, 2024 • edited Loading

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

Amoghrd commented Jun 25, 2024

openshift-ci-robot commented Jun 25, 2024 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

msherif1234 commented Jun 26, 2024

jotak commented Jun 26, 2024

openshift-ci bot commented Jun 26, 2024

jotak commented Jun 14, 2024 •

edited

Loading

openshift-ci-robot commented Jun 14, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jun 14, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jun 14, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jun 14, 2024 •

edited by openshift-ci bot

Loading

codecov bot commented Jun 17, 2024 •

edited

Loading

msherif1234 Jun 24, 2024 •

edited

Loading

openshift-ci-robot commented Jun 25, 2024 •

edited by openshift-ci bot

Loading