k3s-io / kine

Run Kubernetes on MySQL, Postgres, sqlite, dqlite, not etcd.
Apache License 2.0
1.49k stars 226 forks source link

feat: compaction support #282

Closed TylerGillson closed 1 month ago

TylerGillson commented 4 months ago

This PR adds support for gRPC compaction requests for all backends. For NATS, compaction is handled by Jetstream and the bucket's History setting, so compaction requests are no-ops.

Additionally, unused params were removed from the LimitedServer's compact and create methods.

Given that the default compaction interval of 5min is not exposed as a configuration option, it can be useful to be able to trigger ad hoc compactions, which partially addresses #230.

Test code

package main

import (
    "context"
    "fmt"
    "os"
    "strconv"

    "go.etcd.io/etcd/etcdserver/etcdserverpb"
    "google.golang.org/grpc"
    "google.golang.org/grpc/credentials/insecure"
)

func main() {
    compactRevision := os.Args[1]
    cr, err := strconv.ParseInt(compactRevision, 10, 64)
    if err != nil {
        panic(err)
    }

    conn, err := grpc.Dial("localhost:2379",
        grpc.WithTransportCredentials(insecure.NewCredentials()),
    )
    if err != nil {
        panic(err)
    }
    defer conn.Close()
    fmt.Println("Connected to kine")

    r := &etcdserverpb.CompactionRequest{
        Revision: cr,
    }
    fmt.Println("Triggering compaction", "revision", r.Revision)

    c := etcdserverpb.NewKVClient(conn)
    resp, err := c.Compact(context.Background(), r)
    if err != nil {
        panic(err)
    }

    fmt.Println("Received compaction response with revision", resp.Header.Revision)
}

Sample output

root@tyler-k11-8277bf2c:/home/demo# ./kine-compact 1755748
Connected to kine
Triggering compaction revision 1755748
Received compaction response with revision 1002
brandond commented 4 months ago

If we enable this, then the built-in compaction loop doesn't serve much purpose. Can we perhaps put this behind a CLI flag, and disable the built-in compaction loop when client-initiated compaction is enabled?

TylerGillson commented 4 months ago

If we enable this, then the built-in compaction loop doesn't serve much purpose. Can we perhaps put this behind a CLI flag, and disable the built-in compaction loop when client-initiated compaction is enabled?

In my use case the default 5m interval is fine 99% of the time. When a certain situation arises, I trigger a couple of manual compactions over a shorter time period, then fall back to the default.

I'm totally open to adding a flag that disables the built-in compaction loop, but would you consider allowing both?

brandond commented 4 months ago

upstream etcd has:

--auto-compaction-retention '0'
  Auto compaction retention length. 0 means disable auto compaction.
--auto-compaction-mode 'periodic'
  Interpret 'auto-compaction-retention' one of: periodic|revision. 'periodic' for duration based retention, defaulting to hours if no time unit is provided (e.g. '5m'). 'revision' for revision number based retention.

right now, kine's behavior is essentially as if it had those flags and it defaulted to --auto-compaction-retention=5m --auto-compaction-mode=periodic

If we are going to allow for manual control over compaction, perhaps we could add those flags, and wire them up appropriately?

I will also note that kine will always refuse to compact the most recent 1000 revisions. That is hardcoded and I wouldn't recommend changing it, at the risk of having nodes miss rows from the database in the case of multiple nodes talking to the same sql db.

TylerGillson commented 4 months ago

@brandond I'm happy to add support for those flags, but supposing they've been added, kine (and etcd) would respect ad hoc compaction requests, right? Other than in the edge case you mentioned.

If that's the case, what are your thoughts on merging this and adding the flags in a follow-up PR? I don't mind adding that here either, just trying to keep the scope of this PR as narrow as possible.

brandond commented 4 months ago

So I just looked at this again and I see that you're adding support for the direct compact grpc method, without modifying the compact transaction that the apiserver sends - the apiserver's automatic compaction should still be a no-op because we always fail the transaction. As long as we're not changing the interaction with the apiserver, I think we're OK.