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

postgres: listen_addresses no longer sets PGHOST #1374

Open
DGollings opened this issue Aug 10, 2024 · 10 comments
Open

postgres: listen_addresses no longer sets PGHOST #1374

DGollings opened this issue Aug 10, 2024 · 10 comments
Labels
bug Something isn't working

Comments

@DGollings
Copy link

DGollings commented Aug 10, 2024

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

@DGollings DGollings added the bug Something isn't working label Aug 10, 2024
@sandydoo
Copy link
Member

@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
Copy link
Author

DGollings commented Aug 11, 2024

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?

  • before devenv (ci.yaml)
  • pre setting up service (devenv.nix)
  • post setting up service (devenv.nix)
  • within devenv shell
  • env as seen from within application?

?

@k3yss
Copy link
Collaborator

k3yss commented Aug 11, 2024

@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
Copy link
Author

DGollings commented Aug 11, 2024

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
Copy link
Collaborator

k3yss commented Aug 11, 2024

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
Copy link
Author

DGollings commented Aug 11, 2024

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
Copy link
Collaborator

k3yss commented Aug 11, 2024

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
Copy link
Author

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? #1374 (comment)

@k3yss
Copy link
Collaborator

k3yss commented Aug 11, 2024

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
Copy link
Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants