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
[pipelined] Add conntrack pipelined controller #2191
Conversation
Signed-off-by: Nick Yurchenko <koolzz@fb.com>
Signed-off-by: Nick Yurchenko <koolzz@fb.com>
8ceca89
to
b9135b2
Compare
datapath, self.tbl_num, outbound_match, [], | ||
priority=flows.MINIMUM_PRIORITY, | ||
resubmit_table=self.next_table) | ||
|
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 dont see need to match on direction, given that action for both flows is same.
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 for some reason these are our default flows for a lot of controllers. I'm actually not sure why it was done this way initially. I've always been to lazy but I think I might finally refactor all controllers to use 1 default rule instead of 2.
actions = [parser.NXActionCT( | ||
flags=0x0, | ||
zone_src=None, | ||
zone_ofs_nbits=0, |
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.
Do you think it is work setting zone to avoid polluting root conntract table?
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 I didn't think it mattered but I can add that
priority=flows.DEFAULT_PRIORITY) | ||
|
||
inbound_match = MagmaMatch(eth_type=ether_types.ETH_TYPE_IP, | ||
direction=Direction.IN) |
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.
can you squash multiple patches in single commit, its tricky to review patch when first patch adds code and second removes it, let me know if there is way to look at final code on github.
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.
Not sure If i follow, when using github UI you can see the final code. https://github.com/magma/magma/pull/2191/files
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.
ok, I was looking at commits. I generally pull files on my laptop.
warnings.simplefilter('ignore') | ||
cls.service_manager = create_service_manager([], | ||
['ue_mac', 'conntrack']) | ||
cls._tbl_num = cls.service_manager.get_table_num( |
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.
why not add IPFIX APP and validate flow stats in IPFIX table.
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 default agw ovs doesn't have ipfix patches so it will need some modifications to work. thats why we don't have any tests for ipfix table
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.
lets fix it, Can you create task to build vagrant vm with the unified OVS build.
@pshelar Added zone picked the last number not sure if that matters, nuked double rule. lmk if otherwise it looks good |
actions = [parser.NXActionCT( | ||
flags=0x1, | ||
zone_src=None, | ||
zone_ofs_nbits=ofs_nbits(30, 31), |
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.
zone id is 16bit number, can you set lower bit.
Signed-off-by: Nick Yurchenko <koolzz@fb.com>
9d20f3e
to
f11388b
Compare
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.
Looks good to me.
looking into a unit test failure, don't merge for a bit |
Signed-off-by: Nick Yurchenko <koolzz@fb.com>
* 'master' of github.com:magma/magma: (31 commits) [AGW]: uplink_br0: configure using DHCP (magma#2218) Remove axios as a package dependency from magmalte. (magma#2246) [pipelined] Add conntrack pipelined controller (magma#2191) Bump dotenv from 6.2.0 to 8.2.0 in /nms/app (magma#2247) Bump nodemon from 1.19.4 to 2.0.4 in /nms/app (magma#2248) Bump @testing-library/react from 9.5.0 to 10.4.8 in /nms/app (magma#2249) [orc8r][cwf]LI swagger API update (magma#2170) [AGW] MME: add retry for mobility ip block read API. (magma#2233) Refactored CardTitleRow into single component (magma#2224) Add policy JSON editor (magma#2182) [lte][agw] Updating _get_enb_label_from_request to get IP from peername request (magma#2244) Make gateway ID a link in Gateway overview page (magma#2232) Fix metric label used for querying (magma#2241) Hide gateway status in JSON editor (magma#2223) Bump postcss-flexbugs-fixes from 3.3.1 to 4.2.1 in /nms/app (magma#2236) Bump regenerator-runtime from 0.13.5 to 0.13.7 in /nms/app (magma#2234) Bump @testing-library/react-hooks from 3.3.0 to 3.4.1 in /nms/app (magma#2235) Clean up Session Recycling Logic (magma#2149) Migrate CreateSessionRequest to use bundled fields (magma#2166) orc8r-values.tpl YAML template fix (magma#2228) ...
Signed-off-by: Nick Yurchenko koolzz@fb.com
Summary
Adds a new controller to match on conntrack flows, specifically the flow creation
Test Plan
Added unit test that sends a pcap and verifies snapshots match
This controller is not yet enabled on any gateway
Additional Information