Add ovn-monitor-all configuration option to ovn-controller#106
Add ovn-monitor-all configuration option to ovn-controller#106goldberl wants to merge 1 commit intoopenstack-charmers:masterfrom
Conversation
goldberl
force-pushed
the
bug/2138758-add-ovn-monitor-all
branch
from
824dbc6 to
6c15dc2
Compare
| pinning to avoid data plane outage during the upgrade. | ||
| ovn-monitor-all: | ||
| type: boolean | ||
| default: false |
There was a problem hiding this comment.
In my opinion we should default to true, as we have found in our deployments it helps reduce CPU load. Will have to ask the OVN team for a review to see if they are Ok with that.
There was a problem hiding this comment.
I'm bit split on this decision. I'm leaning slightly towards default false because:
- It's cleaner to backport (i.e. existing deployment won't change their configuration on charm upgrade)
- This option is not always more efficient. Only in deployments where the controller needs to monitor almost all SB objects anyway (ref)
Though I don't have strong opinion. Perhaps we could get @fnordahl to chime in.
There was a problem hiding this comment.
Hi Martin,
Yes, it might be best to leave False since this needs to be backported, then maybe we open a new PR to change to True going forward. We have now seen this setting be useful in multiple customer deployments, and believe being opinionated here will help to not have additional escalations. From what I can tell it is also the default in the RH installer.
Would welcome Frode's opinion.
Thanks,
-Brian
There was a problem hiding this comment.
Okay changing it back to false for now
goldberl
force-pushed
the
bug/2138758-add-ovn-monitor-all
branch
from
6c15dc2 to
84d7aad
Compare
| pinning to avoid data plane outage during the upgrade. | ||
| ovn-monitor-all: | ||
| type: boolean | ||
| default: false |
There was a problem hiding this comment.
Hi Martin,
Yes, it might be best to leave False since this needs to be backported, then maybe we open a new PR to change to True going forward. We have now seen this setting be useful in multiple customer deployments, and believe being opinionated here will help to not have additional escalations. From what I can tell it is also the default in the RH installer.
Would welcome Frode's opinion.
Thanks,
-Brian
goldberl
force-pushed
the
bug/2138758-add-ovn-monitor-all
branch
from
84d7aad to
c761d4c
Compare
This commit adds the option 'ovn-monitor-all' to the OVN charm, allowing the operator to enable or disable monitoring of all records in the OVN_Southbound database by the ovn-controller. - Updated config.yaml to include 'ovn-monitor-all' (default: false) - Updated lib/charms/ovn_charm.py to handle the option in configure_bridges() - Added unit tests Closes-Bug: 2138758
goldberl
force-pushed
the
bug/2138758-add-ovn-monitor-all
branch
from
c761d4c to
0a881e5
Compare
brianphaley
left a comment
brianphaley
left a comment
There was a problem hiding this comment.
LGTM. Added Frode as reviewer.
3 participants
This PR adds the option
ovn-monitor-allto the OVN charm, allowing the operator to enable or disable monitoring of all records in the OVN_Southbound database by the ovn-controller.config.yamlto includeovn-monitor-all(default: false)lib/charms/ovn_charm.pyto handle the option inconfigure_bridges()Closes-Bug: LP 2138758