Bug 1837953: fixup server_id regex #694

rbbratta · 2020-07-07T03:03:18Z

We need to search the whole line for the server_id.

Current regex is too aggressive.

The original example indicates that the prev_server line is a list of servers

$ ovsdb-tool show-log /etc/ovn/ovnnb_db.db | grep "server_id\|prev_servers"
 server_id: d5db
  prev_servers: 33b4("ssl:10.19.138.37:9643"), 720d("ssl:10.19.138.33:9643"), 7e4f("ssl:10.19.138.32:9643"

The above DB is inconsistent because d5db is not part of prev_servers.

A consistent DB looks like this:

$ ovsdb-tool show-log /etc/ovn/ovnnb_db.db | grep "server_id\|prev_servers"
 server_id: 33b4
  prev_servers: 33b4("ssl:10.19.138.37:9643"), 720d("ssl:10.19.138.33:9643")

danwinship

makes sense. I have some minor nitpicks

danwinship · 2020-07-07T17:19:18Z

bindata/network/ovn-kubernetes/ovnkube-master.yaml

-            serverid=$(ovsdb-tool show-log "${db}" | sed -ne '/server_id:/s/server_id: *\([[:xdigit:]]\+\)/\1/p')
-            match=$(ovsdb-tool show-log "${db}" | grep "prev_servers: *${serverid}(" || true)
+            # remove leading and trailing space around server_id
+            serverid=$(ovsdb-tool show-log "${db}" | sed -ne '/server_id:/s/ *server_id: *\([[:xdigit:]]\+\).*/\1/p')


The sed can be simplified to just

sed -ne 's/ *server_id: *$[[:xdigit:]]\+$.*/\1/p'

The s/// rule already won't match if there's no "server_id:" in the string, so the extra /server_id:/ at the front wasn't doing anything

I was originally matching the original grep plus sed formulation and the simpler regex /server_id:/ would be faster to match than the substitution regex but it won't matter much.

I finally checked the ovsdb-tools source and there is a fixed amount of space after the server_id: so we can use a positive lookbehind with grep -o and that should simplify things. Also threw in a -m 1 to exit after the first match for good measure, assuming that there should always be only one match. The -m 1 causes ovsdb-tool to complain about the broken pipe but that is okay since it's on stderr.

grep -m 1 -oP '(?<=server_id: )[[:xdigit:]]+'

danwinship · 2020-07-07T17:21:02Z

bindata/network/ovn-kubernetes/ovnkube-master.yaml

-            match=$(ovsdb-tool show-log "${db}" | grep "prev_servers: *${serverid}(" || true)
+            # remove leading and trailing space around server_id
+            serverid=$(ovsdb-tool show-log "${db}" | sed -ne '/server_id:/s/ *server_id: *\([[:xdigit:]]\+\).*/\1/p')
+            # prev_servers is a list we are searching for " $server_id(" in the list


Either that needs to be two sentences (period before "we") or the "the list" at the end is superfluous.

danwinship · 2020-07-07T17:21:31Z

bindata/network/ovn-kubernetes/ovnkube-master.yaml

+            # remove leading and trailing space around server_id
+            serverid=$(ovsdb-tool show-log "${db}" | sed -ne '/server_id:/s/ *server_id: *\([[:xdigit:]]\+\).*/\1/p')
+            # prev_servers is a list we are searching for " $server_id(" in the list
+            # prev_servers: 33b4("ssl:10.19.138.37:9643"), 720d("ssl:10.19.138.33:9643"), 7e4f("ssl:10.19.138.32:9643"


ok, now I'm totally just nitpicking but presumably there's a ")" missing at the end of that line...

This was copied from the original bugzilla. I have yet to see multiple prev_servers in testing, but I checked the source and there should always be a closing paren.

rbbratta · 2020-07-08T03:43:10Z

/cc @fedepaol

fedepaol · 2020-07-08T07:25:22Z

Looks good, thanks for fixing! I think in my original (despictable) implementation I included this case, but it got eventually got lost while improving it.
I wonder if we should point the bz from this PR too. In that case, @rbbratta would you mind changing the title?

fedepaol · 2020-07-08T07:25:30Z

/retest

fedepaol · 2020-07-08T07:25:43Z

/lgtm

danwinship · 2020-07-08T18:08:57Z

bindata/network/ovn-kubernetes/ovnkube-master.yaml

@@ -123,8 +123,10 @@ spec:
                  return
            fi
            echo "Checking ${db} health"
-            serverid=$(ovsdb-tool show-log "${db}" | sed -ne '/server_id:/s/server_id: *\([[:xdigit:]]\+\)/\1/p')
-            match=$(ovsdb-tool show-log "${db}" | grep "prev_servers: *${serverid}(" || true)
+            serverid=$(ovsdb-tool show-log "${db}" | grep -m 1 -oP '(?<=server_id: )[[:xdigit:]]+')


🤷‍♂️ I like the original version better... I feel like it's reasonable to expect future people reading this to understand sed syntax but not reasonable to expect them to know GNU-specific grep flags... Which may be a totally unreasonable expectation?

grep -o seems cleaner instead of trying to remove all the text surrounding the match. It feels kinda sad that GNU maintainers add all these nice features (-o and PCRE, for example, ~20 years ago) and no one uses them.

fedepaol · 2020-07-12T12:16:09Z

/retest

fedepaol · 2020-07-13T12:46:24Z

@rbbratta do you mind referring the bug in the title?
The bug is half baked as of today and I am afraid we loose track.

dcbw · 2020-07-14T14:52:28Z

/retest

@rbbratta yes, we should have the bug in the title as well.

rcarrillocruz · 2020-07-14T14:58:57Z

i.e.
Bug X:

rcarrillocruz · 2020-07-14T14:59:27Z

the description should follow the colon and a space

We need to search the whole line for the server_id. Current regex is too aggressive. The original example indicates that the prev_server line is a list of servers ```bash $ ovsdb-tool show-log /etc/ovn/ovnnb_db.db | grep "server_id\|prev_servers" server_id: d5db prev_servers: 33b4("ssl:10.19.138.37:9643"), 720d("ssl:10.19.138.33:9643"), 7e4f("ssl:10.19.138.32:9643") ``` The above DB is inconsistent because d5db is not part of prev_servers. A consistent DB looks like this: ```bash $ ovsdb-tool show-log /etc/ovn/ovnnb_db.db | grep "server_id\|prev_servers" server_id: 33b4 prev_servers: 33b4("ssl:10.19.138.37:9643"), 720d("ssl:10.19.138.33:9643") ```

openshift-ci-robot · 2020-07-14T16:04:39Z

@rbbratta: This pull request references Bugzilla bug 1837953, which is invalid:

expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is VERIFIED instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1837953: fixup server_id regex

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.

dcbw · 2020-07-14T16:58:56Z

If this is a follow-on to the already fixed bug, then we need a new bug cloned from the original, target release 4.6, so that QE can verify this additional fix separate from the original one.

openshift-ci-robot · 2020-07-15T21:54:45Z

@rbbratta: This pull request references Bugzilla bug 1857455, 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.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1857455: fixup server_id regex

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.

fedepaol · 2020-07-20T14:09:22Z

/retest

sdodson · 2020-07-20T14:32:15Z

/retitle Bug 1837953: fixup server_id regex
This is a followup to fixes in #688

openshift-ci-robot · 2020-07-20T14:32:35Z

@rbbratta: This pull request references Bugzilla bug 1837953, which is valid. 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.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1837953: fixup server_id regex

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.

rbbratta · 2020-07-20T15:49:41Z

bindata/network/ovn-kubernetes/ovnkube-master.yaml

@@ -123,8 +123,10 @@ spec:
                  return
            fi
            echo "Checking ${db} health"
-            serverid=$(ovsdb-tool show-log "${db}" | sed -ne '/server_id:/s/server_id: *\([[:xdigit:]]\+\)/\1/p')
-            match=$(ovsdb-tool show-log "${db}" | grep "prev_servers: *${serverid}(" || true)
+            serverid=$(ovsdb-tool show-log "${db}" | grep -m 1 -oP '(?<=server_id: )[[:xdigit:]]+')


https://github.com/openvswitch/ovs/blob/master/ovsdb/ovsdb-tool.c#L881

printf(" server_id: "SID_FMT"\n", SID_ARGS(&h->sid));

https://github.com/openvswitch/ovs/blob/master/ovsdb/raft-private.h#L36

#define SID_FMT "%04x"

rbbratta · 2020-07-20T15:53:31Z

bindata/network/ovn-kubernetes/ovnkube-master.yaml

+            serverid=$(ovsdb-tool show-log "${db}" | grep -m 1 -oP '(?<=server_id: )[[:xdigit:]]+')
+            # prev_servers is a list.  We are searching for " $server_id(" in the list
+            # prev_servers: 33b4("ssl:10.19.138.37:9643"), 720d("ssl:10.19.138.33:9643"), 7e4f("ssl:10.19.138.32:9643")
+            match=$(ovsdb-tool show-log "${db}" | grep "prev_servers:.* ${serverid}(" || true)


https://github.com/openvswitch/ovs/blob/master/ovsdb/ovsdb-tool.c#L897

print_servers("prev_servers", h->snap.servers);

https://github.com/openvswitch/ovs/blob/master/ovsdb/ovsdb-tool.c#L828

printf(" %s: ", name); const struct shash_node **nodes = shash_sort(json_object(servers)); size_t n = shash_count(json_object(servers)); for (size_t i = 0; i < n; i++) { if (i > 0) { printf(", "); } const struct shash_node *node = nodes[i]; printf("%.4s(", node->name); const struct json *address = node->data; char *s = json_to_string(address, JSSF_SORT); fputs(s, stdout); free(s); putchar(')'); }

dcbw · 2020-07-20T16:00:20Z

/override ci/prow/e2e-gcp-upgrade
that job is for openshift-sdn and this PR only touches ovn-kubernetes

openshift-ci-robot · 2020-07-20T16:00:42Z

@dcbw: Overrode contexts on behalf of dcbw: ci/prow/e2e-gcp-upgrade

In response to this:

/override ci/prow/e2e-gcp-upgrade
that job is for openshift-sdn and this PR only touches ovn-kubernetes

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.

dcbw · 2020-07-20T16:23:32Z

/approve
/lgtm

openshift-ci-robot · 2020-07-20T16:23:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, fedepaol, rbbratta

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dcbw]

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

openshift-bot · 2020-07-20T16:25:37Z

/retest

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

openshift-bot · 2020-07-20T16:38:42Z

/retest

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

openshift-bot · 2020-07-20T16:51:35Z

/retest

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

openshift-bot · 2020-07-20T17:04:36Z

/retest

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

openshift-bot · 2020-07-20T18:22:36Z

/retest

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

openshift-bot · 2020-07-20T19:15:05Z

/retest

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

openshift-ci-robot · 2020-07-20T19:18:21Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-ovn-upgrade	`6c02777`	link	`/test e2e-gcp-ovn-upgrade`
ci/prow/e2e-openstack	`6c02777`	link	`/test e2e-openstack`
ci/prow/e2e-vsphere	`6c02777`	link	`/test e2e-vsphere`

Full PR test history. Your PR dashboard.

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-07-20T19:38:04Z

@rbbratta: All pull requests linked via external trackers have merged: openshift/cluster-network-operator#694, openshift/cluster-network-operator#688. Bugzilla bug 1837953 has been moved to the MODIFIED state.

In response to this:

Bug 1837953: fixup server_id regex

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.

It turned out the prev_servers list contains only the leader, so it can't be used to understand if the db is consistent or not. This leads in always deleting the db file when restarting a ovn-master pod that is not the leader. More details in https://bugzilla.redhat.com/show_bug.cgi?id=1837953#c36 Revert "Merge pull request openshift#694 from rbbratta/fix-server_id_regex" This reverts commit 3ff3b90, reversing changes made to a8ababe. Revert "Merge pull request openshift#688 from fedepaol/dbhealthy" This reverts commit a0aaca7, reversing changes made to 2f04083.

openshift-ci-robot requested review from JacobTanenbaum and pecameron July 7, 2020 03:03

danwinship reviewed Jul 7, 2020

View reviewed changes

rbbratta force-pushed the fix-server_id_regex branch 3 times, most recently from af4fbe4 to 72590b6 Compare July 8, 2020 00:25

openshift-ci-robot requested a review from fedepaol July 8, 2020 03:43

openshift-ci-robot assigned fedepaol Jul 8, 2020

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

danwinship reviewed Jul 8, 2020

View reviewed changes

rbbratta force-pushed the fix-server_id_regex branch from 72590b6 to 6c02777 Compare July 14, 2020 16:03

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

rbbratta changed the title ~~fixup server_id regex~~ Bug 1837953: fixup server_id regex Jul 14, 2020

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

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

rbbratta changed the title ~~Bug 1837953: fixup server_id regex~~ Bug 1857455: fixup server_id regex Jul 15, 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 Jul 15, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jul 15, 2020

openshift-ci-robot changed the title ~~Bug 1857455: fixup server_id regex~~ Bug 1837953: fixup server_id regex Jul 20, 2020

rbbratta commented Jul 20, 2020

View reviewed changes

openshift-ci-robot assigned dcbw Jul 20, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 20, 2020

openshift-merge-robot merged commit 3ff3b90 into openshift:master Jul 20, 2020

fedepaol mentioned this pull request Jul 23, 2020

Bug 1858834: Revert ovn db consistency check. #723

Merged

Bug 1837953: fixup server_id regex #694

Bug 1837953: fixup server_id regex #694

Conversation

rbbratta commented Jul 7, 2020

danwinship left a comment

Choose a reason for hiding this comment

danwinship Jul 7, 2020

Choose a reason for hiding this comment

rbbratta Jul 8, 2020 • edited

Choose a reason for hiding this comment

danwinship Jul 7, 2020

Choose a reason for hiding this comment

danwinship Jul 7, 2020

Choose a reason for hiding this comment

rbbratta Jul 8, 2020

Choose a reason for hiding this comment

rbbratta commented Jul 8, 2020

fedepaol commented Jul 8, 2020

fedepaol commented Jul 8, 2020

fedepaol commented Jul 8, 2020

danwinship Jul 8, 2020

Choose a reason for hiding this comment

rbbratta Jul 8, 2020

Choose a reason for hiding this comment

fedepaol commented Jul 12, 2020

fedepaol commented Jul 13, 2020

dcbw commented Jul 14, 2020

rcarrillocruz commented Jul 14, 2020

rcarrillocruz commented Jul 14, 2020

openshift-ci-robot commented Jul 14, 2020

dcbw commented Jul 14, 2020

openshift-ci-robot commented Jul 15, 2020

fedepaol commented Jul 20, 2020

sdodson commented Jul 20, 2020

openshift-ci-robot commented Jul 20, 2020

rbbratta Jul 20, 2020

Choose a reason for hiding this comment

rbbratta Jul 20, 2020 • edited

Choose a reason for hiding this comment

dcbw commented Jul 20, 2020

openshift-ci-robot commented Jul 20, 2020

dcbw commented Jul 20, 2020

openshift-ci-robot commented Jul 20, 2020

openshift-bot commented Jul 20, 2020

openshift-bot commented Jul 20, 2020

openshift-bot commented Jul 20, 2020

openshift-bot commented Jul 20, 2020

openshift-bot commented Jul 20, 2020

openshift-bot commented Jul 20, 2020

openshift-ci-robot commented Jul 20, 2020 • edited

openshift-ci-robot commented Jul 20, 2020

rbbratta Jul 8, 2020 •

edited

rbbratta Jul 20, 2020 •

edited

openshift-ci-robot commented Jul 20, 2020 •

edited