Unbreak Debian support with custom encoding #1564

smortex · 2024-01-03T22:17:12Z

On Debian, version 10.0.0+ of the module cause a broken catalog due to dependecy cycles:

Anchor[postgresql::server::service::begin::main] =>
Postgresql_psql[Set template1 encoding to UTF8] =>
Postgresql::Server::Instance::Late_initdb[main] =>
Postgresql::Server::Instance::Initdb[main] =>
Class[Postgresql::Server::Initdb] =>
Postgresql_conf[port_for_instance_main] =>
Class[Postgresql::Server::Service] =>
Postgresql::Server::Instance::Service[main] =>
Anchor[postgresql::server::service::begin::main]

Reenable the test that trigger this issue (taken from #1547) and try to fix it.

antaflos · 2024-01-10T15:14:17Z

We are successfully running this branch and it works as advertised. No more dependency cycles. 👍

smortex · 2024-01-11T03:04:35Z

@antaflos Beware that this is unfortunately still WIP because it breaks EL for reasons I had not time to discover yet. I have been busy the last week with other $WORK related things, and am about to go on vacation for a week, so will not be able to have a look before I am back.

But thank your for your feedback 👍 : for now I only tested it with the CI and as you can see with d15ac61 there is some nasty bug because A -> B + A -> B -> C and A -> B + B -> C should not change much regarding dependencies.

bastelfreak · 2024-04-16T06:48:17Z

lib/puppet/type/postgresql_psql.rb

@@ -135,11 +135,7 @@ def matches(value)
  end

  autorequire(:anchor) do


as an alternative, can we maybe autorequire the postgresql_conn_validator resource?

That quite make sense. I'll dig into this.

bastelfreak · 2024-04-16T06:50:09Z

spec/acceptance/utf8_encoding_spec.rb

+    puts '-------------------------------'
+    puts LitmusHelper.instance.run_shell('systemctl status postgresql*').stdout
+    puts '-------------------------------'
+    expect(port(5432)).to be_listening.on('127.0.0.1').with('tcp')


the problem I had in the past: some systems report tcp, some tcp4. that was really ugly so I switched to matching the IP address. and that sometimes didn't work because some CI systems have IPv6 so it binds to ::1 but sometimes it's only 127.0.0.1. all ugly...

I am doing more test because this is quite weird. Looking at the code it should work, looking at ss output it seems to be working, but it is not and that makes me feel angry. Similar code work in other areas of the project so I guess something is very wrong… I would like to nail it rather than ignoring it if possible.

ekohl

Looking at this, I think the initdb (which doesn't need a DB running) shouldn't include late_initdb (which does need a server running). Instead, postgresql::server_instance should be modified to split it.

The reason you run into this is that Debian has $needs_initdb set to false by default:

puppetlabs-postgresql/manifests/params.pp

Line 145 in 5e6e994

    
           $needs_initdb = pick($needs_initdb, $postgresql::globals::manage_package_repo == true)

So this code path is triggered:

puppetlabs-postgresql/manifests/server/instance/initdb.pp

Lines 195 to 202 in 5e6e994

    
           postgresql::server::instance::late_initdb { $name: 
        
             encoding       => $encoding, 
        
             user           => $user, 
        
             group          => $group, 
        
             module_workdir => $module_workdir, 
        
             psql_path      => $psql_path, 
        
             port           => $port, 
        
           }

Wouldn't it make more sense to move it to postgresql::server_instance as:

if !$initdb_settings['needs_initdb'] and $initdb_settings['encoding'] {
  postgresql::server::instance::late_initdb { $instance_name:
    encoding       => $initdb_settings['encoding'],
    user           => $initdb_settings['user'],
    group          => $initdb_settings['group'],
    module_workdir => $initdb_settings['module_workdir'],
    psql_path      => $initdb_settings['psql_path'],
    port           => $initdb_settings['port'],
  }
}

It may break because those values may not be undef, but I hope the idea is clear.

ekohl · 2024-04-16T18:05:09Z

lib/puppet/type/postgresql_psql.rb

-    ["postgresql::server::service::begin::#{self[:instance]}"]
-  end
-
-  autorequire(:service) do


I'm trying to understand the logic. Don't you always need the server to be running to execute any SQL statements?

yes, but the service doesn't seem to be available immediately after start. There's a custom resource that waits until the tcp port is open and I think we should depend on that.

Oh yes, that makes sense. Though with the newest versions (Fedora 38 with PostgreSQL 15) I see the systemd unit is Type=notify so I'd assume that there it will only really be ready once it's listening (spoiler: the patch sends READY=1 right after logging database system is ready to accept connections).

Digging into this, it was introduced in postgres/postgres@7d17e68 so with PostgreSQL 9.6 it became possible to support. Looking at EL8 that's built with Type=notify, but EL7 probably isn't. Sadly Debian 11 & 12 are also Type=forking. That means you're right and we still need to depend on the connection validator.

Oh yes, that makes sense. Though with the newest versions (Fedora 38 with PostgreSQL 15) I see the systemd unit is Type=notify so I'd assume that there it will only really be ready once it's listening (spoiler: the patch sends READY=1 right after logging database system is ready to accept connections).

Digging into this, it was introduced in postgres/postgres@7d17e68 so with PostgreSQL 9.6 it became possible to support. Looking at EL8 that's built with Type=notify, but EL7 probably isn't. Sadly Debian 11 & 12 are also Type=forking. That means you're right and we still need to depend on the connection validator.

These seem to be the relevant bugs about this:

https://bugs.launchpad.net/ubuntu/+source/postgresql-common/+bug/1862138

https://bugs.debian.org/872915

ShadNex · 2024-07-12T11:12:13Z

I've tested this PR as suggested by @bastelfreak and indeed it seems to solve the dependency issue for our environment as seen in #1541 👍 Would be great if this gets released soon!

davidsandilands · 2024-07-16T13:50:51Z

manifests/server/instance/service.pp

@@ -42,6 +42,10 @@
      status    => $service_status,
    }

+    Anchor["postgresql::server::service::begin::${name}"]


I may deeply regret asking when I do not have a full background on this but my understanding was Anchor was a redundant pattern as the contain function came in?

Yes anchors are from the past. Not sure if attempting to remove them in this PR will help however (and that would be a backwards incompatible change). But in the long term, having them gone is probably a good idea.

ShadNex · 2024-09-11T04:15:27Z

Is there someting missing for this PR to get merged? Would be nice to have it in the next relase.

bastelfreak · 2024-10-17T17:02:10Z

@ShadNex yes, passing tests :)

@smortex can you rebase again please?

When using anchors, resources should be sandwiched those. An ordering relation was missing for the "end" anchor.

Some resources where not properly ordered between the "begin" and "end" anchors. Make sure they are properly bound to them.

It does not really make sense to require the "begin" anchor, the `postgresql_psql` type communicating with the server, it makes more sense to require the "end" anchor which mean that the server is running and reachable.

We already have a relationship in `manifests/server/instance/service.pp` that ensure `Service["postgresqld_instance_${name}"]` is realized before `Anchor["postgresql::server::service::end::${name}"]`. Fun fact, removing this duplaciate fix the circular dependency reported by puppet.

smortex · 2024-10-17T21:07:13Z

Please bear with me while my fat fingers type crap on the keyboard 😒 .

ss(8) report the same for a working and non-working port as far as serverspec is concerned. Maybe ordering has an impact? We know these tests fail when running at the end of the test suite, so attempt to run them earlier.

smortex · 2024-10-18T01:33:45Z

Please bear with me while my fat fingers type crap on the keyboard 😒 .

Thank you for your patience.

Rebasing done, I pushed each commit individually to have a step by step view of what works or not. I obviously lost track since January, and was (and still am) out of ideas so if you want to try something you are very welcome to do so. It seems that failures are test-order dependent…

ia-content added the community label Jan 3, 2024

bastelfreak added the bugfix label Jan 3, 2024

smortex force-pushed the unbreak-debian-support-with-encoding branch from 37dfadf to ed8981e Compare January 4, 2024 00:57

ekohl linked an issue Apr 5, 2024 that may be closed by this pull request

setting the encoding parameter causes a dependency cycle #1543

Open

smortex force-pushed the unbreak-debian-support-with-encoding branch 5 times, most recently from 6414904 to b2284f1 Compare April 16, 2024 02:37

bastelfreak reviewed Apr 16, 2024

View reviewed changes

ekohl reviewed Apr 16, 2024

View reviewed changes

smortex force-pushed the unbreak-debian-support-with-encoding branch 2 times, most recently from e6c4fd0 to fc5ff27 Compare April 17, 2024 21:35

smortex mentioned this pull request May 2, 2024

Modules version 7.14.0 not uploaded to the forge puppetlabs/puppetlabs-puppetdb#381

Closed

Ramesh7 force-pushed the unbreak-debian-support-with-encoding branch from 50226f2 to 692f6e1 Compare June 6, 2024 15:48

bastelfreak mentioned this pull request Jul 12, 2024

postgresql::server::config_entry - dependency cycles detected (after upgrade to 10.0.0) #1541

Open

davidsandilands reviewed Jul 16, 2024

View reviewed changes

bastelfreak and others added 5 commits October 17, 2024 11:00

[PATCH] reenable acceptance test for server encoding

2d19e18

Fix anchoring in postgresql::server::plperl

5141e7d

When using anchors, resources should be sandwiched those. An ordering relation was missing for the "end" anchor.

Fix anchoring in postgresql::server::instance::service

cb08fca

Some resources where not properly ordered between the "begin" and "end" anchors. Make sure they are properly bound to them.

Fix postgresql_psql autorequire

823af9a

It does not really make sense to require the "begin" anchor, the `postgresql_psql` type communicating with the server, it makes more sense to require the "end" anchor which mean that the server is running and reachable.

smortex force-pushed the unbreak-debian-support-with-encoding branch from 692f6e1 to 2d19e18 Compare October 17, 2024 21:01

smortex closed this Oct 17, 2024

smortex deleted the unbreak-debian-support-with-encoding branch October 17, 2024 21:04

smortex restored the unbreak-debian-support-with-encoding branch October 17, 2024 21:05

smortex reopened this Oct 17, 2024

DELETE ME

38c9189

ss(8) report the same for a working and non-working port as far as serverspec is concerned. Maybe ordering has an impact? We know these tests fail when running at the end of the test suite, so attempt to run them earlier.

smortex force-pushed the unbreak-debian-support-with-encoding branch from 2893af5 to 38c9189 Compare October 18, 2024 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unbreak Debian support with custom encoding #1564

Unbreak Debian support with custom encoding #1564

smortex commented Jan 3, 2024

antaflos commented Jan 10, 2024

smortex commented Jan 11, 2024 •

edited

Loading

bastelfreak Apr 16, 2024

smortex Apr 17, 2024

bastelfreak Apr 16, 2024

smortex Apr 17, 2024

ekohl left a comment

ekohl Apr 16, 2024

bastelfreak Apr 18, 2024

ekohl Apr 18, 2024

kenyon Apr 18, 2024

ShadNex commented Jul 12, 2024

davidsandilands Jul 16, 2024

smortex Oct 17, 2024

ShadNex commented Sep 11, 2024

bastelfreak commented Oct 17, 2024

smortex commented Oct 17, 2024

smortex commented Oct 18, 2024

		@@ -135,11 +135,7 @@ def matches(value)
		end

		autorequire(:anchor) do

	postgresql::server::instance::late_initdb { $name:
	encoding => $encoding,
	user => $user,
	group => $group,
	module_workdir => $module_workdir,
	psql_path => $psql_path,
	port => $port,
	}

Unbreak Debian support with custom encoding #1564

Are you sure you want to change the base?

Unbreak Debian support with custom encoding #1564

Conversation

smortex commented Jan 3, 2024

antaflos commented Jan 10, 2024

smortex commented Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShadNex commented Jul 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShadNex commented Sep 11, 2024

bastelfreak commented Oct 17, 2024

smortex commented Oct 17, 2024

smortex commented Oct 18, 2024

smortex commented Jan 11, 2024 •

edited

Loading