-
Notifications
You must be signed in to change notification settings - Fork 520
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
SwSS Changes for DHCP DoS Mitigation Feature #3130
base: master
Are you sure you want to change the base?
Conversation
@Yarden-Z Hi Yarden, Can you please help review this code. Thanks in advance! |
b761b27
to
d22882a
Compare
Hi @prsunny, could you please help us by reviewing the code? |
i have to go to previous commit and to do work and then come back new work into my original brnach
orchagent/port/porthlpr.cpp
Outdated
@@ -667,6 +667,7 @@ bool PortHelper::parsePortLinkTraining(PortConfig &port, const std::string &fiel | |||
return true; | |||
} | |||
|
|||
|
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 please revert the changes that are not part of this PR? like this extra line?
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.
We removed the extra lines that were added during the coding process.
<< TC_CMD << " filter add dev " << shellquote(alias) << " protocol ip parent ffff: prio 1 u32 match ip protocol 17 0xff match ip dport 67 0xffff police rate " << to_string(byte_rate) << "bps burst " << to_string(byte_rate) << "b conform-exceed drop"; | ||
cmd_str = cmd.str(); | ||
ret = swss::exec(cmd_str, res); | ||
SWSS_LOG_WARN("ret Value in setPortDHCPMitigationRate is ret:%d,", ret); |
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 is this log warn? Seems it doesn't mean unexpected situation?
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 log SWSS_LOG_WARN("ret Value in setPortDHCPMitigationRate is ret:%d,", ret); was added temporarily for debugging purposes to track failures and will be removed.
orchagent/port/porthlpr.cpp
Outdated
@@ -1159,13 +1160,8 @@ bool PortHelper::parsePortConfig(PortConfig &port) const | |||
return false; | |||
} | |||
} | |||
else if (field == PORT_DESCRIPTION) |
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 remove this part?
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.
this part was removed by mistake
@@ -138,7 +138,7 @@ def _verify_db_contents(): | |||
return (True, None) | |||
|
|||
# Verify that ASIC DB has been fully initialized | |||
init_polling_config = PollingConfig(2, 30, strict=True) | |||
init_polling_config = PollingConfig(2, 60, strict=True) |
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 we change this value from 30 to 60?
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 timeout duration was increased after analyzing the failures and identifying that the issue might be related to initialization timing. Adding a delay helped resolve the problem, and the build was successful. However, when these delays were removed, the dvs_test failed again, indicating that the delays are necessary for the DVS to be ready.
@@ -1848,6 +1856,7 @@ def update_dvs(log_path, new_dvs_env=[]): | |||
dvs.destroy_servers() | |||
dvs.create_servers() | |||
dvs.restart() | |||
time.sleep(60) |
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 do we need sleep 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.
I added a delay here to ensure that once the DVS switch starts, there is a pause before it performs any further actions, allowing sufficient time for the ports to initialize properly.
@@ -79,7 +79,7 @@ def test_remove_add_remove_port_with_buffer_cfg(self, dvs, testlog): | |||
config_db.delete_entry('PORT', PORT_A) | |||
num = asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_PORT", | |||
num_of_ports-1, | |||
polling_config = PollingConfig(polling_interval = 1, timeout = 5.00, strict = True)) | |||
polling_config = PollingConfig(polling_interval = 1, timeout = 30.00, strict = True)) |
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 increase timeout?
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 timeout duration was increased after analyzing the failures and identifying that the issue might be related to initialization timing. Adding a delay helped resolve the problem, and the build was successful. However, when these delays were removed, the dvs_test failed again, indicating that the delays are necessary for the DVS to be ready.
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.
LGTM
@Blueve, @dgsudharsan, @prabhataravind . could you please help us by reviewing the code? |
What I did
Added support for DHCP DoS Mitigation feature via kernel through PortMgrd
Why I did it
DHCP Mitigation Rate attribute added to config_db needed a path to appl_db and kernel configurations. This was done through PortMgrd