Skip to content

Bug 1887040: OVS config: check if OVS is installed #2154

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Oct 15, 2020
Merged

Bug 1887040: OVS config: check if OVS is installed #2154

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions templates/common/_base/files/configure-ovs-network.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@ contents:
inline: |
#!/bin/bash
set -eux
# Workaround to ensure OVS is installed due to bug in systemd Requires:
# https://bugzilla.redhat.com/show_bug.cgi?id=1888017
if ! rpm -qa | grep -q openvswitch; then
Copy link
Copy Markdown
Contributor

@dcbw dcbw Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@trozet openvswitch2.13 ? But will be fine the way it is.

Copy link
Copy Markdown
Contributor

@sinnykumari sinnykumari Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

maybe just use rpm -q openvswitch ? using grep here can also give a match for packages which has openvswitch as sub-string

Copy link
Copy Markdown
Contributor

@dcbw dcbw Oct 15, 2020

Choose a reason for hiding this comment

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

If we go for rpm -q, it would have to be rpm -q openvswitch2.13 FWIW

Copy link
Copy Markdown
Contributor Author

@trozet trozet Oct 15, 2020

Choose a reason for hiding this comment

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

"using grep here can also give a match for packages which has openvswitch as sub-string"

Yeah that was intentional. What if we move to openvswitch2.14 and the package changes names? I don't think that's better. Really openvswitch package should not have the version in its package name, but to workaround this we only look for openvswitch here.

Copy link
Copy Markdown
Contributor

@sinnykumari sinnykumari Oct 15, 2020

Choose a reason for hiding this comment

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

sure, if this is preferred way by the OVN team :)

Copy link
Copy Markdown
Contributor

@squeed squeed Oct 14, 2020

Choose a reason for hiding this comment

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

how about if ! command-v ovs-vsctl; then ?

Copy link
Copy Markdown
Contributor

@dcbw dcbw Oct 14, 2020

Choose a reason for hiding this comment

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

missing space, right? if ! command -v ovs-vsctl; then

Copy link
Copy Markdown
Contributor

@squeed squeed Oct 14, 2020

Choose a reason for hiding this comment

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

yes, indeed.

Copy link
Copy Markdown
Contributor Author

@trozet trozet Oct 14, 2020

Choose a reason for hiding this comment

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

why is checking for a binary better than checking if the RPM is installed?

Copy link
Copy Markdown
Member

@cybertron cybertron Oct 14, 2020

Choose a reason for hiding this comment

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

I suppose there's an argument that if the package got split and the binary moved to a different package it might be more robust to check the binary itself. That said, if I understand this correctly it's a workaround that only needs to exist for one release, right? After that openvswitch will have been installed and we don't need the check for future upgrades. We shouldn't need to worry about binaries moving around in that case.

I guess the one thorny issue would be if skip-level upgrades need to be supported. I'm pretty sure our IPI templates wouldn't handle that right now, but I don't know what UPI requires.

Copy link
Copy Markdown
Contributor Author

@trozet trozet Oct 14, 2020

Choose a reason for hiding this comment

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

if the binary gets split then its even more of an argument not to check for the binary. We need ovs-vswitchd up and running here not just ovs-vsctl binary. I'm going to stick with the ensuring the package is installed since that is actually what we care about for UPI. If openvswitch for some split into multiple packages, then we will need to update this check to look for those packages as well.

Copy link
Copy Markdown
Member

@cybertron cybertron Oct 14, 2020

Choose a reason for hiding this comment

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

Oh, yeah if the binary is not the thing you care about then that wouldn't make sense. I suppose you could check for ovs-vswitchd instead, but like I said it's a temporary workaround so I'm not inclined to get too hung up on the details.

That said, although I got assigned to this I don't have approval on this repo so this is just my 2 cents anyway. :-)

echo "Warning: Openvswitch package is not installed!"
exit 1
fi

if [ "$1" == "OVNKubernetes" ]; then
# Configures NICs onto OVS bridge "br-ex"
# Configuration is either auto-detected or provided through a config file written already in Network Manager
Expand Down