Avoid dynamic-plugins writing conflicts #2285

gazarenkov · 2025-01-29T20:25:09Z

Description

It introduces quasi locking mechanism to make sure install-dynamic-plugins script does not write to dynamic-plugins-root directory concurrently to shared persistence volume.

Which issue(s) does this PR fix

https://issues.redhat.com/browse/RHIDP-5732

PR acceptance criteria

Please make sure that the following steps are complete:

GitHub Actions are completed and successful
Unit Tests are updated and passing
E2E Tests are updated and passing
Documentation is updated if necessary (requirement for new features)
Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

To test on Operator:

Configure PVC for dynamic-plugins-root volume in profile/rhdh/default-config/deployment.yaml

  volumes:
        - name: dynamic-plugins-root
          persistentVolumeClaim:
            claimName: dynamic-plugins-root
#        - ephemeral:
#          name: dynamic-plugins-root

and deploy Operator with it
2. Create PVC like

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: dynamic-plugins-root
spec:
  accessModes:
    - ReadWriteOnce
  volumeMode: Filesystem
  resources:
    requests:
      storage: 2Gi

Create multi replica Backstage CR like:

apiVersion: rhdh.redhat.com/v1alpha3
kind: Backstage
metadata:
  name: bs2
spec:
  deployment:
    patch:
      spec:
        replicas: 3
        template:
          spec:
            containers:
              - name: backstage-backend
                image: quay.io/gazarenk/rhdh:latest
            initContainers:
              - name: install-dynamic-plugins
                image: quay.io/gazarenk/rhdh:latest
            volumes:
              - persistentVolumeClaim:
                  claimName: dynamic-plugins-root

Make sure it is deployed successfully and
kubectl logs backstage-bs2-XXXXXXXXX -c install-dynamic-plugins -n on one Pod should start with:
`
======= Created lock file: /dynamic-plugins-root/install-dynamic-plugins.lock

`on 2 others:

======= Waiting for lock release...
======= Lock released.
======= Created lock file: /dynamic-plugins-root/install-dynamic-plugins.lock

openshift-ci · 2025-01-29T20:25:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign coreydaley for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

github-actions · 2025-01-29T20:25:23Z

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Avoid dynamic-plugins writing conflicts". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat
 - fix
 - chore
 - docs
 - style
 - refactor
 - perf
 - test
 - revert

github-actions · 2025-01-29T20:57:30Z

The image is available at:

durandom

the scenario works if all goes well. But in case of a graceful termination the lock would not be removed - see inline comments.

also, if the pod is killed via SIGKILL or OOM, then the lock remains. That means something is wrong anyway and has to be adjusted. But I think we should provide a command to remove the lock.

oc exec deploy/backstage-rhdh -c install-dynamic-plugins -- rm //dynamic-plugins-root/install-dynamic-plugins.lock

Maybe put that into the docs/dynamic-plugins/installing-plugins.md section and get it into our docs?

durandom · 2025-01-30T09:26:00Z

docker/install-dynamic-plugins.py

    dynamicPluginsRoot = sys.argv[1]
+
+    lock_file_path = os.path.join(dynamicPluginsRoot, 'install-dynamic-plugins.lock')
+    atexit.register(remove_lock, lock_file_path)


I wonder if that also handles a SIGTERM, which is send by k8s to gracefully terminate the pod. To handle that scenario you could add this

import signal # Register signal handlers signal.signal(signal.SIGTERM, remove_lock) # Kubernetes graceful shutdown

Good catch, thanks! Fixed

kadel · 2025-01-30T11:19:42Z

the scenario works if all goes well. But in case of a graceful termination the lock would not be removed - see inline comments.

also, if the pod is killed via SIGKILL or OOM, then the lock remains. That means something is wrong anyway and has to be adjusted. But I think we should provide a command to remove the lock.

Would it make sense to have a kind of time-out on the lock? For example, if the lock is older than 1 hour, just delete the old lock and assume you script is good to go.

durandom · 2025-01-30T14:28:20Z

Would it make sense to have a kind of time-out on the lock? For example, if the lock is older than 1 hour, just delete the old lock and assume you script is good to go.

Maybe, but isn't in that case something broken already and would require manual intervention?

github-actions · 2025-01-30T19:00:02Z

The image is available at:

gazarenkov · 2025-01-30T19:27:18Z

Would it make sense to have a kind of time-out on the lock? For example, if the lock is older than 1 hour, just delete the old lock and assume you script is good to go.

Maybe, but isn't in that case something broken already and would require manual intervention?

Doc added, in principle can add time-out based releasing too... I thought about it as well but not sure. @kadel Are you insisting on it? :)

github-actions · 2025-01-30T20:01:14Z

The image is available at:

rm3l

Few comments below..

rm3l · 2025-02-01T14:05:31Z

the scenario works if all goes well. But in case of a graceful termination the lock would not be removed - see inline comments.

also, if the pod is killed via SIGKILL or OOM, then the lock remains. That means something is wrong anyway and has to be adjusted. But I think we should provide a command to remove the lock.
oc exec deploy/backstage-rhdh -c install-dynamic-plugins -- rm //dynamic-plugins-root/install-dynamic-plugins.lock
Maybe put that into the docs/dynamic-plugins/installing-plugins.md section and get it into our docs?

Would manual intervention be acceptable to production users?

rm3l · 2025-02-01T14:06:46Z

docker/install-dynamic-plugins.py

+def create_lock(lock_file_path):
+    while True:
+      try:
+        with open(lock_file_path, 'x'):


There could be a risk that multiple replicas call this at the same time, no?

Avoid dynamic-plugins writing conflicts

c419211

gazarenkov requested review from kadel, durandom and rm3l January 29, 2025 20:25

openshift-ci bot requested review from dzemanov and its-mitesh-kumar January 29, 2025 20:25

durandom requested changes Jan 30, 2025

View reviewed changes

add SIGTERM handling

f9a7421

add manual lock remove doc

ad98a08

gazarenkov requested a review from durandom February 1, 2025 09:28

rm3l reviewed Feb 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid dynamic-plugins writing conflicts #2285

Avoid dynamic-plugins writing conflicts #2285

gazarenkov commented Jan 29, 2025 •

edited

Loading

openshift-ci bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025

durandom left a comment

durandom Jan 30, 2025

gazarenkov Jan 30, 2025

kadel commented Jan 30, 2025

durandom commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

gazarenkov commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

rm3l left a comment •

edited

Loading

rm3l commented Feb 1, 2025

rm3l Feb 1, 2025

Avoid dynamic-plugins writing conflicts #2285

Are you sure you want to change the base?

Avoid dynamic-plugins writing conflicts #2285

Conversation

gazarenkov commented Jan 29, 2025 • edited Loading

Description

Which issue(s) does this PR fix

PR acceptance criteria

How to test changes / Special notes to the reviewer

openshift-ci bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025 • edited Loading

github-actions bot commented Jan 29, 2025

durandom left a comment

Choose a reason for hiding this comment

durandom Jan 30, 2025

Choose a reason for hiding this comment

gazarenkov Jan 30, 2025

Choose a reason for hiding this comment

kadel commented Jan 30, 2025

durandom commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

gazarenkov commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

rm3l left a comment • edited Loading

Choose a reason for hiding this comment

rm3l commented Feb 1, 2025

rm3l Feb 1, 2025

Choose a reason for hiding this comment

gazarenkov commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

rm3l left a comment •

edited

Loading