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

Added Neo4j IE #2610

Merged
merged 6 commits into from
Jul 27, 2016
Merged

Added Neo4j IE #2610

merged 6 commits into from
Jul 27, 2016

Conversation

thobalose
Copy link
Contributor

@thobalose thobalose commented Jul 13, 2016

A Neo4j GIE based on a modified version of the Neo4j:2.3.3 image which takes a neostore datatype generated by this tool allowing users to explore a Neo4j Graph database.


# Additional arguments that are passed to the `docker run` command.
# NEO4J_UID=1371, NEO4j_GIG=999
command_inject = --sig-proxy=true -e DEBUG=false -e DEFAULT_CONTAINER_RUNTIME=120 -e NEO4J_UID=none -e NEO4J_GID=none
Copy link
Member

Choose a reason for hiding this comment

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

According to https://github.com/SANBI-SA/neo4j_galaxy_ie/blob/master/docker-entrypoint.sh#L7 NEO4J_U/GID are required. Maybe we can default to $UID and $GID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erasche ,

The tool used to generate the data type boots up a Neo4j docker container used load data into a Neo4j database, this runs as the galaxy user which is a member of the docker group.

The IE then needs to mount the data directory with rw access for the user to explore the data.

Copy link
Member

Choose a reason for hiding this comment

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

@thobalose

The IE then needs to mount the data directory with rw access for the user to explore the data.

yes. So would it make sense to use NEO4J_UID=$UID NEO4J_GID=$GID as the default, in order to have the default command use the same UID and GID as the galaxy user who launches the container and owns the files which have been mounted in the volume?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will have to take another look at this. What do you get when you $echo $GID ?

Copy link
Member

Choose a reason for hiding this comment

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

[hxr@leda:~]$ echo $GID
1000

when galaxy runs this, it'd be the primary group id of the galaxy user.

Copy link
Contributor Author

@thobalose thobalose Jul 14, 2016

Choose a reason for hiding this comment

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

Added this.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, that is helpful. This way people could at least launch this without changing configuration.

@hexylena
Copy link
Member

So excited to see this, but am still thinking over any implementation concerns.

@thobalose did you consider using an official neo4j image as the base? Was there a rationale against doing that?

@thobalose
Copy link
Contributor Author

The official Neo4j docker images exposes, depending on the version, 2-3 ports and does not automatically terminate.

Additionally, it assumes/writes as root.

@hexylena
Copy link
Member

hexylena commented Jul 13, 2016

@thobalose sure, I meant building on top of that as your FROM image, adding your auto-killing, and port changes.

Pros:

  • It might allow your group to more easily import security updates that upstream makes
  • you can ignore the other ports that are exposed, since you have some configuration that makes it function with just one port.
  • As for reading/writing as root, use you could drop privileges with a USER statement, or with a customized CMD that runs gosu since you have that installed.

Just thinking about future maintenance and updates.

@thobalose
Copy link
Contributor Author

thobalose commented Jul 27, 2016

Hi @erasche

This looks like something Docker is still working on.
Kindly have a look at this.

@hexylena
Copy link
Member

@thobalose yes, but do you actually need to reset them? In the past, I've just ignored the extra volumes, exposed ports with no negative effects.

@thobalose
Copy link
Contributor Author

thobalose commented Jul 27, 2016

@erasche

Do you mean like so?

From there, we can see that ignoring exposed ports adds the built port as an extra one. Thus, having three exposed ports.

# appropriate `apt-get/pip install` statements.
---
-
image: thoba/neo4j_galaxy_ie
Copy link
Member

Choose a reason for hiding this comment

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

Ok, sorry for one more thing. Would it be possible that you could pin a specific tag, like we do in jupyter?

https://github.com/galaxyproject/galaxy/blob/dev/config/plugins/interactive_environments/jupyter/config/allowed_images.yml.sample#L9

This is so if/when we change the GIE API, we can help all of the GIE building groups migrate to a newer version of the API with a new tagged image, and cause fewer "Which image are you using?" issues for end users.

@hexylena
Copy link
Member

@thobalose yes, exactly like that. However that raises a good point, since we started using the -P flag in the docker GIE launcher, all ports are exposed rather than a single one being mapped externally (and all other exposed ports not being exposed thanks to -p's behaviour).

Okay, due to the fact that using -P will chew up more exposed ports than expected and might decrease number of neo4j GIEs that could be run by heavier deployments, your group has made the right technical decision.

Thank you for working through this with me, I appreciate it!

I had one more last minute comment, otherwise I'm 👍 on this, it should be merged.

@galaxybot test this

@hexylena
Copy link
Member

ping @martenson can you get galaxy bot to test this?

@hexylena hexylena merged commit d7a1776 into galaxyproject:dev Jul 27, 2016
@martenson martenson modified the milestones: 16.07, 16.10 Jul 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants