Combine FlowExporter resources initialization in agent.go

[heanlan]

1. Include the initializations of priority queue and connection store into flowExporter
2. Use setter to to inject the denyConnStore into the networkPolicyController

Fixed: #2829

Signed-off-by: heanlan hanlan@vmware.com

[codecov-commenter]

Codecov Report

Merging #2854 (80cad1b) into main (fb7ed5c) will increase coverage by 21.04%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main    #2854       +/-   ##
===========================================
+ Coverage   40.61%   61.65%   +21.04%     
===========================================
  Files         158      283      +125     
  Lines       19906    23830     +3924     
===========================================
+ Hits         8084    14693     +6609     
+ Misses      11050     7567     -3483     
- Partials      772     1570      +798     

Flag	Coverage Δ
kind-e2e-tests	49.24% <100.00%> (?)
unit-tests	40.53% <25.00%> (-0.08%)	⬇️

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

Impacted Files	Coverage Δ
...ntroller/networkpolicy/networkpolicy_controller.go	71.25% <100.00%> (+16.81%)	⬆️
pkg/agent/flowexporter/connections/connections.go	77.27% <100.00%> (+33.33%)	⬆️
.../flowexporter/connections/conntrack_connections.go	83.21% <100.00%> (+24.38%)	⬆️
...agent/flowexporter/connections/deny_connections.go	86.15% <100.00%> (+35.38%)	⬆️
pkg/agent/flowexporter/exporter/exporter.go	81.46% <100.00%> (+28.68%)	⬆️
pkg/agent/controller/egress/egress_controller.go	38.32% <0.00%> (-1.47%)	⬇️
pkg/apiserver/handlers/webhook/convert_crd.go	2.56% <0.00%> (ø)
pkg/apiserver/handlers/webhook/mutation_labels.go	24.71% <0.00%> (ø)
...agent/controller/egress/local_ip_detector_linux.go	0.00% <0.00%> (ø)
pkg/agent/util/sysctl/sysctl_linux.go	0.00% <0.00%> (ø)
... and 239 more

[srikartati]

LGTM.
I think current function calls are similar to calls with other packages in agent.go.

[srikartati]

LGTM

[antoninbas]

LGTM
I was hoping for more changes. Why don't we have a unique initialization function for the connection store and the flow exporter?

[heanlan]

LGTM I was hoping for more changes. Why don't we have a unique initialization function for the connection store and the flow exporter?

Thanks Antonin. denyConnStore cannot be created at the same place with conntrackConnStore and flowExporter. As denyConnStore is required by networkPolicyController, and networkPolicyController is required by flowExporter.
I created a unique function for only conntrackConnStore and flowExporter in the latest commit. If that's not desirable, probably we can still go back with the previous version.

[antoninbas]

@heanlan No I don't think we want that last commit, we should keep things symmetric
However, I do think we could consider using a setter to do the dependency injection; we don't have to pass everything though the constructor. For example, maybe it would make sense to inject the denyConnStore into the networkPolicyController using a setter. This way you do not need to create the denyConnStore before the networkPolicyController. I'll let @srikartati comment on this as well.

[srikartati]

@heanlan No I don't think we want that last commit, we should keep things symmetric However, I do think we could consider using a setter to do the dependency injection; we don't have to pass everything though the constructor. For example, maybe it would make sense to inject the denyConnStore into the networkPolicyController using a setter. This way you do not need to create the denyConnStore before the networkPolicyController. I'll let @srikartati comment on this as well.

Thanks @antoninbas. Looked into the code more closely. Having a setter in the network policy controller is definitely doable. However, it is better to break the initialization of connection stores and the exporter, and their Run functions. Add them separately like other packages. In this way, we can make sure denyConnStore is not nil when the network policy controller in the agent starts running.

[antoninbas]

this way, we can make sure denyConnStore is not nil when the network policy controller in the agent starts running.

All goroutines are created at the end of the initialization function

[srikartati]

All goroutines are created at the end of the initialization function

Yes, Flow Exporter pkg structures are being initialized along with the creation of goroutines right now. I just meant that we can modify and follow the similar style as other packages. Then we will have two groups: initialization of structures along with the setter and goroutines creation.

[heanlan]

Thanks Antonin and Srikar. In the latest commit, I have

1. used setter for setting denyConnStore of networkPolicyController
2. separate connection stores & flowExporter initialization and go routines creation

To make the separation more consistent, shall I move the initialization of NPLAgent - line 362, agentQuerier, apiServer, packetInReasons - line 405-444 above the creation of all go routines?

[srikartati]

To make the separation more consistent, shall I move the initialization of NPLAgent - line 362, agentQuerier, apiServer, packetInReasons - line 405-444 above the creation of all go routines?

I think it is a small change and ok to do in this PR--you may have to change the title and description. Another option is doing it in a separate PR so that it can be reviewed by others. I am ok with either way and do not have a strong opinion.

[heanlan]

I think it is a small change and ok to do in this PR--you may have to change the title and description. Another option is doing it in a separate PR so that it can be reviewed by others. I am ok with either way and do not have a strong opinion.

Thanks Srikar. I tried to do the changes in the latest commit. It seems involved in a lot of windows CI failures... I've already been blocked by the go import error for hours https://github.com/antrea-io/antrea/runs/3819830137?check_suite_focus=true
,haven't got chance to look at other failures. I'm thinking I'd probably keep those part unchanged for this PR...

[heanlan]

Could you please take another look on this PR @antoninbas @srikartati ? Thanks a lot!

[srikartati]

LGTM

[heanlan]

squashed the commits

[heanlan]

/test-all

[antoninbas]

one nit, otherwise LGTM

[tnqn]

@heanlan @antoninbas @srikartati do we plan to include it in v1.4?

[heanlan]

I have resolved the conflicts. It's not urgent to be included in 1.4. But if there's no more comments, I think it's ready to be merged.

[heanlan]

/test-all

[heanlan]

the test failures will be fixed by #2968

[antoninbas]

@heanlan should you rebase and run the tests again, now that #2968 has been merged?

[heanlan]

/test-all
/test-integration