Skip to content

Change update_feature_state call to pass False as default if feature has no 'has_timer' field#5260

Merged
tahmed-dev merged 2 commits intosonic-net:masterfrom
noaOrMlnx:noaor_hostcfgd
Sep 14, 2020
Merged

Change update_feature_state call to pass False as default if feature has no 'has_timer' field#5260
tahmed-dev merged 2 commits intosonic-net:masterfrom
noaOrMlnx:noaor_hostcfgd

Conversation

@noaOrMlnx
Copy link
Copy Markdown
Collaborator

- Why I did it
In order to make hostcfgd not to fail if feature has no 'has_timer' field
- How I did it
Change the update_feature_state call to pass False by default.
- How to verify it
Enable feature that has no 'has timer' field and make sure hostcfgd not failing.
- Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@noaOrMlnx
Copy link
Copy Markdown
Collaborator Author

retest vsimage please

liat-grozovik
liat-grozovik previously approved these changes Aug 27, 2020
@liat-grozovik
Copy link
Copy Markdown
Collaborator

retest vsimage please

@tahmed-dev
Copy link
Copy Markdown
Contributor

tahmed-dev commented Aug 27, 2020

@noaOrMlnx, can you please check with latest master. There was an issue where has_timer was passed in as a string and so it was evaluating to True all the time. That issues was fixed via PR:5248

@noaOrMlnx
Copy link
Copy Markdown
Collaborator Author

@tahmed-dev I checked an image with the merged PR.
The problem is caused when calling update_feature_state function, and trying to receive the has_timer field, even though there is no such field.
update_feature_state(feature_name, state, feature_table[feature_name]['has_timer']) -> this call makes the feature initialization to fail if the feature doesn't have the field.
This PRs purpose is to prevent the failure.

@noaOrMlnx
Copy link
Copy Markdown
Collaborator Author

retest vsimage please

1 similar comment
@noaOrMlnx
Copy link
Copy Markdown
Collaborator Author

retest vsimage please

New changes can be found in PR:5248
@liat-grozovik
Copy link
Copy Markdown
Collaborator

retest baseimage please

@tahmed-dev
Copy link
Copy Markdown
Contributor

@tahmed-dev I checked an image with the merged PR.
The problem is caused when calling update_feature_state function, and trying to receive the has_timer field, even though there is no such field.
update_feature_state(feature_name, state, feature_table[feature_name]['has_timer']) -> this call makes the feature initialization to fail if the feature doesn't have the field.
This PRs purpose is to prevent the failure.

@noaOrMlnx what is the exact scenario where this new field has_timer would go missing? The reason is I see all feature have their has_timer fields populated in init_cfg.json.j:21. So, it is not clear to me which scenario requires this change.

@noaOrMlnx
Copy link
Copy Markdown
Collaborator Author

@noaOrMlnx what is the exact scenario where this new field has_timer would go missing? The reason is I see all feature have their has_timer fields populated in init_cfg.json.j:21. So, it is not clear to me which scenario requires this change.

@tahmed-dev This change is required for external features we have. Instead of changing the feature structure, this change will prevent the system to crush if the external feature doesn't have this field.

@tahmed-dev tahmed-dev merged commit 353003f into sonic-net:master Sep 14, 2020
abdosi pushed a commit that referenced this pull request Sep 19, 2020
…has no 'has_timer' field (#5260)

* Pass False as default if feature has no timer field

* Update hostcfgd to fit the new changes merged

New changes can be found in PR:5248
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
…has no 'has_timer' field (sonic-net#5260)

* Pass False as default if feature has no timer field

* Update hostcfgd to fit the new changes merged

New changes can be found in PR:5248
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