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.
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
Bug 1974364: Change the way of gathering ovn db #245
Bug 1974364: Change the way of gathering ovn db #245
Conversation
…s to ovsdb-client backup. This change should solve the following problems: - copying the .db file might pick partial data - even after compacting, it's not guaranteed that there are no transactions (which could happen after compacting and before copying the file). This makes it difficult for parsers other than ovsdb-server, e.g: insights. Also, files from ovsdb-client backup may be used by ovsdb-client restore for local debugging
@npinaeva: This pull request references Bugzilla bug 1974364, which is invalid:
Comment In response to this:
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. |
/bugzilla refresh |
@npinaeva: This pull request references Bugzilla bug 1974364, 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
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (anusaxen@redhat.com), skipping review request. In response to this:
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. |
Thanks for the PR @npinaeva.
Can you check if, as you also point out, ovsdb-server has removed the "ephemeral" flag of that column? You should be able to see it in the schema, which is the first OVSDB JSON object:
If we want to keep ephemeral columns, another approach that could at least guarantee we don't store transactions (which make the file bigger and difficult its parsing by programs other than ovsdb-server, such as insights) is: |
Also, if you end up using ovsdb-client backup, you might want to the |
Thank you for the comments, @amorenoz
But
Also found this comment https://bugzilla.redhat.com/show_bug.cgi?id=1818754#c8 where db on update states that ephemeral columns are not supported for clusters and are changed to persistent (our Connection table has 2 ephemeral columns: status and is_connected), so maybe these columns are not ephemeral for a long time now, which also means that it shouldn't be a problem for gathering. The next thing we should decide on is desired format for gathered db files
If we first copy file and then compact, I think there is no difference with creating a backup for parsers (both methods just create a snapshot of db)? About file size: the difference for just created cluster is (old way->new way): nbdb=148kb->206kb, sbdb=2mb->2.2mb. Not sure if this difference is considerable and if it's going to increase on a real-life cluster. So, I think there is no need to copy db file manually, we can use the benefits of backup considering the difference in file size is not so important to us. What do you think? I also think we may need @astoycos opinion on that (and also to confirm that ephemeral columns may not exist anymore) |
> "${NETWORK_LOG_PATH}"/"${OVNKUBE_MASTER_POD}"_sbdb_size_pre_compact | ||
|
||
oc exec -n openshift-ovn-kubernetes "${OVNKUBE_MASTER_POD}" -c sbdb -- ovs-appctl -t /var/run/ovn/ovnsb_db.ctl ovsdb-server/compact | ||
oc exec -n openshift-ovn-kubernetes "${OVNKUBE_MASTER_POD}" -c sbdb -- ovsdb-client backup unix:/var/run/ovn/ovnsb_db.sock> \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space before >
We should definitely do this. You could implement logic so that if the
No I don't think this is considerable, we should try to not move back to On the ephemeral columns, I don't think they are super important for debugging... If the server is not connected to the db then we would most likely be seeing other "no connection" errors all over OVN/ OVN-K |
Looking at some of the problems we're having with large-scale clusters, I think I've changed my mind. |
Thinking about this "to cp or not to cp " dilemma. Writing into the file is done using buffered IO which is flushed just after each transaction: Without locking mechanism, it's technically possible copy partial data. However, this should not be very often, in fact, it's possibility is neglected even by the ovsdb-client man page that claims:
If this rare situation does happen, we can always ask for another mustgather report. However, I think interacting with the system we're trying to gather information from is wrong by design. Besides, if ovsdb-server itself is crashed, hung, buggy, etc (which might even be the cause we want to pick up information in the first place), all these interactions could fail.
Agree |
Sooo, the key point here from ovsdb-client man page is:
And ovs docs says
So, our case is clustered db, and ovsdb-tool doesn't work with clustered dbs:
Thus, if we don't want to bother ovsdb-server we can only
We can also try to run ovsdb-server somewhere out of cluster and make it handle copied ovsdb files, but since db is clustered, and you can see fields like "local_address": "ssl:10.0.140.173:9643" in db files, I think you can't just change its db file and making "a copy" of ovsdb cluster may be a struggle. The only way to get nice output directly is to ask ovsdb-server either to compact db or to backup it, meaning we have to bother ovsdb-server, so I've run out of ideas here. Any thoughts please? |
Also, from ovsdb(7)
Which I think means that using ovsdb-client backup is the only way to gather data from a cluster and also the only way to restore data from a snapshot later. |
Ugh. We could use
If we 'cp' first we need to drop the raft information, but maybe we can print it in another manner. I'll take a deeper look at the clutered-related commands to see what is less intrusive (backup, compact or dumping the clustered data) |
I think ovsdb-client backup also returns db in a standalone format, because there is no specific format for clustered db dumps (Clustered db can also be restored from standalone format). I am also wondering do you think we need this "clustered" format of db? I can't see how it can be used as opposed to standalone? |
If there are issues with the ovsdb-server clustering algorithm (raft), we would need to look into the clustered-only columns/logs. So, we should see if this information can be dumped by other means. |
I see, but then I suppose we use
show-log:
And then we can have |
My biggest concern is the potential impact on the cluster itself if they are big (e.g: stopping pods from coming up for 10s of seconds). I would say the safest thing would be:
cc @dceara |
Do you mean run |
I think @amorenoz meant But this makes me wonder, why not just copy the original, uncompacted, db file; gzip it and that's it? There's the small risk that we get some inconsistent data (because of buffered I/O) but except for that we would be getting the most useful information. Other tools can later run offline compaction/change to standalone, if it makes their life easier. We only have 3 SB DB files (maybe larger) and 3 NB DB files (usually way smaller than the SB ones). |
ovsdb-tool cannot be filtered right? I don't see the benefit of 'ovsdb-tool show-log' vs copying the uncompacted, clusted db file.
The problem is those tools have to be run manually since insights cannot run ovs binary automatically. This whole discussion started as a way to better integrate with insights. However, the conclusion seems to be that we only have 3 ways to go:
We can keep discussing things like:
But the more we discuss, the more I lean towards |
With 2 we also lose recent transaction history too, which, if the data is collected early, is often very useful for debugging. E.g., it allows checking when something was deleted, whereas in the compacted version we just don't have the record.
Me too. |
I think we still will loose some information about transactions because ovsdb-server automatically compacts databases when they grow too much (and compacting only leaves current db state in file), but copying db file really seems like the only way to collect data without interacting with ovsdb. And since just copying raw db file is the most informative way to collect the data, maybe we should do that (although extracting transactions data from that file may be difficult). Do we agree on that? |
It's actually the opposite, having the raw db file is the only way to get transactions data. So this is a plus in my opinion.
This sounds good to me (from core OVN perspective) but I'll let @amorenoz share his opinions too from tools (insights) perspective. |
I'm comparing extracting transactions data from db file with reading nice output from |
I think we don't have any other way. We'll have to work around the format being difficult to parse at the insights/tooling stage. +1 for just copying the file |
I am a bit worried about the size of these db files that were collecting and who much that will affect the overall size of the must-gather archive. If I recall we made a switch away from collecting the db files directly so that we could reduce the overall size of the archive. That said; given this script gather_network_logs is optional (and not run by default) I don't see an issue switching back to this if engineering feel its the only way to debug situations within the SDN. That said we may want to warn users about the DB/archive size that might be generated. |
From another thread, @dceara reported the following sizes of the db files on a scale test (i.e: we could consider these as worst-case): uncompacted: 319M (66M gzipped) I agree that if this was to be collected by default we would have to find a way to reduce the footprint significantly. If these numbers are still too high, we could consider having two flavors of |
So, do you think this PR needs some changes? Or we leave it as is? |
Leave it |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: npinaeva, sferich888 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:
Approvers can indicate their approval by writing |
@npinaeva: All pull requests linked via external trackers have merged: Bugzilla bug 1974364 has been moved to the MODIFIED state. In response to this:
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. |
Change the way of gathering ovs db from compacting + copying db files to ovsdb-client backup.
This change should solve the following problems:
Also, files from ovsdb-client backup may be used by ovsdb-client restore for local debugging.
Test:
Run must-gather, check
network_logs/<OVNKUBE_MASTER_POD>_nbdb.gz
andnetwork_logs/<OVNKUBE_MASTER_POD>_sbdb.gz
files.Example of previous nbdb file:
Example of current nbdb file (contains information from "prev_data" of old nbdb file):
New version of db files also contains more fields, e.g.:
old version:
new version:
About ephemeral columns - new db files contain ephemeral columns like Connection.is_connected https://github.com/ovn-org/ovn/blob/master/ovn-nb.ovsschema#L467 although ovsdb-client backup docs say
Not sure if it's the case, but this link says