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

[PBH] CLI for Policy Based Hashing #1701

Merged
merged 80 commits into from
Aug 25, 2021

Conversation

vadymhlushko-mlnx
Copy link
Contributor

@vadymhlushko-mlnx vadymhlushko-mlnx commented Jun 29, 2021

What I did

Created click CLI plugins for PBH feature

How I did it

The CLI plugins were auto-generated (by using the sonic-cli-gen) for show and config CLI groups, then manually those were manually edited to meet PBH the CLI requirements according to the PBH HLD

How to verify it

Added the UT

Which release branch to backport (provide reason below if selected)

  • 202106

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
…tions

Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Comment on lines +21 to +30
inner_v4_hash inner_ip_proto
inner_l4_dst_port
inner_l4_src_port
inner_dst_ipv4
inner_src_ipv4
inner_v6_hash inner_ip_proto
inner_l4_dst_port
inner_l4_src_port
inner_dst_ipv6
inner_src_ipv6
Copy link
Contributor

Choose a reason for hiding this comment

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

If we won't have too many Hash Field Items / Name can maybe consolidate this into one row / Name

Copy link
Collaborator

@nazariig nazariig Jul 15, 2021

Choose a reason for hiding this comment

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

@kktheballer the question is not clear: can you please share an example for such a use case?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant was that if we have too many hash field items and we make each item go into each row we may have to scroll down to view all the output.

If we will only have 4-8 hash field items per inner_v4/6_hash we could leave it as is

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kktheballer so far there will be only up to 7 hash fields. If we introduce more, than we can revisit the current CLI business logic. Are you ok with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure that sounds good @nazariig

vadymhlushko-mlnx and others added 8 commits July 15, 2021 05:18
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
…to parametrized

Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
@click.option(
"--ip-mask",
help="""Configures IPv4/IPv6 address mask for this hash field, required when the value of --hash-field are - INNER_DST_IPV4 or
INNER_SRC_IPV4 or INNER_SRC_IPV4 or INNER_SRC_IPV4 """,
Copy link
Collaborator

@nazariig nazariig Jul 22, 2021

Choose a reason for hiding this comment

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

@vadymhlushko-mlnx please fix the description

)
@click.option(
"--hash-field",
help="Configure native hash field for this hash field",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vadymhlushko-mlnx please fix the description according to YANG model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nazariig it is already according to the YANG model

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vadymhlushko-mlnx in YANG we have description "Configures native hash field for this hash field";

@click.option(
"--hash",
required=True,
help="Configures the hash to apply",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vadymhlushko-mlnx please align the description with update subgroup - use YANG model as reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nazariig fixed

)
@click.option(
"--ip-mask",
help="Configures IPv4/IPv6 address mask for this hash field",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vadymhlushko-mlnx please align the description with add subgroup. Use YANG model as a reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nazariig it is already according to the YANG model

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vadymhlushko-mlnx please rework as per internal review comments

)
@click.option(
"--priority",
help="Configures priority",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vadymhlushko-mlnx please align the description with YANG model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nazariig fixed

Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>
@liat-grozovik
Copy link
Collaborator

@kktheballer kindly reminder we wish to have the code merges so the test can be verified on top of it.

@anshuv-mfst
Copy link

hi @anish-n , @prsunny - could you please review and approve asap, this is needed in 202106 release

@anish-n
Copy link

anish-n commented Aug 6, 2021

@kktheballer could you please review and approve?

@vadymhlushko-mlnx
Copy link
Contributor Author

@liat-grozovik could you please help to merge?

@lguohan
Copy link
Contributor

lguohan commented Aug 23, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik liat-grozovik merged commit 6fd0675 into sonic-net:master Aug 25, 2021
yejianquan pushed a commit to yejianquan/sonic-mgmt that referenced this pull request Jan 20, 2022
Ignore inner_hashing test on non-master image.

The dependent change in sonic-net/sonic-utilities#1701 is not back port into 202012 branch yet. Hence ```config pbh table``` command is not supported.

Signed-off-by: bingwang <bingwang@microsoft.com>
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.

7 participants