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

Differentiate between PID and unique cluster ID #414

Merged
merged 14 commits into from Feb 14, 2020
Merged

Differentiate between PID and unique cluster ID #414

merged 14 commits into from Feb 14, 2020

Conversation

jmcvetta
Copy link
Contributor

@jmcvetta jmcvetta commented Feb 13, 2020

This PR enables django-q to run correctly in namespaced Docker containers, by differentiating between a randomly assigned unique cluster ID and the cluster's PID.

Motivation

Before this PR, django-q used the PID of the ./manage.py qcluster command as the ID of the cluster. This caused the system to become confused when multiple clusters were running inside namespaced Docker containers. Because all the qcluster processes had the same PID.

(Namespaced Docker containers have their PID space separate from the PID space of the host OS. So the entrypoint process always has PID 1.)

Implementation

This PR separates a Cluster.pid field from a Cluster.cluster_id field. The latter is populated by a random UUID at cluster initialization. Other methods are adapted to depend on cluster_id (which is guaranteed to be unique) rather than pid (which is very likely to be non-unique in certain deployment schemes).

@codecov-io
Copy link

codecov-io commented Feb 13, 2020

Codecov Report

Merging #414 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
+ Coverage   90.63%   90.66%   +0.03%     
==========================================
  Files          43       43              
  Lines        2914     2926      +12     
==========================================
+ Hits         2641     2653      +12     
  Misses        273      273              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0acf2ec...93fab1e. Read the comment docs.

@@ -10,15 +11,16 @@

@pytest.mark.django_db
def test_monitor(monkeypatch):
assert Stat.get(0).sentinel == 0
cluster_id = uuid.uuid4()
assert Stat.get(pid=0, cluster_id=4).sentinel == 0
Copy link
Owner

Choose a reason for hiding this comment

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

cluster_id=4 is this a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, totally, it's a typo. Should be cluster_id=cluster_id.

setup.py Outdated
@@ -26,7 +26,7 @@ def run(self):

setup(
name='django-q',
version='1.1.0',
version='1.2.0',
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to keep the versioning out of the PR's. It's never clear when it will be released, there could be some package updates in between for example.

@Koed00
Copy link
Owner

Koed00 commented Feb 13, 2020

Great work on this btw. Especially since this is one of those things that just doesn't come up in the way I use it mysefl.

@jmcvetta jmcvetta requested a review from Koed00 February 13, 2020 11:26
@jmcvetta
Copy link
Contributor Author

PR updated per your requests. Thanks for the quick response - and thanks for writing django-q!

@Koed00 Koed00 merged commit e3a4699 into Koed00:master Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants