Skip to content
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

vpp: pass hashconfig to vpp for service annotations #635

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

hedibouattour
Copy link
Collaborator

This patch uses the newly introduced field in cnat translation vpp api to pass ecmp hashconfig defined by annotations in services.

@hedibouattour hedibouattour force-pushed the pass-ECMP-hashConfig-to-VPP-from-annotations branch 3 times, most recently from 5c38b8d to 7dae114 Compare September 15, 2023 09:22
Copy link
Collaborator

@sknat sknat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks nice, just a few comments

calico-vpp-agent/services/service_handler.go Outdated Show resolved Hide resolved
Comment on lines 36 to 45
var hashConfigMap = map[hashConfig]types.IPFlowHash{
hashConfigDstaddr: types.FlowHashDstIP,
hashConfigDstport: types.FlowHashDstPort,
hashConfigIproto: types.FlowHashProto,
hashConfigReverse: types.FlowHashReverse,
hashConfigSrcaddr: types.FlowHashSrcIP,
hashConfigSrcport: types.FlowHashSrcPort,
hashConfigSymmetric: types.FlowHashSymetric,
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing an extra layer of mangling here, we could directly build the bitmap in ParseServiceAnnotations() as we already are doing a switch statement there

@@ -34,6 +35,18 @@ const (
MaglevLB = CnatLbType(cnat.CNAT_LB_TYPE_MAGLEV)
)

type CnatHashConfig uint8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth adding a String() function on the CnatHashConfig type

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no need for that as we already have [0] and we can cast to it when logging.

func (x IPFlowHashConfig) String() string {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, there is this, but the output will probably be really ugly...
e.g. usually "IP_API_FLOW_HASH_SRC_IP|IP_API_FLOW_HASH_DST_IP|IP_API_FLOW_HASH_SRC_PORT|IP_API_FLOW_HASH_DST_PORT|IP_API_FLOW_HASH_PROTO" ...

@hedibouattour hedibouattour force-pushed the pass-ECMP-hashConfig-to-VPP-from-annotations branch 2 times, most recently from 276bcd0 to e3aee78 Compare October 4, 2023 16:12
@hedibouattour hedibouattour force-pushed the pass-ECMP-hashConfig-to-VPP-from-annotations branch from e3aee78 to b5d0f00 Compare October 9, 2023 07:51
@sknat sknat merged commit e9b39af into master Oct 9, 2023
3 checks passed
@sknat sknat deleted the pass-ECMP-hashConfig-to-VPP-from-annotations branch October 9, 2023 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants