cachix / devenv

Fast, Declarative, Reproducible, and Composable Developer Environments
https://devenv.sh
Apache License 2.0
4.04k stars 303 forks source link

postgres: listen_addresses no longer sets PGHOST #1374

Open DGollings opened 1 month ago

DGollings commented 1 month ago

Describe the bug After updating from devenv 1.0.2 to 1.0.8 we could no longer connect to our database during CI tests. After extensive debugging (mostly because I knew nothing about PGHOST) it was determined to be caused by this change

git diff v1.0.7..v1.0.8 src/modules/services/postgres.nix

diff --git a/src/modules/services/postgres.nix b/src/modules/services/postgres.nix
index 7e8584c..183aff5 100644
--- a/src/modules/services/postgres.nix
+++ b/src/modules/services/postgres.nix
@@ -282,10 +282,7 @@ in
     packages = [ postgresPkg startScript ];

     env.PGDATA = config.env.DEVENV_STATE + "/postgres";
-    env.PGHOST =
-      if cfg.listen_addresses == ""
-      then runtimeDir
-      else cfg.listen_addresses;
+    env.PGHOST = lib.mkDefault runtimeDir;
     env.PGPORT = cfg.port;

     services.postgres.settings = {

In our CI it never quite worked until I set listen_addresses to "127.0.0.1", although I did not quite understnad at the time why

To be honest, I think how we fixed it merely worked around a different issue. The issue being that we run our CI on NixOS github runners which has a particular runtimeDir which is then overridden when we use devenv shell within that same environment. This manifests itself in the sense that psql works during devenv process but at some point the PGHOST (or runtimeDir) is changed again

from memory, /var/run/1000/devenv/something_socket vs /tmp/devenv/something_socket

the current 'fix' I have now is setting

if System.get_env("CI") == "true" do
  System.put_env("PGHOST", "127.0.0.1")
end

in our runtime config. Which should speak for itself

Anyway, this still raises a different question as to why is PGHOST currently 'hardcoded' or even set at all, with no clear way to override. Could it be that the previous code was better? Apart from the listen_addreses = "*" issue I believe was the cause of this change?

Version

1.0.7 and 1.0.8

sandydoo commented 1 month ago

@DGollings, PGHOST is there as a default to help tools connect to the database. Perhaps we need to parse listen_addresses after all to set this properly.

But I'm surprised to hear that the socket directory doesn't work for you. I wonder if it's because of the systemd service or the runner. For example, It might be a sign that we need to detect and use RUNNER_TEMP when setting up our temporary directories.

It's tough for us to document these things right now, because it's just an env var. We'll soon have a place to do so per service, language, etc.

cc @k3yss

DGollings commented 1 month ago

what makes PGHOST special is that the driver uses that if set, pretty much disregarding other options/settings/parameters afaict

which makes setting it from devenv without being able to override it (easily) a little problematic :)

It is however true that the root issue is likely the whole systemd -> runner -> devenv shell combo, and something somewhere being changed regarding the tmp directories by something. How can I help debug this? Some strategically placed echos are likely useful, where would you like to see some?

?

k3yss commented 1 month ago

@DGollings

which makes setting it from devenv without being able to override it (easily) a little problematic :)

{ pkgs, lib, config, inputs, ... }:

{
 services.postgres.enable = true;
 env.PGHOST = "custom-pg-host";
}

I can easily override it like this, does this not work for you? While sending the patch you mentioned before I was expecting people to change their PGHOST in a similar manner.

DGollings commented 1 month ago

You might be over estimating the average nix user. Was not aware that that would override the other value. Also, it feels unintuitive to change that 'outside' of the services and postgres context. Also, you'd have to know that many drivers use that value and that it's being set in the first place by devenv.

But thanks, good to know, it'll make it easy to remove my workaround :)

k3yss commented 1 month ago

You might be over estimating the average nix user. Was not aware that that would override the other value. Also, it feels unintuitive to change that 'outside' of the services and postgres context.

I am working on the new documentation here to resolve this exact issue, we can include a small guide in it on how to use postgres with devenv : )

Also, you'd have to know that many drivers use that value and that it's being set in the first place by devenv.

The default value that we are setting for the PGHOST is based upon the Official Postgres documentation, I find it a bit strange that drivers that you are mentioning are expecting 127.0.0.1 as the value, but since the POSTGRES documentation mentions to use the unix socket I don't really see a reason to change it.

DGollings commented 1 month ago

The default value that we are setting for the PGHOST is based upon the Official Postgres documentation, I find it a bit strange that drivers that you are mentioning are expecting 127.0.0.1 as the value, but since the POSTGRES documentation mentions to use the unix socket I don't really see a reason to change it.

Oh no, there's two issues here. One being PGHOST and all we've discussed so far regarding defaults and discoverability.

But the other issue is that your default, and I believe you if you say it's a good default, for whatever reaso, does not play nicely with systemd and devenv shell combined (we think)

The driver is not the issue, nor does it depend on localhost being the value. That's just a symptom

As mentioned, I'd be happy to help debug?

k3yss commented 1 month ago

As mentioned, I'd be happy to help debug?

That would be awesome! I don't have any experience with systemd, but if you could provide detailed steps on how to create a reproducible setup of the problem and share some logs, I'd be happy to take a look.

DGollings commented 1 month ago

As mentioned, I'd be happy to help debug?

That would be awesome! I don't have any experience with systemd, but if you could provide detailed steps on how to create a reproducible setup of the problem and share some logs, I'd be happy to take a look.

Sure, how does this sound? https://github.com/cachix/devenv/issues/1374#issuecomment-2282705206

k3yss commented 1 month ago

What I want to know is the step before these, specifically how to provision the CI environment locally and the steps how I would reproduce the error.

DGollings commented 1 month ago

If you happen to run NixOS it should be easy enough to set up your own version of this:

github-runners = {
      runner = {
        name = config.networking.hostName;
        user = *youruser*;
        enable = true;
        replace = true;
        url = "redacted";
        tokenFile = *github token runner file*;
        extraPackages = with pkgs;
          [
            devenv
            bash
            # any other dependency you might want to add
          ];
        extraEnvironment = {
          NIXPKGS_ALLOW_UNFREE = "1";
          JAVA_HOME = "${pkgs.openjdk}";
        };
        serviceOverrides = {
          ProtectProc = "default";
          ProtectSystem = "full";
        };
      };
    };

if not, the above pretty much is a systemd service, which you could install like this

As to how to reproduce in more detail, these steps work:

  processes = {
    phoenix.exec = ''
      # sets up db automatically
      until psql -U "postgres" -c '\q' 2>/dev/null; do
        sleep 1
      done

      mix ecto.setup
      mix phx.server
    '';
  };

These are run by devenv itself within the systemd environment

neither psql nor ecto.setup error. The former being postgres cli and the latter the same Elixir Postgres driver that I have referenced before

What does not work is this:

      - name: Build the devenv shell and run any pre-commit hooks
        run: devenv shell mix check

which is running the same Postgrex/Ecto-like code as in mix ecto.setup but within a 'new' context/shell/env (being devenv shell)