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

Implement the API version of the Docker containers interface #5861

Merged
merged 32 commits into from Apr 18, 2018

Conversation

natefoo
Copy link
Member

@natefoo natefoo commented Apr 5, 2018

Initially when I'd started work on adding Docker Swarm support for GIEs, I was just augmenting the existing (command line) docker calls in the GIE module. Before long, the this grew in to creating a library for interacting with containerization systems. I moved the command line work over to the new library and expanded it, adding a generalized parser for the command line's column output, among other things.

Although this worked at the time, the correct route would've been to use the Docker API, but I fell a bit for the sunk cost fallacy and finished things up using the command line. The command line stuff is now incompatible with at least some commands in the latest docker versions.

Soooooo, I've finally implemented the API versions of the Docker and Docker Swarm interfaces and deprecated the command line interfaces, to be removed in the release after 18.05.

This also includes some general stability improvements for the swarm manager, and it touches the tool dependency docker util slightly, as there was already a model for a docker volume in there. Rather than duplicate, I moved that model over to the containers lib.

@natefoo natefoo added this to the 18.05 milestone Apr 5, 2018
@jmchilton
Copy link
Member

This would require adding galaxy.containers to galaxy-lib since that DockerVolume class is used outside the project. I looked over the source and it looks like you are conditionally importing everything that isn't in galaxy-lib or a hard dependency - so that should be fine.

@natefoo
Copy link
Member Author

natefoo commented Apr 5, 2018

Yes, good, my library shall become the standard. ;D

@natefoo natefoo changed the title [WIP] Implement the API version of the Docker containers interface Implement the API version of the Docker containers interface Apr 6, 2018
@natefoo natefoo changed the title Implement the API version of the Docker containers interface [WIP] Implement the API version of the Docker containers interface Apr 6, 2018
@natefoo natefoo changed the title [WIP] Implement the API version of the Docker containers interface Implement the API version of the Docker containers interface Apr 9, 2018
sys.path.remove(os.path.dirname(__file__))
except ValueError:
pass

try:
import galaxy # noqa: F401 this is a test import
except ImportError:
Copy link
Member

Choose a reason for hiding this comment

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

Can you do this without the sys.path edit stuff? I don't think importing things from galaxy-lib should be modifying the path like this - the whole library should be importable without side effects.

Can you keep this script clean, even define a main, but then place something in scripts/ that messes with the sys.path and then just calls out the main here? Like this (https://github.com/galaxyproject/galaxy/blob/dev/scripts/create_db.py) maybe?

@natefoo natefoo changed the title Implement the API version of the Docker containers interface [WIP] Implement the API version of the Docker containers interface Apr 12, 2018
@natefoo
Copy link
Member Author

natefoo commented Apr 12, 2018

@jmchilton how about e651b34?

Unrelated, back in WIP due to the DockerAPIClient class still not working as intended: failover doesn't work properly because bound APIClient instancemethods are passed around in the handler staticmethod, so making new APIClients in DockerAPIClient does nothing for a single call. A class var with the APIClient instance and then passing the method name to the handler rather than the method itself might work.

class vars, since we only ever want callers to use one APIClient.
better job of catching them in the application, but this is good enough
for the moment.
@natefoo natefoo changed the title [WIP] Implement the API version of the Docker containers interface Implement the API version of the Docker containers interface Apr 13, 2018
@jmchilton
Copy link
Member

@jmchilton how about e651b34?

@natefoo If you prefer that approach that is fine with me. Seems like the script might be useful as a stand-alone thing installable through pip if we placed it cleanly in galaxy-lib but if that isn't a concern for you I'm fine.

@natefoo
Copy link
Member Author

natefoo commented Apr 18, 2018

I think it's useful, but probably nobody but me is going to use it...

I really am surprised something doesn't already exist for this purpose and I expect something better to come along at some point.

@jmchilton jmchilton merged commit 921fde4 into galaxyproject:dev Apr 18, 2018
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.

None yet

2 participants