[Flow Visibility] Add policy recommendation Spark job #3064

dreamtalen · 2021-11-30T00:52:59Z

In this PR, we are adding the initial policy recommendation Spark job for flow visility long term architecture.

codecov-commenter · 2021-11-30T02:32:55Z

Codecov Report

Merging #3064 (00ca02d) into main (c135609) will increase coverage by 1.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3064      +/-   ##
==========================================
+ Coverage   58.76%   59.88%   +1.11%     
==========================================
  Files         292      334      +42     
  Lines       24724    28639    +3915     
==========================================
+ Hits        14530    17151    +2621     
- Misses       8622     9620     +998     
- Partials     1572     1868     +296

Flag	Coverage Δ
kind-e2e-tests	`47.84% <ø> (+2.46%)`	⬆️
unit-tests	`41.77% <ø> (+1.62%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/cniserver/pod_configuration_linux.go	`66.66% <0.00%> (-33.34%)`	⬇️
pkg/util/k8s/name.go	`6.25% <0.00%> (-27.09%)`	⬇️
pkg/ipam/poolallocator/allocator.go	`50.00% <0.00%> (-12.42%)`	⬇️
pkg/controller/egress/validate.go	`64.44% <0.00%> (-9.37%)`	⬇️
pkg/controller/networkpolicy/status_controller.go	`69.94% <0.00%> (-7.19%)`	⬇️
pkg/agent/nodeportlocal/k8s/annotations.go	`93.54% <0.00%> (-6.46%)`	⬇️
pkg/agent/nodeportlocal/npl_agent_init.go	`84.61% <0.00%> (-6.30%)`	⬇️
pkg/ovs/openflow/ofctrl_packetout.go	`79.60% <0.00%> (-6.05%)`	⬇️
pkg/ovs/ovsconfig/ovs_client.go	`43.96% <0.00%> (-5.20%)`	⬇️
pkg/agent/interfacestore/interface_cache.go	`77.27% <0.00%> (-4.10%)`	⬇️
... and 108 more

antoninbas

was it decided that this will live in the main antrea repo?

dreamtalen · 2021-12-01T00:12:02Z

was it decided that this will live in the main antrea repo?

Hi Antonin, we decided not to open a separate repo for flow visibility long term arch.
Currently we are going to put all related files under a new folder build/yamls/flow-visibility/, what we haven't decided is whether we need to create a separate branch for this long term work.
One concern of having a separate branch is that last year when we were developing flow aggregator feature and merge the branch to main branch, github only records Srikar's contribution and all other co-authors' contribution is ignored because all commits are squashed into one commit. May I ask your suggestion for our code check-in workflow? Thanks!

antoninbas · 2021-12-01T18:34:29Z

@dreamtalen

May I ask your suggestion for our code check-in workflow?

Note that one commit can have multiple co-authors, but the commit message needs to be written carefully (https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors). However, I am not sure we need a feature branch in this case, as long as the changes are incremental and independent, they can be merged into main separately.

Currently we are going to put all related files under a new folder build/yamls/flow-visibility/

I don't think this is the right place for this (a Python script for policy recommendation definitely doesn't belong under build/yamls/). Maybe consider plugins/ for now.

dreamtalen · 2021-12-01T22:05:37Z

@dreamtalen

May I ask your suggestion for our code check-in workflow?

Note that one commit can have multiple co-authors, but the commit message needs to be written carefully (https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors). However, I am not sure we need a feature branch in this case, as long as the changes are incremental and independent, they can be merged into main separately.

Currently we are going to put all related files under a new folder build/yamls/flow-visibility/

I don't think this is the right place for this (a Python script for policy recommendation definitely doesn't belong under build/yamls/). Maybe consider plugins/ for now.

Thanks Antonin, after another round discussion with team, we think our flow visibility long term arch has three main parts: Clickhouse database deployment, Grafana UI implementation and Python Spark jobs. There will be some dependencies between PRs inside each part. So we are thinking about creating 3 branches for each part which could have better integration/e2e tests of each part before merging into main branch.
Does this approach look better than merging each individual PR into main branch directly?

antoninbas · 2021-12-02T00:37:32Z

Does this approach look better than merging each individual PR into main branch directly?

Given that this doesn't affect main Antrea functions, and I don't expect that we will deprecate current flow visibility stuff until we make good progress with the new version, either option is fine with me. Note that managing multiple feature branches can be a bit of a pain, so if possible I would still recommend merging into main directly. By "if possible" I mean that each PR that is merged should of course not break any existing functionality or cause any CI job to fail.

dreamtalen · 2021-12-02T18:29:25Z

Does this approach look better than merging each individual PR into main branch directly?

Given that this doesn't affect main Antrea functions, and I don't expect that we will deprecate current flow visibility stuff until we make good progress with the new version, either option is fine with me. Note that managing multiple feature branches can be a bit of a pain, so if possible I would still recommend merging into main directly. By "if possible" I mean that each PR that is merged should of course not break any existing functionality or cause any CI job to fail.

Thanks Antonin, I agree with your opinion and had won the agreement of other team members. We will merge the long term PRs into main branch directly in the future.

zyiou · 2021-12-21T21:06:16Z

plugins/flow-visibility/policy-recommendation/policy_recommendation_job.py

+        return "pod_to_external"
+
+def map_flow_to_egress(flow):
+    src = "#".join([flow.sourcePodNamespace, flow.sourcePodLabels])


Is # a self-defined delimiter? If so, I think we'd better define it as a constant.

zyiou · 2021-12-21T21:12:59Z

plugins/flow-visibility/policy-recommendation/antrea_crd.py

+# This library is used to define Antrea Network Policy related CRDs in Python.
+# Code structure is following the Kubernetes Python Client library (https://github.com/kubernetes-client/python).
+# Could be improved by using openAPI generator in the future.


Is this file auto-generated or manually created? If there is a change in Antrea Network Policy related CRD, this file also needs to be changed?

Thanks for your review Yiou.
This file is manually created now, yes we need to change this file if there is a related change on Antrea side affecting the policy recommendation. This file could be changed to auto-generated by using openAPI generator in the future like the K8s python lib.

zyiou · 2021-12-21T21:15:12Z

plugins/flow-visibility/policy-recommendation/policy_recommendation_job.py

+def get_IP_version(IP):
+    return "v4" if type(ip_address(IP)) is IPv4Address else "v6"
+
+def camel(s):
+    s = sub(r"(_|-)+", " ", s).title().replace(" ", "")
+    if not s: return s
+    return s[0].lower() + s[1:]


I feel many of these utility functions can be moved into a separate file for maintenance.

zyiou · 2021-12-21T21:24:16Z

plugins/flow-visibility/policy-recommendation/policy_recommendation_job.py

+        .option("dbtable", table_name) \
+        .save()
+
+def initial_recommendation_job(spark, db_jdbc_address, table_name, limit=100, option=1, start_time=None, end_time=None, ns_allow_list=NAMESPACE_ALLOW_LIST):


It would be better to describe the main functions (initial_recommendation_job and subsequent_recommendation_job) with some comments. Same for fields like option (what are the values 1,2,3 for).

heanlan · 2022-01-03T22:19:19Z

plugins/flow-visibility/policy-recommendation/policy_recommendation_job.py

+    return (dst, (src, ""))
+
+def combine_network_peers(a, b):
+    if a[0] != "" and b[0] != "":


Could you please explain a little bit more on why the case a[0] != "" and b[0] != "" is possible?
I assume in the ingress_rdd, for each key - dst, there will only be one element (key, val) - (dst, (src0|src1|src2|..., "")), after performing the reduceByKey in L413.

Thanks Anlan, I agree this if condition is redundant, will also remove L66.

yanjunz97 · 2022-01-11T19:46:03Z

plugins/flow-visibility/policy-recommendation/policy_recommendation_job.py

+    elif destinationPodLabels != "":
+        return "pod_to_pod"


Do we expect to always store podLabels in the database even if the optional podLabels are set as false?

Currently yes becasue we could not create policies without pod lables.
We may need additional service inside k8s cluster to fetch Pod labels and store in dababase, if there is no podLabels fields when flow aggregator exported.

Signed-off-by: Yongming Ding <[email protected]>

dreamtalen force-pushed the init-recommendation branch from 4cfd261 to 0356e66 Compare November 30, 2021 00:54

antoninbas reviewed Nov 30, 2021

View reviewed changes

[Flow Visibility] Add initial policy recommendation Spark job

5458b30

dreamtalen force-pushed the init-recommendation branch from 0356e66 to 5458b30 Compare December 2, 2021 22:34

dreamtalen changed the title ~~[Flow Visibility] Add initial policy recommendation Spark job~~ [Flow Visibility] Add policy recommendation Spark job Dec 7, 2021

dreamtalen marked this pull request as draft December 7, 2021 19:07

dreamtalen force-pushed the init-recommendation branch from ee39314 to 5408048 Compare December 7, 2021 22:26

[Flow Visibility] Add subsequent policy recommendation Spark job

7da486a

dreamtalen force-pushed the init-recommendation branch from 5408048 to 7da486a Compare December 8, 2021 23:04

dreamtalen force-pushed the init-recommendation branch 2 times, most recently from efb0a2a to 96a5f1a Compare December 16, 2021 21:34

dreamtalen marked this pull request as ready for review December 16, 2021 23:16

dreamtalen requested review from heanlan, yanjunz97 and zyiou December 16, 2021 23:17

Update Spark job with clickhouse schema

d4fb2d2

dreamtalen force-pushed the init-recommendation branch from 96a5f1a to d4fb2d2 Compare December 17, 2021 01:25

zyiou reviewed Dec 21, 2021

View reviewed changes

heanlan reviewed Jan 3, 2022

View reviewed changes

Yongming Ding added 2 commits January 3, 2022 16:49

Address comments from yiou and anlan.

70a9bcc

Add unit tests for policy recommendation jobs

9bd91e8

dreamtalen force-pushed the init-recommendation branch from 66979d1 to 9bd91e8 Compare January 8, 2022 01:40

yanjunz97 reviewed Jan 11, 2022

View reviewed changes

heanlan mentioned this pull request Jan 26, 2022

[Flow Visibility]Add Grafana dashboards #3191

Closed

Made corresponding changes for Spark deployment in k8s cluster.

df0e7a7

Signed-off-by: Yongming Ding <[email protected]>

dreamtalen mentioned this pull request Feb 4, 2022

[Flow Visibility] Add depolyment files of policy recommendation Spark job in k8s cluster #3280

Closed

Add copyright header

00ca02d

Signed-off-by: Yongming Ding <[email protected]>

dreamtalen force-pushed the init-recommendation branch from 11447de to 00ca02d Compare February 4, 2022 23:49

dreamtalen closed this Feb 24, 2022

dreamtalen deleted the init-recommendation branch February 24, 2022 08:18

dreamtalen mentioned this pull request May 5, 2022

Add Policy Recommendation Spark job and image antrea-io/theia#16

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flow Visibility] Add policy recommendation Spark job #3064

[Flow Visibility] Add policy recommendation Spark job #3064

dreamtalen commented Nov 30, 2021

codecov-commenter commented Nov 30, 2021 •

edited

Loading

antoninbas left a comment

dreamtalen commented Dec 1, 2021

antoninbas commented Dec 1, 2021

dreamtalen commented Dec 1, 2021

antoninbas commented Dec 2, 2021

dreamtalen commented Dec 2, 2021

zyiou Dec 21, 2021

zyiou Dec 21, 2021

dreamtalen Jan 3, 2022

zyiou Dec 21, 2021

zyiou Dec 21, 2021

heanlan Jan 3, 2022

dreamtalen Jan 3, 2022

yanjunz97 Jan 11, 2022

dreamtalen Jan 11, 2022

[Flow Visibility] Add policy recommendation Spark job #3064

[Flow Visibility] Add policy recommendation Spark job #3064

Conversation

dreamtalen commented Nov 30, 2021

codecov-commenter commented Nov 30, 2021 • edited Loading

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

dreamtalen commented Dec 1, 2021

antoninbas commented Dec 1, 2021

dreamtalen commented Dec 1, 2021

antoninbas commented Dec 2, 2021

dreamtalen commented Dec 2, 2021

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

codecov-commenter commented Nov 30, 2021 •

edited

Loading