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

MCLOUD-5859: Fix for conflicting volumes - #43 #168

Merged
merged 15 commits into from Mar 23, 2020

Conversation

G-Arvind
Copy link
Contributor

@G-Arvind G-Arvind commented Mar 12, 2020

Description

https://jira.corp.magento.com/browse/MCLOUD-5859
Currently, the Magento Cloud Docker uses magento-sync volume and other volumes with no prefix for project. This causes collision and conflict when trying to switch projects.

To solve this, it should use a prefix for all volumes, this should probably be from .magento.app.yaml or .magento.docker.yaml and is by default set as follows:name: mymagento

This would then create volumes as mymagento-magento-sync and mymagento-magento-db. This allows projects to set this name and have it generate volumes that can persist.

Fixed Issues (if relevant)

  1. MCLOUD-5859: Give us the ability to have multiple projects with different volumes #43: Give us the ability to have multiple projects with different volumes (Prefixed the volume name with the name that is provided in .magento.app.yaml or .magent.docker.yaml)
  2. Made Name as a required field. (So that the volumes can prefixed with the given name)

Manual testing scenarios

  1. Add .magento.app.yaml or .magento.docker.yaml (with name field included).
  2. Run ./vendor/bin/ece-docker build:compose --mode="developer".
  3. docker-compose.yaml will be generated.
  4. Verify that volume names are prefixed with Name given in .magento.app.yaml or .magento.docker.yaml
  5. Verify Multilple projects with different volumes are working.
  • Mount a project using docker environment
  • After that, try to create another one using the same architecture
  1. Both projects should work with no error
  2. if no Name is provided in .magento.app.yaml or .magento.docker.yaml an exception will be thrown "Name could not be parsed."

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages

releasenotes
Fixed an issue that can cause volume conflicts when using multiple Docker environments.

Copy link
Member

@bbatsche bbatsche left a comment

Choose a reason for hiding this comment

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

Simple whitespace fix needed

src/Compose/ProductionBuilder.php Outdated Show resolved Hide resolved
src/Compose/ProductionBuilder.php Outdated Show resolved Hide resolved
bbatsche
bbatsche previously approved these changes Mar 13, 2020
bbatsche
bbatsche previously approved these changes Mar 13, 2020
@@ -105,9 +107,9 @@ public function build(Config $config): Manager
}

$manager->setVolumes([
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better if the concatenation of the prefix and volumes fit inside the "Manager" object
Thus, you would not need to add a prefix every time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started to work on your comment but I have a few questions, this change introduces 4 if statements inside setVolumes() alone is this fine?, other than this should addService() and update service() present in Manager Class be able to prefix the volumes, or directly prefixing them in DeveloperBulider /ProductionBuilder is enough(Current Approach)?

Example snippet: like this 4cases should be done inside setVolumes()

if(array_key_exists( "magento-db", $volumes)) 
{
        $oldkey= "magento-db";
        $keys = array_keys($volumes);
        $keys[array_search($oldkey, $keys)] = $this->config->getName().'-'.$oldkey;
        $volumes= array_combine($keys, $volumes);
        $extConfig["volumes"]=$volumes;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The main idea of ​​my proposal was that prefix concatenation be implemented in the Manager class
After that, in the Manager, we can add a method to get the mounts for a specific service.

Now, in order to expedite the delivery, I suggest for now just updating the branch without rework.

@G-Arvind G-Arvind requested a review from bbatsche March 19, 2020 07:15
@andriyShevtsov
Copy link
Contributor

I suppose we should automatically change port. Because port 80 will be occupied in case of few docker projects

@andriyShevtsov
Copy link
Contributor

The same for TLS (port 443)

@aepod
Copy link
Contributor

aepod commented Mar 23, 2020

This ticket only covers conflicting volumes. This will still fix the issue with multiple projects on the same VM, as the volumes being names the same means DATA conflicts if you try to switch projects.

I do not think Magento Cloud Docker can support multiple projects running at the same time, without some drastic changes to the containers. This ticket was just to allow multiple projects to not conflict, which they do now.

@andriyShevtsov
Copy link
Contributor

Test:

  1. Have 2 projects
  2. Name prefix the same
  3. Start first project
  4. Ensure all works
  5. Stop project with docker-compose stop
  6. Complete steps 3-4 for 2nd project

Approved

@shiftedreality shiftedreality merged commit 50af85a into magento:develop Mar 23, 2020
Pull Request Progress automation moved this from Ready for Review to Done Mar 23, 2020
@YPyltiai YPyltiai added the community PR/issue origin label Apr 22, 2020
@mveeramneni mveeramneni changed the title Fix for conflicting volumes - #43 MCLOUD-5859: Fix for conflicting volumes - #43 Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet