Skip to content
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

Gracefully handle empty db file on startup #315

Merged

Conversation

booxter
Copy link
Contributor

@booxter booxter commented Jul 1, 2024

In some situations, e.g. when ovsdb-tool is killed during db file initialization, the file may end up empty. On next restart, the service fails to start because the file format is invalid.

Ideally, ovs start scripts would handle this situation gracefully, as tracked in https://issues.redhat.com/browse/FDP-689

But in the meantime, we can also handle the situation on operator's side, by removing the empty file if present.

FYI a valid file always contains at least some data, because ovsdb-tool adds a log entry with raft config as the first entry. See:

https://github.com/openvswitch/ovs/blob/56e315937eeb640d5d8f305988d133390445eaee/ovsdb/raft.c#L525

Resolves: https://issues.redhat.com/browse/OSPRH-8117

@openshift-ci openshift-ci bot requested review from abays and viroel July 1, 2024 21:18
Copy link
Contributor

openshift-ci bot commented Jul 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: booxter

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Jul 1, 2024
@booxter
Copy link
Contributor Author

booxter commented Jul 1, 2024

@karelyatin this is the fix body. Do you think we need a kuttl scenario for this? We can probably start the cluster, then echo -n > $DB_FILE, then kill the pod and see if it restarts successfully.

@booxter booxter requested review from slawqo and karelyatin July 1, 2024 21:19
In some situations, e.g. when ovsdb-tool is killed during db file
initialization, the file may end up empty. On next restart, the service
fails to start because the file format is invalid.

Ideally, ovs start scripts would handle this situation gracefully, as
tracked in https://issues.redhat.com/browse/FDP-689

But in the meantime, we can also handle the situation on operator's
side, by removing the empty file if present.

FYI a valid file always contains at least some data, because ovsdb-tool
adds a log entry with raft config as the first entry. See:

https://github.com/openvswitch/ovs/blob/56e315937eeb640d5d8f305988d133390445eaee/ovsdb/raft.c#L525

Resolves: https://issues.redhat.com/browse/OSPRH-8117
@booxter
Copy link
Contributor Author

booxter commented Jul 2, 2024

 error: build error: Failed to push image: trying to reuse blob sha256:e394ea8406c7ed85ddc862f882ec77dcfd3863de3cb90fd8006238d521d22d44 at destination: pinging container registry quay.rdoproject.org: Get "https://quay.rdoproject.org/v2/": dial tcp 38.129.56.158:443: i/o timeout 

@booxter
Copy link
Contributor Author

booxter commented Jul 2, 2024

/test ovn-operator-build-deploy-kuttl

2 similar comments
@booxter
Copy link
Contributor Author

booxter commented Jul 2, 2024

/test ovn-operator-build-deploy-kuttl

@karelyatin
Copy link
Contributor

/test ovn-operator-build-deploy-kuttl

@karelyatin
Copy link
Contributor

@karelyatin this is the fix body. Do you think we need a kuttl scenario for this? We can probably start the cluster, then echo -n > $DB_FILE, then kill the pod and see if it restarts successfully.

Thanks Ihar, doesn't looks we need any kuttl test just for this.

@karelyatin
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 2, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 598886a into openstack-k8s-operators:main Jul 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants