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

feat: compaction support #282

Merged
merged 3 commits into from
May 9, 2024
Merged

Conversation

TylerGillson
Copy link
Contributor

@TylerGillson TylerGillson commented Mar 5, 2024

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

Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
@TylerGillson TylerGillson marked this pull request as ready for review March 5, 2024 21:02
@TylerGillson TylerGillson requested a review from a team as a code owner March 5, 2024 21:02
@brandond
Copy link
Member

brandond commented Mar 5, 2024

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

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

brandond commented Mar 5, 2024

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

TylerGillson commented Mar 5, 2024

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

brandond commented Mar 6, 2024

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.

pkg/client/client.go Outdated Show resolved Hide resolved
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
@brandond brandond merged commit edd8f35 into k3s-io:master May 9, 2024
3 checks passed
@TylerGillson TylerGillson deleted the feat/compact branch May 9, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants