HA: Limit max open database connections to 1

[yhabteab]

Previously, the HA feature was allowed to open max_connections database connection in parallel to other Icinga DB components. Meaning, Icinga DB wasn't limited to the configured max_connections, but effectively to 2 * max_connections.

[yhabteab]

The HA transaction is rolled back correctly with:

• Abort HA Realization Logic After Timeout #800

[yhabteab]

This is the proof that we actually leak database connections but only for as long as the context is not cancelled.

package main

import (
	"context"
	"github.com/icinga/icinga-go-library/logging"
	"github.com/icinga/icingadb/internal/command"
	"go.uber.org/zap"
	"log"
	"time"
)

func main() {
	cmd := command.New()

	db, err := cmd.Database(logging.NewLogger(zap.NewNop().Sugar(), time.Second))
	if err != nil {
		log.Fatal(err)
	}
	db.SetMaxOpenConns(3)

	go func() {
		time.Sleep(1 * time.Second)

		ctx, _ := context.WithDeadline(context.Background(), time.Now().Add(10*time.Second))
		_, _ = db.BeginTxx(ctx, nil)
		return
	}()
	go func() {
		time.Sleep(1 * time.Second)

		ctx, _ := context.WithDeadline(context.Background(), time.Now().Add(10*time.Second))
		_, _ = db.BeginTxx(ctx, nil)
		return
	}()

	time.Sleep(3 * time.Second)

	ctx, cancelFunc := context.WithDeadline(context.Background(), time.Now().Add(2*time.Second))
	defer cancelFunc()
	_ = db.PingContext(ctx)
	log.Printf("OpenConnections: %v", db.Stats().OpenConnections)
	log.Printf("InUse: %v", db.Stats().InUse)
}

~/Workspace/go/icingadb (set-ha-conn-limit ✗) go run ./cmd/decoder/decoder.go --config config.example.yml
2024/10/23 16:19:59 OpenConnections: 3
2024/10/23 16:19:59 InUse: 2

[lippserd]

I would remove this information as it is an implementation detail. Sure, users might be surprised if there are 17 database connections, but I doubt anyone will ever count them.

Instead, we could reduce the size of the main pool by one. But I wouldn't attach too much importance to that.

[yhabteab]

Since the size of the main pool is initialized in the library, we don't want to adjust it just because of Icinga DB's HA feature. Thus, I removed the highlighted info box.

[lippserd]

The example configuration would also have to be adjusted. But please just remove it completely. It is irrelevant.

[yhabteab]

As we discussed offline, I have completely removed the extra info.

[lippserd]

I want to summarize our discussion: With this PR, we currently have only one connection that isn't tied to max_connections. However, there's a chance more could be added or changed, leaving it up to the user to determine and maintain the correct max_connections value if the goal is to truly limit Icinga DB database connections to that number. Therefore, if max_connections is intended to represent the actual maximum connections, it should be implemented accordingly, rather than documented that some connections are unaffected by that setting.

[oxzi]

Additionally, this masked a serious bug in the HA#realize() method, where we start a new transaction after each retry without rolling back in case of an error, leading to connections not being released before exceeding the ctx deadline.

This bug is being addressed in #800. In spite of other changes over there, I am a bit critical about reducing the amount of database connections to a single one. Currently, there is still the possibility of a long-running COMMIT to the database which cannot be canceled, as described in this comment. Could such a scenario then result into a blocking state if there is just one connection available? I guess no, but my gut instinct is uncertain.

[lippserd]

Additionally, this masked a serious bug in the HA#realize() method, where we start a new transaction after each retry without rolling back in case of an error, leading to connections not being released before exceeding the ctx deadline.

This bug is being addressed in #800. In spite of other changes over there, I am a bit critical about reducing the amount of database connections to a single one. Currently, there is still the possibility of a long-running COMMIT to the database which cannot be canceled, as described in this comment. Could such a scenario then result into a blocking state if there is just one connection available? I guess no, but my gut instinct is uncertain.

With a long-running commit and more than one database connection, successive transactions would be blocked during the SELECT. With only one database connection, the code waits at the begin transaction point. To me this is a valid change as we are not opening new database connections unnecessarily. But since PR #800 was merged, the PR and commit description should reflect that this change is only about that. @yhabteab Would you please adjust that accordingly?

[lippserd]

@yhabteab With both MySQL and PostgreSQL integration tests failing, I think something is wrong 😆

[oxzi]

I followed @yhabteab's statement that this PR breaks when used together with #800 and tried a bisect with this PR's change on each commit of my PR. Indeed, dd0ca8f broke it. Let me debug it further :)

$ git bisect start HEAD HEAD~6 --
Bisecting: 2 revisions left to test after this (roughly 2 steps)
[f8819208ce4856a83d627d703f749e040c4d2888] HA: Deferred SQL Transaction Rollback


$ cat 828.sh
#!/bin/sh

set -eux

git cherry-pick --no-commit set-ha-conn-limit

CGO_ENABLED=0 go build ./cmd/icingadb

pushd tests
go test -o ../icingadb-test -c .
popd

set +e
export ICINGADB_TESTS_DATABASE_TYPE="mysql"
export ICINGA_TESTING_ICINGADB_BINARY="$(pwd)/icingadb"
export ICINGA_TESTING_ICINGADB_SCHEMA_MYSQL="$(pwd)/schema/mysql/schema.sql"
./icingadb-test -icingatesting.debuglog debug.log -test.v -test.run TestCleanupAndRetention
RET="$?"
set -e

git restore --staged cmd/icingadb/main.go
git restore cmd/icingadb/main.go

exit "$RET"


$ git bisect run ./828.sh
[ . . . ]
dd0ca8fb072ae8713b7e3f32df5b6417aa0fb7b6 is the first bad commit
commit dd0ca8fb072ae8713b7e3f32df5b6417aa0fb7b6
Author: Alvar Penning <[email protected]>
Date:   Fri Oct 25 10:39:05 2024 +0200

    HA: Insert environment within retryable function

    The HA.insertEnvironment() method was inlined into the retryable
    function to use the deadlined context. Otherwise, this might block
    afterwards, as it was used within HA.realize(), but without the passed
    context.

 pkg/icingadb/ha.go | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)
bisect found first bad commit

[oxzi]

After a bit of debugging together with @yhabteab, we have identified a bug and drafted #878.

[oxzi]

@yhabteab: Could you please rebase again. Thanks :)

[lippserd]

@yhabteab: Could you please rebase again. Thanks :)

Already did.