search

NBISweden / LocalEGA

Please go to to https://github.com/EGA-archive/LocalEGA instead
Apache License 2.0
4 stars 1 forks source link

Docker Swarm cluster provisioning #332

Closed dtitov closed 6 years ago

dtitov commented 6 years ago

Readme updated with the instructions, the video is available here: https://www.youtube.com/watch?v=8hvXxqW8uP0

silverdaz commented 6 years ago

I'm not a reviewer, but I looked at the video and I could write down my 2 cents here.

This PR seems to deploy an Openstack cluster, and in it, it deploys a Docker Swarm cluster, to start the LocalEGA containers. By all means, it's good work, you make it work (albeit the docker inside openstack is not my favorite thing) but I'm not understanding why this is relevant for this repo. It seems that it is only related to the Norwegian settings. I don't understand its place in the big picture. Can't you just put that in your repos, and do you really need to push it here?

Moreover, I spotted something in the design choice that makes me ache: At 12:20, you say "There is a constraint: We must make the ingestion workers and the inbox on the same node because they use the same volume." I do not believe it is a constraint at all, so is it so, at the moment, just to get things running, and you plan on updating that part later on, or you do mean that it is a hard constraint? In the latter case, I think it's a flaw in the design: It should not be necessary to put the ingestion next to the inbox. They are, for example, not on the same network. The inbox is accessible from outside, the ingestion are not (they are on some internal network, and routing tables, or network filtering rules, prevent access from the inbox to any other nodes).

The network configuration is a complex topic that we can keep for another Sprint. This is not part of that PR, I see it, but before merging, I think it should.

silverdaz commented 6 years ago

Which issue does this PR solve? Click the "Connect with an issue" button, please, and select one.

dtitov commented 6 years ago

It seems that it is only related to the Norwegian settings. I don't understand its place in the big picture. Can't you just put that in your repos, and do you really need to push it here?

As well as OpenShift deployment is only related to Finnish settings and Kubernetes deployment is only related to Swedish settings. I'm not against of splitting deployments folder into separate repos, but it should be the consolidated approach and general decision for all of these deployments. And now I feel like you are just proposing to put out from the repo the Swarm deployment, which I don't agree with. In my mind either every deployment is here, or all the deployments are distributed across repos. We can discuss it with the team later on.

I do not believe it is a constraint at all, so is it so, at the moment, just to get things running, and you plan on updating that part later on, or you do mean that it is a hard constraint?

It's the constraint for the current setup. In the future there should be added some mechanisms to share volumes across the Swarm like NFS, for instance. But it's out of the scope of this PR. So yes, you are right, this limitation is temporary.

Which issue does this PR solve? Click the "Connect with an issue" button, please, and select one.

Done.

silverdaz commented 6 years ago

And now I feel like you are just proposing to put out from the repo the Swarm deployment

Nope, that's not the case. I'm suggesting: All specific settings for each country in their respective repo. (and I could agree on having a pointer inside that repo, if it is reaaaaaaally necessary, but I fail to see why it would be necessary.) It turns out the NBIS repo was the swedish deployments too, but that can be moved to a separate repo in no breeze.

I just want to keep the docker-compose one inside, for testing when people git-clone the repo. They would even have a fake CentralEGA, and fake broker, some fake scripts to submit to the brokers, just to test. Country deployments should not have any need of having a fake CentralEGA, other than testing.

dtitov commented 6 years ago

Fair enough, I find it quite reasonable. But, again, we should have a separate discussion on it and it shouldnt affect this PR anyway as Swarm deployment is already in this repo and it’s just an extension/enhansment.