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

Monolithic Docker provider #2632

Merged
merged 72 commits into from
Aug 3, 2023
Merged

Monolithic Docker provider #2632

merged 72 commits into from
Aug 3, 2023

Conversation

tomjn
Copy link
Member

@tomjn tomjn commented Sep 11, 2022

Based on the work from @pentatonicfunk, I didn't have access to their github repo for this so here's a branch

Checks

  • I've updated the changelog.
  • I've tested this PR
  • This PR is for the develop branch not the stable branch.
  • This PR is complete and ready for review.

@tomjn
Copy link
Member Author

tomjn commented Jun 26, 2023

With that change I can load the dashboard, the 2 sites fail to provision though with Nginx issues though

@tomjn
Copy link
Member Author

tomjn commented Jun 26, 2023

It looks like the Nginx config didn't have proper search replacements, here's the vvv-nginx.conf it tried to install:

server {
    listen       80;
    listen       443 ssl http2;
    server_name  {vvv_hosts};
    root         "{vvv_path_to_site}{vvv_public_dir}";

    # Nginx logs
    error_log    "{vvv_path_to_site}/log/nginx-error.log";
    access_log   "{vvv_path_to_site}/log/nginx-access.log";

    # This is needed to set the PHP being used
    set          $upstream {upstream};

    # Enable server push if SSL/HTTP2 is being used for link preload headers
    http2_push_preload on;

    {vvv_tls_cert}
    {vvv_tls_key}

    # Nginx rules for WordPress, rewrite rules, permalinks, etc
    include      /etc/nginx/nginx-wp-common.conf;

    {{LIVE_URL}}

    location ~* \.(css|eot|gif|ico|jpeg|jpg|js|png|svg|tiff|tiff|ttf|webp|woff|woff2)$ {
        expires 100d;
    }
}

There's lots of unreadable temporary files in the provision folder too:

Screenshot 2023-06-26 at 14 02 39

the log suggests that sed couldn't open the file:

�[0;38;5;2m ▷ Running the �[0m�[1m�[0;38;5;5m'site-wordpress-one'�[21m�[0;38;5;2m provisioner...�[0m�[0m�[0m
 * Setting the default PHP CLI version ( 7.4 ) for this site
Now using node v14.21.3 (npm v6.14.18)
�[0m�[39m�[2m * Pulling down the master branch of https://github.com/Varying-Vagrant-Vagrants/custom-site-template.git�[21m�[0m
 * Updating wordpress-one provisioner repo in /srv/www/wordpress-one (https://github.com/Varying-Vagrant-Vagrants/custom-site-template.git, master)
 * Any local changes not present on the server will be discarded in favor of the remote branch
 * Checking that remote origin is https://github.com/Varying-Vagrant-Vagrants/custom-site-template.git
 * Fetching origin master
From https://github.com/Varying-Vagrant-Vagrants/custom-site-template
 * branch            master     -> FETCH_HEAD
 * performing a hard reset on origin/master
HEAD is now at 9f03c75 Update README.md
 * Updating provisioner repo complete
 * Adding domains to the virtual machine's /etc/hosts file...
 * Adding hosts for the site to the VM hosts file
   - Added one.wordpress.test from /vagrant/config.yml
 * Searching for a site template provisioner, vvv-init.sh
 * Found vvv-init.sh at /srv/www/wordpress-one/provision/vvv-init.sh
 * Custom site template provisioner wordpress-one - downloads and installs a copy of WP stable for testing, building client sites, etc
 * Creating database 'wordpress-one' (if it's not already there)
 * Granting the wp user priviledges to the 'wordpress-one' database
 * DB operations done.
 * Setting up the log subfolder for Nginx logs
 * Creating the public folder at 'public_html' if it doesn't exist already
 * Install type is 'single'
Notice: Undefined index: HTTP_HOST in /srv/www/wordpress-one/public_html/wp-includes/functions.php on line 6095
 * WordPress is present but isn't installed to the database, checking for SQL dumps in wp-content/database.sql or the main backup folder.
 * Installing WordPress
 * Installing using wp core install --url="one.wordpress.test" --title="one.wordpress.test" --admin_name="admin" --admin_email="[email protected]" --admin_password="password"
Success: WordPress installed successfully.
 * WordPress was installed, with the username 'admin', and the password 'password' at '[email protected]'
 * Copying the sites Nginx config template
 * Using the default vvv-nginx-default.conf, to customize, create a vvv-nginx-custom.conf
 * Applying public dir setting to Nginx config
sed: couldn't open temporary file /srv/www/wordpress-one/provision/sed1xsKca: Permission denied
sed: couldn't open temporary file /srv/www/wordpress-one/provision/sedRul2Ut: Permission denied
 * Adding constant 'WP_DEBUG' with value 'True' to wp-config.php
Success: Updated the constant 'WP_DEBUG' in the 'wp-config.php' file with the raw value 'True'.
 * Adding constant 'WP_DEBUG_LOG' with value 'True' to wp-config.php
Success: Updated the constant 'WP_DEBUG_LOG' in the 'wp-config.php' file with the raw value 'True'.
 * Adding constant 'WP_DISABLE_FATAL_ERROR_HANDLER' with value 'True' to wp-config.php
Success: Updated the constant 'WP_DISABLE_FATAL_ERROR_HANDLER' in the 'wp-config.php' file with the raw value 'True'.
 * Site Template provisioner script completed for wordpress-one
�[0m�[39m�[2m * sourcing of vvv-init.sh reported success�[21m�[0m
�[0m�[39m�[2m * VVV is adding an Nginx config from /srv/www/wordpress-one/provision/vvv-nginx.conf�[21m�[0m
nginx: [warn] the "listen ... http2" directive is deprecated, use the "http2" directive instead in /etc/nginx/custom-sites/vvv-wordpress-one-60730e70.conf:3
nginx: [warn] the "http2_push_preload" directive is obsolete, ignored in /etc/nginx/custom-sites/vvv-wordpress-one-60730e70.conf:15
nginx: [emerg] unexpected "{" in /etc/nginx/custom-sites/vvv-wordpress-one-60730e70.conf:23
nginx: configuration file /etc/nginx/nginx.conf test failed
�[0;38;5;9m ! Installing an Nginx config failed! VVV tried to install vvv-wordpress-one-60730e70.conf into sites from /tmp/vvv-site-IMkTb but a syntax test with sudo nginx -t failed!�[0m�[0m
�[0;38;5;9m ! VVV is now deleting the config to avoid further breakage�[0m�[0m
�[0;38;5;3m ! This sites nginx config had problems, it may not load. Look at the above errors to diagnose the problem�[0m�[0m
�[0m�[39m�[2m ! VVV will now continue with provisioning so that other sites have an opportunity to run�[21m�[0m
�[0;38;5;2m * Installed vvv-wordpress-one-60730e70.conf�[0m�[0m
�[0;38;5;2m ✔ The �[0m�[1m�[0;38;5;5m'site-wordpress-one'�[21m�[0;38;5;2m provisioner completed in �[0m�[0m�[1m�[0;38;5;5m3�[21m�[0;38;5;2m seconds.�[0m�[0m�[0m

@tomjn
Copy link
Member Author

tomjn commented Jun 26, 2023

The -i parameter is the problem, the temporary files it makes aren't writable in certain filesystems, this is fixed in v4.8 of sed, but we should look into an alternative such as in memory replacement instead, or explicit temporary files

@msaggiorato
Copy link
Member

The -i parameter is the problem, the temporary files it makes aren't writable in certain filesystems, this is fixed in v4.8 of sed, but we should look into an alternative such as in memory replacement instead, or explicit temporary files

Hmm, that problem is common for me in volumes with Docker on Mac. I always have to go with explicit temp files, which makes all commands more complicated to write.

Switching to this mapped the ports:

    d.ports =  [ "80:80" ]
    d.ports += [ "443:443" ]
    d.ports += [ "3306:3306" ]
    d.ports += [ "8025:8025" ]

however I had to destroy the container and recreate it for it to apply, using vagrant 2.3.7

The need to destroy and recreate the container should be on the docker provider for Vagrant.

I've implemented the "magic" (not really, as it didn't work for you) port mapping to avoid conflicts with other containers that may be using that port already. We can remove the function that is not used anymore (if you didn't remove it already) and just force it to collide with anything else you may have running.

@tomjn
Copy link
Member Author

tomjn commented Jun 27, 2023

I've implemented the "magic" (not really, as it didn't work for you) port mapping to avoid conflicts with other containers that may be using that port already. We can remove the function that is not used anymore (if you didn't remove it already) and just force it to collide with anything else you may have running.

Honestly I'm happy to just document it as that's what all the other docker local environments do, fully stop your other environments before starting ours is very common to see.

In the long run it might be nice to explore reusing their Traefik proxies but for now this is reasonable, and more user friendly than switching ports

@tomjn
Copy link
Member Author

tomjn commented Jun 27, 2023

Hmm, that problem is common for me in volumes with Docker on Mac. I always have to go with explicit temp files, which makes all commands more complicated to write.

Could that be wrapped in a function? I was mulling using sed with a temporary variable, the downside is if sed fails we don't have a file to debug

@tomjn
Copy link
Member Author

tomjn commented Jun 28, 2023

I asked ChatGPT and forgot to specify sed and it came up with this:

replace_variables_in_file() {
  local file="$1"
  local token="$2"
  local value="$3"

  # Read the file contents and replace the token with the value
  local content
  if [[ -f "$file" ]]; then
    content=$(<"$file")
    content=${content//$token/$value}
    echo "$content"
  else
    return 1
  fi
}

I tested on MacOS and surprisingly it worked in ZSH which I didn't expect. Need to try adjusting the provisioner and with shellcheck related changes

@tomjn
Copy link
Member Author

tomjn commented Jun 28, 2023

We'll need this function or something with similar functionality in provision-site.sh as well as in the custom site template repos, a polyfill of sorts will be needed for those

@msaggiorato
Copy link
Member

I tried working on a replacement, and we need to also replace it in the template provider, but it was late at night and got stuck in retro compatibility...

we may not be able to reuse what we include in VVV docker, in case people don't update right away, so we may need to duplicate the function in VVV and the site template for some time

@msaggiorato
Copy link
Member

Just pushed the replacement function here

@tomjn
Copy link
Member Author

tomjn commented Jun 28, 2023

@msaggiorato I just tried to push commits and saw you'd made changes, there's a new PR with my version that relies on bash replacement and variables, there's other things that would be good to cherry pick out

Copy link
Member Author

@tomjn tomjn left a comment

Choose a reason for hiding this comment

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

looks like I forgot to submit some comments

Vagrantfile Outdated Show resolved Hide resolved
Vagrantfile Outdated Show resolved Hide resolved

# Docker use image.
config.vm.provider :docker do |d, override|
d.image = 'pentatonicfunk/vagrant-ubuntu-base-images:20.04'
Copy link
Member Author

Choose a reason for hiding this comment

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

we can avoid this by using a dockerfile and layering over a general ubuntu 20 container

Copy link
Member

Choose a reason for hiding this comment

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

That'd be preferrable for me as well

d.ports += [ "#{mailhog_port}:8025" ]

## Fix goodhosts aliases format for docker
override.goodhosts.aliases = { '127.0.0.1' => vvv_config['hosts'], '::1' => vvv_config['hosts'] }
Copy link
Member Author

Choose a reason for hiding this comment

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

noting that if we used docker compose then traefik can be used and goodhosts becomes optional

Copy link
Member

Choose a reason for hiding this comment

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

traefik will not handle domains unless we have some true real life domain available, like vvv.dev or something on those lines. This is all explained here.

Traefik would be useful if we wanted to have multi-container nginx, or node apps supports or something, so that you can access different things based on, for example, paths:

Example
localhost/wp1 -> nginx container -> php8 container
localhost/wp2 -> nginx-legacy container (old version or something) -> php 5.4 container
localhost/nodeapp -> node app

In this example, and since all sites are accesed on port 80, and just different apps, traefik will forward traffic to each container, and in the case of nginx containers, they'll use the configured php upstream containers as needed.

Since i don't think we need node apps forwarding for the time being, and also, we're not thinking of supporting multiple versions of nginx in the foreseable future.

We don't need traefik, and we can just expose nginx port 80.

vvv.test will never load in traefik (or nginx) unless your machine can resolve vvv.test to 127.0.0.1. there's only a few ways this can happen:

  • hostfile editing (goodhosts), which is already in place and works fine, with the added dependency
  • zeroconf dns (bonjour, etc) which Chassis uses by interfacing with dbus here. This works because of the way VirtualBox handles networking. This wouldn't work on Docker for Mac or Windows (may work for Linux, with extra permissions). In any case, it requires implementation which we don't currently have, and is not straightforward.
  • Real life domain (this is the lando approach), explained here. This is basically a small server somewhere on the public internet, which works as a DNS and resolves any domain you point to it to 127.0.0.1. This is why asd.test2.lndo.site test2.lndo.site vvv.lndo.site and asd.vvv.lndo.site or anything you can think of, works. Because it's a real life domain on the public internet. You can check it out on any DNS checker online.

Copy link
Member Author

Choose a reason for hiding this comment

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

Traefik would be useful if we wanted to have multi-container nginx, or node apps supports or something, so that you can access different things based on, for example, paths:

That is something I'd like in the future, particularly for MariaDB/ES/Mailhog. I can see an argument for there being an Nginx container too, and in the long run it also allows us to be more flexible with PHP versions and php logging on a per site basis

Copy link
Member Author

Choose a reason for hiding this comment

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

also note that Altis Local Server manages this somehow with Traefik

Copy link
Member

Choose a reason for hiding this comment

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

I can look into the code, but it's likely with one of the 3 options above.

Is the project open source for the altis solution?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/humanmade/altis-local-server/blob/f44d1ca1b803537ed5c8c940adf96ac9cfb0132b/inc/composer/class-command.php#L922

As far as I can see, they are not generating entries, at least not directly in the project.

Copy link
Member

Choose a reason for hiding this comment

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

Even more so, they state this on the docs

Note: Altis does not manage the host entries for subdomains or custom domains, you'll need to manage those manually, via editing /etc/hosts in Linux / macOS, or C:\Windows\System32\Drivers\etc\hosts in Windows. Altis however tries to detect if those entries do not exist, and outputs the necessary configurations to add to your hosts file.

Copy link
Member Author

@tomjn tomjn Jul 4, 2023

Choose a reason for hiding this comment

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

that's ok, I'm not seeing any crazy hacks to map it though, they're just saying go to 127.0.0.1 essentially. The caveat being that this might mean local server isn't what responds if it's not active since it isn't that specific container it's mapped to

Copy link
Member Author

Choose a reason for hiding this comment

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

In the grand scheme of things though, I don't think this is the highest priority, I'd prioritise extracting improvements that aren't strictly docker related that could be merged in advance, and having a docker file to remove the need for the pentatonic docker image so we can layer things in the future

Copy link
Member

Choose a reason for hiding this comment

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

ok, so goodhosts stays for the foreseable future as it solves this problem

about the more prioritary things, totally onboard with doing those that can be merged in advance

@@ -121,7 +121,7 @@ function cleanup_vvv(){
echo "127.0.0.1 tideways.vvv.test # vvv-auto" >> "/etc/hosts"
echo "127.0.0.1 xhgui.vvv.test # vvv-auto" >> "/etc/hosts"
fi
mv /tmp/hosts /etc/hosts
echo "$(</tmp/hosts)" | sudo tee -a /etc/hosts > /dev/null
Copy link
Member Author

Choose a reason for hiding this comment

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

this could be extracted into a separate PR

@@ -125,7 +129,7 @@ function mysql_setup() {
# MySQL gives us an error if we restart a non running service, which
# happens after a `vagrant halt`. Check to see if it's running before
# deciding whether to start or restart.
if [ $(service mariadb status|grep 'mysql start/running' | wc -l) -ne 1 ]; then
if [ $(service mariadb status|grep 'Uptime' | wc -l) -ne 1 ]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

this could be extracted into a separate PR

Comment on lines +61 to +62
git config --global --add safe.directory '*' # Allow git to work well under docker provider
noroot git config --global --add safe.directory '*' # Allow git to work well under docker provider
Copy link
Member Author

Choose a reason for hiding this comment

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

this should be extracted into a separate PR

Comment on lines +30 to +33
# /etc/host doesn't survive restart on docker
vvv_info " * Reinit /etc/hosts"
vvv_update_guest_hosts

Copy link
Member Author

Choose a reason for hiding this comment

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

noting this would be better in a dockerfile

Vagrantfile Show resolved Hide resolved
Vagrantfile Outdated Show resolved Hide resolved
@tomjn tomjn mentioned this pull request Jul 23, 2023
4 tasks
@tomjn tomjn marked this pull request as ready for review August 3, 2023 11:19
@tomjn
Copy link
Member Author

tomjn commented Aug 3, 2023

@Mte90 @msaggiorato shall we merge this into develop later today after a 3.12 release?

@tomjn tomjn merged commit b678676 into develop Aug 3, 2023
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