fix: deadlock issue #9728

amandavialva01 · 2024-07-25T17:51:53Z

Ticket

DET-10441

Description

Fix deadlocking issue with creating or verifying existence of a kubernetes namespace

Test Plan

Checklist

Changes have been manually QA'd
New features have been approved by the corresponding PM
User-facing API changes have the "User-facing API Change" label
Release notes have been added as a separate file under docs/release-notes/
See Release Note for details.
Licenses have been included for new code which was copied and/or modified from any external code

netlify · 2024-07-25T17:52:11Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`4a2edd9`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66a791b2cbb4d90008479066

codecov · 2024-07-25T17:52:35Z

Codecov Report

Attention: Patch coverage is 70.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 53.44%. Comparing base (202ab62) to head (4a2edd9).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9728      +/-   ##
==========================================
- Coverage   53.44%   53.44%   -0.01%     
==========================================
  Files        1257     1257              
  Lines      154350   154359       +9     
  Branches     3298     3298              
==========================================
  Hits        82500    82500              
- Misses      71700    71709       +9     
  Partials      150      150

Flag	Coverage Δ
backend	`44.87% <70.00%> (-0.01%)`	⬇️
harness	`72.69% <ø> (ø)`
web	`51.92% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/internal/rm/kubernetesrm/jobs.go	`71.40% <70.00%> (+0.11%)`	⬆️

... and 3 files with indirect coverage changes

davidfluck-hpe · 2024-07-25T21:24:56Z

Were you able to reproduce the issue?

amandavialva01 · 2024-07-25T23:10:11Z

Were you able to reproduce the issue?

This PR seems to resolve the issue, which I have tested by helm installing Determined with this commit SHA onto a remote cluster and running the workflow that does produce the deadlock when a SHA from main is used with the helm install instead.
Working on end-to-end tests that I'll add to this PR tomorrow

davidfluck-hpe · 2024-07-25T23:42:12Z

Fantastic, thank you! What is the workflow that reproduces the issue?

amandavialva01 · 2024-07-26T00:07:35Z

Fantastic, thank you! What is the workflow that reproduces the issue?

If you're running a remote GKE cluster: Create a workspace bound to an auto-created namespace and start a task (I used a JL instance) in that workspace. Then, delete the master deployment pod and wait for the k8s API server to recreate it. Once the pod is back up and running, open the "Edit Workspace" modal in the webUI and save the workspace as is. You'll see the loading sign, and then nothing will load after that 🙈😂

I image that a CLI command such as det w bindings set workspace <workspace_name> --auto-create-namespace would cause the same issue as opening the "Edit" modal and saving the workspace on the UI, because when master restarts, we are no longer correctly tracking the namespaces that we have informers for, so any API call that invokes createNamespace causes the creation of duplicate informers for the same namespace, resulting in a deadlock.

master/internal/rm/kubernetesrm/jobs.go

kkunapuli

LGTM!

I just have a question about when we set j.namespacesWithInformers[namespace] = true (line 260).

kkunapuli

LGTM! Nice work.

kkunapuli · 2024-07-29T13:35:15Z

e2e_tests/tests/cluster/test_wksp_nmsp_concurrency.py

+# function that only gets called when a job is running in the namespace that we want to bind to the
+# workspace. Verify that we don't run into this deadlock when triggering multiple calls to the
+# API handler that binds a workspace to an auto-created namespace, as this request can trigger the
+# deadlock if the namespace (or verify the existence of for the first time) is running a job.


I love this explanation! Great comment and great test.

kkunapuli · 2024-07-29T13:39:30Z

.circleci/devcluster/single-k8s.devcluster.yaml

@@ -35,9 +35,3 @@ stages:
          kubeconfig_path: ~/.kube/config
          determined_master_ip: $DOCKER_LOCALHOST
          determined_master_port: 8080
-          internal_task_gateway:


Out of curiosity, why is this deleted? Was it a bit of cleanup discovered during testing?

Yeaa, I think we only need to use a gateway for mutliRM to make tasks that create resources in the remote cluster (the cluster tied to the additional RM) accessible to the determined master, so single RM clusters don't need that config!

(cherry picked from commit 9dc0afa)

fix: deadlock issue

1b0cba1

amandavialva01 requested a review from a team as a code owner July 25, 2024 17:51

amandavialva01 requested a review from carolinaecalderon July 25, 2024 17:51

cla-bot bot added the cla-signed label Jul 25, 2024

carolinaecalderon added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Jul 25, 2024

kkunapuli reviewed Jul 26, 2024

View reviewed changes

master/internal/rm/kubernetesrm/jobs.go Outdated Show resolved Hide resolved

kkunapuli reviewed Jul 26, 2024

View reviewed changes

master/internal/rm/kubernetesrm/jobs.go Outdated Show resolved Hide resolved

kkunapuli approved these changes Jul 26, 2024

View reviewed changes

amandavialva01 requested a review from a team as a code owner July 26, 2024 16:11

add test and address comments

0832e0e

amandavialva01 force-pushed the amanda/fixDeadlockIssue branch from e2a8dc2 to 0832e0e Compare July 26, 2024 16:59

adjust test config

0e68ba7

amandavialva01 requested a review from a team as a code owner July 26, 2024 20:03

amandavialva01 requested a review from djanicekpach July 26, 2024 20:03

testing changes

06fa978

amandavialva01 requested a review from a team as a code owner July 26, 2024 22:07

amandavialva01 requested a review from jgongd July 26, 2024 22:07

amandavialva01 force-pushed the amanda/fixDeadlockIssue branch 2 times, most recently from a6ff0b5 to 7e0cdb0 Compare July 26, 2024 22:32

fix lint

f50f82f

amandavialva01 force-pushed the amanda/fixDeadlockIssue branch from 7e0cdb0 to f50f82f Compare July 26, 2024 22:55

amandavialva01 removed request for a team and djanicekpach July 27, 2024 01:39

amandavialva01 removed the request for review from jgongd July 27, 2024 01:39

amandavialva01 force-pushed the amanda/fixDeadlockIssue branch 3 times, most recently from a325786 to a90a6a1 Compare July 29, 2024 12:12

create and pass down lock indicator and cleanup tests

ea877a2

amandavialva01 force-pushed the amanda/fixDeadlockIssue branch 2 times, most recently from 1bebba5 to ef3c3d1 Compare July 29, 2024 12:44

remove gateway service from singleRM cluster config

4a2edd9

amandavialva01 force-pushed the amanda/fixDeadlockIssue branch from ef3c3d1 to 4a2edd9 Compare July 29, 2024 12:57

amandavialva01 requested a review from kkunapuli July 29, 2024 13:28

kkunapuli approved these changes Jul 29, 2024

View reviewed changes

amandavialva01 merged commit 9dc0afa into main Jul 29, 2024
77 of 95 checks passed

amandavialva01 deleted the amanda/fixDeadlockIssue branch July 29, 2024 14:15

github-actions bot pushed a commit that referenced this pull request Jul 29, 2024

fix: deadlock issue (#9728)

27b8dbd

(cherry picked from commit 9dc0afa)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: deadlock issue #9728

fix: deadlock issue #9728

amandavialva01 commented Jul 25, 2024 •

edited

Loading

netlify bot commented Jul 25, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading

davidfluck-hpe commented Jul 25, 2024

amandavialva01 commented Jul 25, 2024 •

edited

Loading

davidfluck-hpe commented Jul 25, 2024

amandavialva01 commented Jul 26, 2024 •

edited

Loading

kkunapuli left a comment

kkunapuli left a comment

kkunapuli Jul 29, 2024

kkunapuli Jul 29, 2024

amandavialva01 Jul 29, 2024

fix: deadlock issue #9728

fix: deadlock issue #9728

Conversation

amandavialva01 commented Jul 25, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Jul 25, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

codecov bot commented Jul 25, 2024 • edited Loading

Codecov Report

davidfluck-hpe commented Jul 25, 2024

amandavialva01 commented Jul 25, 2024 • edited Loading

davidfluck-hpe commented Jul 25, 2024

amandavialva01 commented Jul 26, 2024 • edited Loading

kkunapuli left a comment

Choose a reason for hiding this comment

kkunapuli left a comment

Choose a reason for hiding this comment

kkunapuli Jul 29, 2024

Choose a reason for hiding this comment

kkunapuli Jul 29, 2024

Choose a reason for hiding this comment

amandavialva01 Jul 29, 2024

Choose a reason for hiding this comment

amandavialva01 commented Jul 25, 2024 •

edited

Loading

netlify bot commented Jul 25, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading

amandavialva01 commented Jul 25, 2024 •

edited

Loading

amandavialva01 commented Jul 26, 2024 •

edited

Loading