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

Fix deadlock and DELETE/UPDATE unindexed datasets #3620

Merged
merged 5 commits into from
May 6, 2024

Conversation

dbutenhof
Copy link
Member

PBENCH-1328

A bug revealed another bug:

  1. POST/DELETE /datasets/{id} fails when the dataset isn't indexed; with
    the ability to disable server indexing entirely and to remove dataset indexed
    data, this is undesirable. As long as the dataset doesn't have a WORKING ops
    status, we should be able to update or delete.
  2. When the operation fails, the status is already set to WORKING, and this
    was not corrected on exit, leaving the dataset locked perpetually.

Fixed in two phases: first to correct the handling of errors so that we'll
always unlock on failure; second to allow deletion of unindexed datasets. (For
PR review, these phases are available in separate commits.)

PBENCH-1328

A bug revealed another bug:

1. `POST`/`DELETE` `/datasets/{id}` fails when the dataset isn't indexed; with
the ability to disable server indexing entirely and to remove dataset indexed
data, this is undesirable. As long as the dataset doesn't have a `WORKING` ops
status, we should be able to update or delete.
2. When the operation fails, the status is already set to `WORKING`, and this
was not corrected on exit, leaving the dataset locked perpetually.

Fixed in two phases: first to correct the handling of errors so that we'll
always unlock on failure; second to allow deletion of unindexed datasets. (For
PR review, these phases are available in separate commits.)
@dbutenhof dbutenhof added bug Server Code Infrastructure API Of and relating to application programming interfaces to services and functions Database Operations Related to operation and monitoring of a service labels May 4, 2024
@dbutenhof dbutenhof requested a review from webbnh May 4, 2024 02:39
@dbutenhof dbutenhof self-assigned this May 4, 2024
@dbutenhof
Copy link
Member Author

I was hoping to do some testing on a runlocal but for some reason I can't quite determine, the Keycloak container is working as expected, and the setup times out. If the CI runs, that'll be a useful diagnostic.

(And I probably should have posted as a Draft, because I realize that my second commit wiped out explicit testing of the WORKING state fix, which will now require some mocking to set up; but I'm also not sure I want to be spending more time on this. We'll see if I get bored over the weekend.)

@dbutenhof
Copy link
Member Author

dbutenhof commented May 4, 2024

Ah: so I think I found our problem:

$ openssl x509 -enddate -noout -in server/pbenchinacan/etc/pki/tls/certs/pbench_CA.crt 
notAfter=Apr 25 21:37:54 2024 GMT

@webbnh Do you remember exactly what you did originally to generate the CA? (It should probably be recorded in a README.md in the pbenchinacan directory.)

Nevermind: I figured it out. I'm going to post a new commit with a new CA, but I'm also going to need to fix some stuff so I'm changing to draft.

@dbutenhof dbutenhof marked this pull request as draft May 4, 2024 13:17
Correct functional index test for index-free operation; add an explicit unit
test to show that `WORKING` state is changed to `FAILED` on failure.
@dbutenhof dbutenhof marked this pull request as ready for review May 4, 2024 19:27
webbnh
webbnh previously approved these changes May 6, 2024
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks good. I have one pointed question, and a few suggestions which probably aren't worth pursuing...unless you want to. 🙃

server/pbenchinacan/load_keycloak.sh Show resolved Hide resolved
lib/pbench/server/api/resources/__init__.py Show resolved Hide resolved
lib/pbench/server/api/resources/__init__.py Show resolved Hide resolved
lib/pbench/server/api/resources/__init__.py Show resolved Hide resolved
lib/pbench/server/api/resources/__init__.py Show resolved Hide resolved
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

On the previous pass, I neglected to review all of the commits.... I found a few typo-things that you might want to fix.

lib/pbench/test/functional/server/test_datasets.py Outdated Show resolved Hide resolved
lib/pbench/test/functional/server/test_datasets.py Outdated Show resolved Hide resolved
lib/pbench/test/functional/server/test_datasets.py Outdated Show resolved Hide resolved
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

👍

@dbutenhof dbutenhof merged commit 81eb077 into distributed-system-analysis:main May 6, 2024
4 checks passed
@dbutenhof dbutenhof deleted the statefix branch May 6, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Of and relating to application programming interfaces to services and functions bug Code Infrastructure Database Operations Related to operation and monitoring of a service Server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants