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.

Sign up for GitHub

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

Jump to bottom

Fix graphite crash by upgrading from 0.9.15 -> 1.0.2 #30166

Merged
merged 1 commit into from
Oct 18, 2017
Merged

Fix graphite crash by upgrading from 0.9.15 -> 1.0.2 #30166

merged 1 commit into from
Oct 18, 2017
+144 −27

Conversation

basvandijk
Copy link
Member

@basvandijk basvandijk commented Oct 6, 2017 •
edited
Loading

Please don't merge yet since this is Work In Progress!

This is an attempt to solve #29961 by upgrading graphite to its latest version.

One problem that remains is that graphite_web doesn't serve its static files like .css and .js. Who can help me fix that?

You can play with graphite in a virtual machine that you can start using:

$(nix-build nixos/tests/graphite.nix -A driver --no-out-link)/bin/nixos-run-vms

This will boot a virtual machine with graphite enabled. The machine will start an X server with xmonad as your window manager. Firefox is installed so that it's easy to see if graphite_web is operating correctly.

  • Press Alt-Shift-Enter to open a terminal.
  • Run firefox
  • Open the network inspector using Ctrl-Shift-C
  • Select the "Network" tab
  • Open graphite_web on http://localhost:8080
  • Observe that all static files return 404s

@basvandijk basvandijk requested a review from FRidh as a code owner October 6, 2017 19:14
@lsix
Copy link
Member

lsix commented Oct 7, 2017

Hi,

I will have a look ASAP, but probably not before the beginning of next week.

I have not looked at this packaging yet, but usually here is what is done for django apps:

  • a site is usually composed of multiple [django] applications, each of them having static files stored along with them
  • before deploying, all those static files should be collected into one common static folder (usually using django-manage.py collectstatic) for the web server to be able to find them. It knows how to load applications since they are python modules available to it, but the static files themselves are expected to be stored all together.

I guess that here either this step is missing or the server does not know where to find the collected files.

See here for some general documentation: https://docs.djangoproject.com/en/1.8/howto/static-files/deployment/

Again, I have not looked at it yet, and have never deployed graphite. This is basically what I will look at first.

@basvandijk
Copy link
Member Author

basvandijk commented Oct 7, 2017 •
edited
Loading

@lsix these pointers are helpful, thanks. I just tried setting STATIC_ROOT and calling
django-admin.py collectstatic but I have no luck so far getting static files to serve.

Ideally we don't need to call collectstatic since we're serving a single django app. Ideally we serve the static files directly from the nix store by setting
STATIC_ROOT = '${pkgs.python27Packages.graphite_web}/webapp/content'.

@basvandijk
Copy link
Member Author

I fixed it.

I'm going to squash my commits into one then this PR could be merged.

@basvandijk basvandijk requested a review from edolstra as a code owner October 8, 2017 01:02
@basvandijk
graphite: 0.9.15 -> 1.0.2

Verified

This commit was signed with the committer’s verified signature.
Okeanos Nikolas Grottendieck
GPG key ID: 44A3563E46261FBA
Verified
Learn about vigilant mode
5b8ff5e 
Fixes: NixOS#29961

Also added the option:

  services.graphite.web.extraConfig

for configuring graphite_web.
@basvandijk basvandijk changed the title [WIP] Fix graphite crash by upgrading from 0.9.15 -> 1.0.2 Fix graphite crash by upgrading from 0.9.15 -> 1.0.2 Oct 8, 2017
@basvandijk
Copy link
Member Author

The PR is now ready to be reviewed and merged.

@FRidh could you remove the work in progress label please?

@basvandijk basvandijk mentioned this pull request Oct 10, 2017
Merged
@peti peti merged commit 757a759 into NixOS:master Oct 18, 2017
@makefu
Copy link
Contributor

makefu commented Oct 18, 2017

@basvandijk I just wanted to thank you for your efforts. Finally i can upgrade my machines to the latest stable release, broken graphite was a major blocker for me. 👍

@basvandijk
Copy link
Member Author

@makefu, my pleasure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh

@edolstra edolstra

Assignees
No one assigned
Labels
2.status: work-in-progress This PR isn't done 6.topic: python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@basvandijk @lsix @makefu @peti @FRidh