-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[subnet_decap] Use new tunnel&&decap term db schema and add subnet decap tunnel to ipinip.json
#18752
Conversation
2e22e18
to
bc0b2c5
Compare
please specify dependency so that we know when to merge. |
@@ -56,15 +55,24 @@ | |||
"ttl_mode":"pipe" | |||
}, | |||
"OP": "SET" | |||
} | |||
}{% if ipv4_addresses %}, |
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 don't see {% endif %} for this if condition.
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 feel like if we don't have ipv4_addresses
, and only have ipv6_loopback_addresses
, the ,
is also required.
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 don't see {% endif %} for this if condition.
Please check LINE#59, this check condition adds the ,
only if there are IPv4 decap terms.
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 feel like if we don't have
ipv4_addresses
, and only haveipv6_loopback_addresses
, the,
is also required.
If we don't have ipv4_addresses
, LINE#45 ~ LINE#69 will be skipped, and we will only have IPv6 tunnel && decap terms, there is no need to add the ,
as the IPv5 tunnel now comes to the begining.
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.
Is the else
at Line#59 to close the if
at Line#45?
{% endif %} | ||
{% endfor %} | ||
{% endif %} | ||
{% if ipv4_loopback_addresses and ipv6_loopback_addresses %}, |
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 ipv4_loopback_addresses
needs to be checked here?
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.
Let me list out all possible conditions:
if ipv4_loopback_addresses
is empty, ipv6_loopback_addresses
is empty: no tunnel && decap terms will be created, no need to add ,
.
if ipv4_loopback_addresses
is not empty, ipv6_loopback_addresses
is empty: only IPv4 tunnel && decap terms will be created, no need to add ,
.
if ipv4_loopback_addresses
is empty, ipv6_loopback_addresses
is not empty: only IPv6 tunnel && decap terms will be created, no need to add ,
.
if ipv4_loopback_addresses
is not empty, ipv6_loopback_addresses
is not empty: both IPv4&&IPv6 tunnel && decap terms will be created, ,
will be added to inter-connect those two parts.
Are you going to include the script for warm-reboot scenario in this PR? |
Added dependency in PR description. |
No, let's do this incrementally PR by PR:
|
ipinip.json.j2
template to use new db schemaipinip.json
Hi @bingwang-ms, added the subnet decap tunnel part to this PR, please help review. |
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
072983e
to
bc80a53
Compare
The PR test failure of |
/azpw ms_conflic |
@lolyu , then we cannot merge. can you have a plan to make sure there is no test failure? |
/azpw ms_conflic |
/azpw ms_conflic |
/azp rerun |
Command 'rerun' is not supported by Azure Pipelines. Supported commands
See additional documentation. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw ms_conflict |
1 similar comment
/azpw ms_conflict |
/azp run |
Commenter does not have sufficient privileges for PR 18752 in repo sonic-net/sonic-buildimage |
/azpw ms_conflict |
5 similar comments
/azpw ms_conflict |
/azpw ms_conflict |
/azpw ms_conflict |
/azpw ms_conflict |
/azpw ms_conflict |
…cap tunnel to `ipinip.json` (sonic-net#18752) * [decap] Enhance `ipinip.json.j2` template to use new db schema Signed-off-by: Longxiang Lyu <lolv@microsoft.com> Co-authored-by: bingwang <bingwang@microsoft.com>
Why I did it
Enable
ipinip.json.j2
to use the new DB schema to create decap tunnel and decap terms.This depends on: sonic-net/sonic-swss#3117
Signed-off-by: Longxiang Lyu lolv@microsoft.com
Work item tracking
How I did it
Use
TUNNEL_DECAP_TERM_TABLE
to create decap rules.How to verify it
UT and verify on testbed together with sonic-net/sonic-swss#3117.
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)