[openshift_ovn] Allow specific container runtime for plugin command#4076
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
| container='ovnkube-master') | ||
| self.add_cmd_output([ | ||
| 'ovs-appctl -t /var/run/ovn/ovn-controller.*.ctl ' + | ||
| 'ct-zone-list'], | ||
| container='ovnkube-node') | ||
| self.add_cmd_output([ | ||
| 'ovs-appctl -t /var/run/ovn/ovn-controller.*.ctl ' + | ||
| 'ct-zone-list'], | ||
| container='ovnkube-controller') | ||
| container='ovnkube-master', | ||
| runtime='crio') | ||
| # We need to determine the actual file name to send | ||
| # to the command | ||
| files = glob.glob("/var/run/ovn/ovn-controller.*.ctl") | ||
| for file in files: |
There was a problem hiding this comment.
Does this collection not behave correctly when directly specifying the glob? Is that why we're also changing this to a per-file iteration?
There was a problem hiding this comment.
When this command was getting executed, the filename '/var/run/ovn/ovn-controller..ctl' parameter within the command string was not getting expanded. The '' represents the current process id of the container, ie, '/var/run/ovn/ovn-controller.3127.ctl', which lead to a "File Not Found Error" condition when executing the command. I changed to find the filename before passing the command string into add_cmd_output.
| self.add_cmd_output([ | ||
| 'ovs-appctl -t ' + file + | ||
| ' ct-zone-list'], | ||
| container='ovnkube-node', | ||
| runtime='crio') | ||
| self.add_cmd_output([ | ||
| 'ovs-appctl -t ' + file + | ||
| ' ct-zone-list'], | ||
| container='ovnkube-controller', | ||
| runtime='crio') |
There was a problem hiding this comment.
a_c_o() takes a list or a string. With this change now doing single files at a time the list is superfluous.
Also, please convert these to f-strings. A while back we made an effort to convert everything we could to f-strings, but I guess we overlooked this.
There was a problem hiding this comment.
These are separate calls because they target different containers and the container parameter is not part of the list. These calls previously existed, I simply updated them to pass the file name correctly. I will update the 3 calls that I changed to use f-strings.
Accidentally hit approve on this instead of comment.
97a1a71 to
f512efd
Compare
TurboTurtle
left a comment
There was a problem hiding this comment.
LGTM. If you could please rebase to current main, we'll get this merged.
f512efd to
deed2a6
Compare
This Update allows a specific container runtime to be passed to add_cmd_output so commands that must use a specific runtime do so. Related: SUPDEV-180 Signed-off-by: David Wolstromer <dwolstro@redhat.com>
deed2a6 to
b583702
Compare
This Update allows a specific container runtime to be passed to add_cmd_output so commands that must use a specific runtime do so.
Related: SUPDEV-180
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines