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

Create New CLI command: Generate Varnish VCL file #9199

Closed
vrann opened this issue Apr 11, 2017 · 20 comments
Closed

Create New CLI command: Generate Varnish VCL file #9199

vrann opened this issue Apr 11, 2017 · 20 comments
Assignees
Labels
Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development up for grabs

Comments

@vrann
Copy link
Contributor

vrann commented Apr 11, 2017

Magento supports Varnish caching and is capable of generating Varnish config *.vcl file. However, that only can be done via UI in the admin backend. This process cannot be used by automated deployments and is very cumbersome for the system administrators: file should be saved locally from the browser and uploaded to the varnish server.

User Story

  • As a system administrator, I'd like to have CLI command which generates the VCL file on the machine where Magento is running so that it can be saved directly to the Varnish config directory.
@piotrkwiecinski
Copy link
Contributor

@vrann I can grab this one. Before I start I have few questions:

  • Should we have CLI options for: Access list, Backend host, Backend port?
  • Should we have CLI option for destination path and what would be default current dir?
  • Command will be added to Page Cache module is that correct?

Is the anything I have missed ?

@convenient
Copy link
Contributor

The following may be good for inspiration ¯\(ツ)

Back in the pre-responsive days when we needed varnish to vary by X-UA-Device I've used a script like https://github.com/willemk/varnish-mobiletranslate/blob/master/varnishtranslate.php to generate the VCL file based on https://github.com/serbanghita/Mobile-Detect.

@adragus-inviqa
Copy link
Contributor

adragus-inviqa commented Apr 11, 2017

Can we just have it output a string? You know, like simple things in Unix do.
Then maybe a -o /path/to/file option, for saving into a file that the caller decides?

@erfanimani
Copy link
Contributor

erfanimani commented Apr 11, 2017

FYI, https://github.com/fastly/fastly-magento2 also updates Varnish with the new VCL (from the admin GUI, not sure if there's a command for it). Maybe an idea?

@piotrkwiecinski
Copy link
Contributor

Fastly only supports V4. For sure it's a start. As @adragus-inviqa suggested writing configuration to output by default and having option to save to file sounds reasonable.

@piotrkwiecinski
Copy link
Contributor

I had a quick look at current implementation \Magento\PageCache\Model\Config and host, port, access list come from scopeConfig, so for start it won't be possible to provide them as parameters from CLI.

@vrann
Copy link
Contributor Author

vrann commented Apr 11, 2017

@piotrkwiecinski Great! Here is what I think:

  • The command should repeat the same functionality of the UI: it should accept Access list, backend host, backend port, grace period.
  • You are right regarding current implementation of the \Magento\PageCache\Model\Config, but it should be fixed. Adding the new method which accepts host, port, etc will make this work. Later you can refactor the existing method and pass the configuration retrieved from the scopeConfig to the new method
  • Latest develop branch supports just the Varnish 4 and 5 -- that's what command should support too
  • Generated VCL file should be same as the one generated in the UI

@vrann
Copy link
Contributor Author

vrann commented Apr 11, 2017

Also, regarding output:

  • I like the suggestion to output to stdout by default
  • -o /path/to/file sounds reasonable

Command should be added to the PageCache module.

@vrann
Copy link
Contributor Author

vrann commented Apr 11, 2017

@piotrkwiecinski can you please assign issue to yourself and move to the "In Progress" column?

@piotrkwiecinski
Copy link
Contributor

@vrann I would, but I cannot.

@vrann
Copy link
Contributor Author

vrann commented Apr 11, 2017

@piotrkwiecinski I'm sorry, my mistake. Apparently, you need to have write permissions to repository to be able to move ticket or assign it. Let me handle it.

@piotrkwiecinski
Copy link
Contributor

@vrann cool thanks. I'll start implementation in meantime.

@piotrkwiecinski
Copy link
Contributor

@vrann I'm think about moving vcl file generator to separate class with nullable accepts host, port, etc and fallback to values from scopeConfig. This way in command we could use generator factory and easily populate parameters from input arguments. In UI we could use the same factory with data from scopeConfig as mentioned. What do you reckon? What about moving XML_VARNISH_PAGECACHE_* - consts to generator class?

I was considering adding stuff to the Config model, but it doesn't feel right :)

@piotrkwiecinski
Copy link
Contributor

OK public function getVclFile($vclTemplatePath) is marked as @api, so I could just pass generator to Config constructor and just execute it's getVclFile method then we wouldn't have to change UI implementation.

@vrann
Copy link
Contributor Author

vrann commented Apr 13, 2017

@piotrkwiecinski can you please join slack team https://magentocommeng.slack.com/, I created a channel for us to talk about this CLI.

Creating a new class and passing it to the \Magento\PageCache\Model\Config makes sense.

The only constraint: you cannot just add it as an argument of the constructor due to the backward compatibility reasons. The proper way to do it is following:

    public function __construct(
        \Old\Dependency\Intreface $oldDependency,
        $oldRequiredConstructorParameter,
        $oldOptinalConstructorParameter = null,
        \New\Dependency\Interface $newDependency = null
    ) {
        ...
        $this->newDependency = $newDependency ?: \Magento\Framework\App\ObjectManager::getInstance()->get(\New\Dependency\Interface::class);
    }

@vrann
Copy link
Contributor Author

vrann commented Apr 13, 2017

@piotrkwiecinski actually I need to invite you, if you can follow me on twitter (@vrann) we can exchange emails in direct messages.

@adragus-inviqa
Copy link
Contributor

adragus-inviqa commented Apr 14, 2017

I was just thinking: woldn't it better for the VCL generation be in a completely separate tool than M2?

Although having it as a CLI command is a start, this command depends on M2 being first installed. I'm not sure how this helps CI or various - say - Docker pipelines: you'd have to actually install M2 before you can setup Varnish. I think this coupling is unnecessary.

Although I'd imagine having a separate small project for this means more housekeeping (releases in sync with the main M2 codebase etc.), the advantages of keeping it separate would out-weight the effort.

For example, the "VCL generation tool" can be created as a) a simple PHP CLI tool, and, if used as a dependency to M2, b) you can merge it into M2's CLI commands via composer, following the instructions mentioned here :trollface: (jokes aside, I know you can inject CLI commands via Composer).

So do you think it's worth having a new magento/vcl repo and a new M2 dependency?

PS: I must insist on it (the new magento/vcl tool) not being an M2 module, but a simple script, so people can easily do php magento_vcl.php. If it was after me, I'd scrap php altogether, and use a shell script. No dependencies whatsoever. This is a pipeline feature, peeps, way outside any web app. It shouldn't be tied to it.

@vrann
Copy link
Contributor Author

vrann commented Apr 14, 2017

@adragus-inviqa from my point of view having it as a part of Magento makes complete sense because Magento handles and supports the default template for the VCL files. These files should be part of the core platform in order to ensure that varnish is always tested with the same configuration and this configuration is what is supported. That was the reason having the template in the repo and the generation feature as a part of admin backend. However, it turns out that admin backend is not very comfortable to use by system administrators.

Regarding requirements to have Magento installed to generate the VCL: sometimes this is the case, when you upgrade from built-in page-cache to the full-scale varnish, Magento will already be installed. For all the new installations (and specifically docker) I would suggest to generate vcl on the developer machine and commit it to the repository.

@adragus-inviqa
Copy link
Contributor

adragus-inviqa commented Apr 15, 2017

@vrann - Magento's VCL should be part of it, yes, but not as an "embedded" functionality. It's because it was hidden in admin UI that you now have to take it out into a separate CLI command. I suggest the same, only a step forward: out of the Magento app altogether. It's not that system admins aren't happy with generating VCL using the backend UI: they're not happy with the web app generating it.

And regarding that case, in which you convert from built-in page-cache to Varnish. I wonder how often that actually happens. All I see on the internetz is pre-built Docker containers: 1 Varnish and 1 M2.

I would suggest to generate vcl on the developer machine and commit it to the repository. - sure, but the point of dev machines is to already have Varnish on it. Catch-22.

This PR exists only because we're mixing infrastructure with business logic.

@magento-team magento-team added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Dev Tools up for grabs labels Jul 31, 2017
@magento-team
Copy link
Contributor

Internal ticket to track issue progress: MAGETWO-67537

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development up for grabs
Projects
None yet
Development

No branches or pull requests

7 participants