-
Notifications
You must be signed in to change notification settings - Fork 23
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
NETOBSERV-1248 deduper using infinispan cache #639
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
||
func (c *Cache) getDedupKey(v config.GenericMap) string { | ||
return regex.ReplaceAllString( | ||
fmt.Sprintf("%d-%d-%s-%d-%s-%d", v["TimeFlowEndMs"], v["Proto"], v["SrcAddr"], v["SrcPort"], v["DstAddr"], v["DstPort"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put TimeFlowEndMs
on top of the 5 tuple to avoid conflicts when 2 Egress comes before first Ingress.
As is, I miss some flows as you can see in the screens. We need to find a better way to unique identify each flows.
Any thoughts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can do an assumption here, that the interfaces are not going to change over time across flows in the same 5-tuples. So we could remove the timestamp from the key. In fact I'm already doing this same assumption here
On top of that, it should be much better for the memory footprint.
The downside is that it works for merging interfaces, but might not work if we want to reuse the cache for other purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this cache is only for deduper so I'm fine with that;
However are we sure that the interfaces are not changing when the pod is recreated for example ?
The ip addresses can be the same but ovn generated interfaces could change 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on top of that, I could push only fields we want to merge in the cache instead of the whole flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However are we sure that the interfaces are not changing when the pod is recreated for example ?
The ip addresses can be the same but ovn generated interfaces could change
A pod restart would very very very likely use a different IP as far as I know; and even if that's not the case, the connection would need to be reinitiated so the source port would change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but perhaps in any case there should be an expiry mechanism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the cache already expire when full 😸 you can also set a lifespan
https://infinispan.org/docs/stable/titles/configuring/configuring.html#expiration_configuring-memory-usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made changes in 335d737
Endpoint: "http://cache.netobserv.svc.cluster.local:11222", | ||
/*Credentials: &Credentials{ | ||
Username: "netobserv", | ||
Password: "netobserv", | ||
},*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to make these configurable and add TLS from secret
log.Debugf("merging cached flow %v with %v ...", srcFlow, outputEntry) | ||
if len(srcFlow["Interfaces"].([]interface{})) == len(srcFlow["IfDirections"].([]interface{})) { | ||
for i := 0; i < len(srcFlow["Interfaces"].([]interface{})); i++ { | ||
outputEntry["Interfaces"] = append(outputEntry["Interfaces"].([]string), fmt.Sprintf("%s*", srcFlow["Interfaces"].([]interface{})[i].(string))) | ||
outputEntry["IfDirections"] = append(outputEntry["IfDirections"].([]int), int(srcFlow["IfDirections"].([]interface{})[i].(float64))) | ||
} | ||
} | ||
log.Debugf("merged flow: %v", outputEntry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👆 merging Egress
flow content in Ingress
one.
For now just merging Interfaces
and IfDirections
and adding a *
next to the names.
We could imagine merging more feature related contents, TCP flags and also check Bytes / Packets amounts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if Egress comes after Ingress, it will be ignored? Perhaps it's another reason to not have the timestamp in the key as discussed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah currently if Egress
comes after Ingress
, it's pushed in the cache and never retreived.
Only Ingress
will appear in storage and will not have merged fields.
An improvment could be to send first flow to the cache whatever direction is found and merge on the "second" one; but that will double the cache queries
) | ||
|
||
var log = logrus.WithField("component", "transform.Network.Cache") | ||
var regex = regexp.MustCompile(`[^a-zA-Z0-9 ]+`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there forbidden characters in key that require this replaceAll ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had issues with my initial attempt either with .
or :
when I had standart ips + ports key
I can give another try to get rid of this regex and improve perfs indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also addressed in 335d737
reinterpretDirection(outputEntry, &n.DirectionInfo) | ||
isInner, isSrcReporter := reinterpretDirection(outputEntry, &n.DirectionInfo) | ||
if n.cache != nil { | ||
return n.cache.DedupFlows(isInner, isSrcReporter, outputEntry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was weirdly seeing no flow under layer=app filter and now I understand : this return
breaks the rules loop processing :-)
It should be continue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case push flow to cache so you don't need to manage extra transform rules on it right ?
The goal is actually to break the pipeline here.
When the second flow will hit this stage, the first flow is retreived from cache and other transform rules are applied such as layer
The new version is better but still increases the overall resource usage in my tests, although it's decrease for Loki & storage: (By overall I mean: everything deployed in netobserv* namespaces - so that includes the infinispan deployment and loki) CPU overall +146% (1.68) Loki CPU -44% (0.035) So the benefit here is really on storage, and comes at a price in CPU and memory. We could run tests at a bigger scale to check if it's different. Also one thing that seems wrong from my tests is that the captured traffic has dropped quite a lot, as if we were removing more flows than expected. That's to investigate. |
335d737
to
1ccd91c
Compare
Rebased this PR and added |
PR needs rebase. 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. |
@jotak is this something we should still consider ? I can replace the json |
Description
Implements infinispan shared cache:
Keeps
Egress
flows between two nodes in cacheThen merge
interfaces
andifdirections
when Ingress is capturedMerged flows contains a list of interfaces / ifdirections with a
![Screenshot from 2024-03-18 20-19-43](https://private-user-images.githubusercontent.com/91894519/313992533-4bc93d07-47a6-493f-be32-af311d1790e1.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ1ODU5NDMsIm5iZiI6MTcyNDU4NTY0MywicGF0aCI6Ii85MTg5NDUxOS8zMTM5OTI1MzMtNGJjOTNkMDctNDdhNi00OTNmLWJlMzItYWYzMTFkMTc5MGUxLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI1VDExMzQwM1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTIzMzZiNzAwYWQzYmEwOTgxOTg1OWY2YmRhMTYwNjNjZDhlMGU4ZmFlZDdjMTg1MmQxY2Y0YTc0ZDdiNWM4NzgmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0._QxG7PM2n5ZnJbMzBWIukmzUOgOPkGNf4I960enONyM)
![Screenshot from 2024-03-18 20-19-55](https://private-user-images.githubusercontent.com/91894519/313992588-af3d590d-0fb1-4f2e-8d8e-254c25258b27.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ1ODU5NDMsIm5iZiI6MTcyNDU4NTY0MywicGF0aCI6Ii85MTg5NDUxOS8zMTM5OTI1ODgtYWYzZDU5MGQtMGZiMS00ZjJlLThkOGUtMjU0YzI1MjU4YjI3LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI1VDExMzQwM1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWVmYTg1OGJjODEwMWQxZGE5NjRmMzZjODZlMDAwYmU3ZmNhODk0YmQ5ZDA2NTY3Y2RkZDIxNTVjOTNmYWI0ODUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.J3Vey-40BxiW-LEyCbFHa-WHpSqo2p1A-56HvkqIi3s)
*
next to names coming from Egress flow:Steps to use this code:
go mod tidy && go mod vendor
Infinispan Operator
ORRed Hat Data Grid Operator
cache
cluster in netobserv namespace with disabled TLS / Authdeduper
cacheThese will provide a service
cache.netobserv.svc.cluster.local
on port11222
.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.