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

generic_config_updater: Filename changed & VLAN validator added #1919

Merged
merged 20 commits into from
Nov 24, 2021

Conversation

renukamanavalan
Copy link
Contributor

@renukamanavalan renukamanavalan commented Nov 10, 2021

What I did

Change filename per review comments from last PR.
Removed print statement from change_applier
Added vlan validator & test code for the same

How I did it

How to verify it

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)

2) Added vlan validator
3) Added test code for vlan validator
@renukamanavalan renukamanavalan changed the title generic_config_updater: Filename change generic_config_updater: Filename changed & VLAN validator added Nov 18, 2021
for key in set(old_vlan.keys()).union(set(upd_vlan.keys())):
if (old_vlan.get(key, {}).get("dhcp_servers", []) !=
upd_vlan.get(key, {}).get("dhcp_servers", [])):
return _service_restart("dhcp_relay")
Copy link
Contributor

Choose a reason for hiding this comment

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

NICE! We just restart dhcp at the first occurrence of mismatch between dhcp_servers in old vs updated.

ghooo
ghooo previously approved these changes Nov 22, 2021
setup.py Show resolved Hide resolved
@@ -39,6 +39,9 @@
},
"DHCP_SERVER": {
"services_to_validate": [ "dhcp-relay" ]
},
"VLAN": {
"services_to_validate": [ "vlan-service" ]
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 22, 2021

Choose a reason for hiding this comment

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

services_to_validate

According to the design doc https://github.com/Azure/SONiC/blob/master/doc/config-generic-update-rollback/Json_Change_Application_Design.md#3212-metadata. It fits better as restart-command than services_to_validate #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not blindly restart.
For example, in case of VLAN table update, we call vlan_validator, which may or may not restart dhcp_sevice.
Again a validator, could potentially restart more than one and possibly none too, as in vlan case.

So I prefer this name validator, which is table specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the intelligence of restarting dhcp_service for vlan-service. However, the validator is design as read-only operations for the purpose of validation, including validating services are completely restarted and healthy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get your last comments. But Design doc updated.

@qiluo-msft qiluo-msft merged commit 64777a4 into sonic-net:master Nov 24, 2021
abdosi pushed a commit that referenced this pull request Dec 8, 2021
#### What I did
Change filename per review comments from last PR.
Removed print statement from change_applier
Added vlan validator & test code for the same
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants