Bug 1887040: OVS config: check if OVS is installed by trozet · Pull Request #2154 · openshift/machine-config-operator

trozet · 2020-10-13T20:26:41Z

With UPI installs, openvswitch package is not installed until after
upgrade. In the ovs-configuration.service there is a Requires
declaration on openvswitch.service. However due to a bug in systemd,
ovs-configuration is started even when openvswitch.service is not
present:

https://bugzilla.redhat.com/show_bug.cgi?id=1888017

As a workaround, this patch checks to ensure OVS is installed in
configure-ovs.sh.

Signed-off-by: Tim Rozet trozet@redhat.com

With UPI installs, openvswitch package is not installed until after upgrade. In the ovs-configuration.service there is a Requires declaration on openvswitch.service. However due to a bug in systemd, ovs-configuration is started even when openvswitch.service is not present: https://bugzilla.redhat.com/show_bug.cgi?id=1888017 As a workaround, this patch checks to ensure OVS is installed in configure-ovs.sh. Signed-off-by: Tim Rozet <trozet@redhat.com>

openshift-ci-robot · 2020-10-13T20:26:50Z

@trozet: This pull request references Bugzilla bug 1887040, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1887040: OVS config: check if OVS is installed

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-10-14T00:57:32Z

@trozet: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/okd-e2e-aws	`6340d5e`	link	`/test okd-e2e-aws`
ci/prow/e2e-ovn-step-registry	`6340d5e`	link	`/test e2e-ovn-step-registry`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

zhaozhanqi · 2020-10-14T02:47:11Z

@trozet I saw 4.6 branch create the file at the begginging touch /var/run/ovs-config-executed
e16d4cd#diff-afb45a3711a77d94f26471d9d94a7f7a03d931d9e72bdf849f2e26e2711d6fd7
this no need for 4.7 version?

dcbw · 2020-10-14T13:56:05Z

    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


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

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

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

"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.

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

dcbw · 2020-10-14T13:57:04Z

/lgtm

squeed · 2020-10-14T13:57:20Z

    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


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

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

yes, indeed.

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

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.

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.

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. :-)

vrutkovs · 2020-10-14T18:47:58Z

OKD 4.6 is known to be broken right now (openshift/release#12683 to fix it), so feel free to skip optional tests

dcbw · 2020-10-15T14:18:42Z

/test e2e-ovn-step-registry

dcbw · 2020-10-15T14:23:21Z

/test e2e-ovn-step-registry

openshift-ci-robot · 2020-10-15T15:50:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, sinnykumari, trozet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [sinnykumari]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrutkovs · 2020-10-15T15:56:23Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2020-10-15T15:56:24Z

@vrutkovs: once the present PR merges, I will cherry-pick it on top of release-4.6 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-bot · 2020-10-15T21:54:13Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-15T22:07:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-15T22:59:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-15T23:51:14Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2020-10-15T23:54:49Z

@trozet: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-ovn-step-registry	`6340d5e`	link	`/test e2e-ovn-step-registry`
ci/prow/okd-e2e-aws	`6340d5e`	link	`/test okd-e2e-aws`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2020-10-15T23:57:24Z

@trozet: All pull requests linked via external trackers have merged:

openshift/machine-config-operator#2154

Bugzilla bug 1887040 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1887040: OVS config: check if OVS is installed

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2020-10-15T23:57:36Z

@vrutkovs: #2154 failed to apply on top of branch "release-4.6":

Applying: OVS config: check if OVS is installed
Using index info to reconstruct a base tree...
M	templates/common/_base/files/configure-ovs-network.yaml
Falling back to patching base and 3-way merge...
Auto-merging templates/common/_base/files/configure-ovs-network.yaml
CONFLICT (content): Merge conflict in templates/common/_base/files/configure-ovs-network.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 OVS config: check if OVS is installed
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. label Oct 13, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Oct 13, 2020

openshift-ci-robot requested review from ashcrow and sinnykumari October 13, 2020 20:27

dcbw reviewed Oct 14, 2020

View reviewed changes

squeed reviewed Oct 14, 2020

View reviewed changes

openshift-ci-robot assigned dcbw Oct 14, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020

ashcrow requested review from cybertron and removed request for ashcrow October 14, 2020 17:27

vrutkovs mentioned this pull request Oct 15, 2020

[Tracking] OKD 4.6 Stable okd-project/okd#348

Closed

6 tasks

sinnykumari approved these changes Oct 15, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2020

openshift-merge-robot merged commit 189ac22 into openshift:master Oct 15, 2020

vrutkovs mentioned this pull request Oct 17, 2020

Bug 1887607: OVS config: check if OVS is installed #2164

Merged

Conversation

trozet commented Oct 13, 2020

Uh oh!

openshift-ci-robot commented Oct 13, 2020

Uh oh!

openshift-ci-robot commented Oct 14, 2020

Uh oh!

zhaozhanqi commented Oct 14, 2020

Uh oh!

dcbw Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sinnykumari Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcbw commented Oct 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrutkovs commented Oct 14, 2020

Uh oh!

dcbw commented Oct 15, 2020

Uh oh!

dcbw commented Oct 15, 2020

Uh oh!

openshift-ci-robot commented Oct 15, 2020

Uh oh!

vrutkovs commented Oct 15, 2020

Uh oh!

openshift-cherrypick-robot commented Oct 15, 2020

Uh oh!

openshift-bot commented Oct 15, 2020

Uh oh!

openshift-bot commented Oct 15, 2020

Uh oh!

openshift-bot commented Oct 15, 2020

Uh oh!

openshift-bot commented Oct 15, 2020

Uh oh!

openshift-merge-robot commented Oct 15, 2020

Uh oh!

openshift-ci-robot commented Oct 15, 2020

Uh oh!

openshift-cherrypick-robot commented Oct 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

dcbw Oct 14, 2020 •

edited

Loading

sinnykumari Oct 15, 2020 •

edited

Loading