-
Notifications
You must be signed in to change notification settings - Fork 4
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
Config builder replacements #8
Conversation
… default GC settings (and push our own defaults if no other garbage_collector is not requested).
…ions missing some cases of rendering text after equal sign
…builder test to verify all files were written
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine overall, now that you've fixed the issue with the float64 heap sizes.
I just have one small issue but that is turning into a suggestion since we don't have a written policy around leaving comments in the code 😅 Would be cool if you could remove it though 😬
So based on our PR guidelines, approved ✅
@@ -59,6 +61,7 @@ jobs: | |||
id: docker_build | |||
uses: docker/build-push-action@v3 | |||
with: | |||
load: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Do we plan on creating a proper release process for this? Seems like we're just publishing tags that use the short sha (and latest).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes (we have to so that cass-operator can ship with correct versions), but that would come in a separate PR. We might also want to publish just a static binary for certain platforms if we add more CLI user features.
pkg/config/builder.go
Outdated
case "Shenandoah": | ||
return []string{"-XX:+UseShenandoahGC"} | ||
case "ZGC": | ||
return []string{"-XX:+UseZGC"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: it looks like ZGC requires to add the -XX:+UnlockExperimentalVMOptions
. Maybe we should make this simpler to use for now and add it directly? Otherwise Cassandra just won't start and it gets non trivial to recover from this.
cassandra@test-dc2-default-sts-2:/var/log/cassandra$ cat stderr.log
Error: VM option 'UseZGC' is experimental and must be enabled via -XX:+UnlockExperimentalVMOptions.
Error: The unlock option must precede 'UseZGC'.
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
pkg/config/builder.go
Outdated
// We need another process here.. | ||
continue | ||
} | ||
targetOptions = append(targetOptions, outputVal.Output(v.(string))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: It looks like k8ssandra-operator is turning the heap sizes into bytes instead of string representations such as 512m
. That's breaking this cast to string:
panic: interface conversion: interface {} is float64, not string
goroutine 1 [running]:
github.com/k8ssandra/k8ssandra-client/pkg/config.createServerJVMOptions(0xc000271830, {0x18d45af, 0x12}, {0x18d9084?, 0x18d1786?}, {0x18c745a, 0x7})
/workspace/pkg/config/builder.go:288 +0xe32
github.com/k8ssandra/k8ssandra-client/pkg/config.createJVMOptions(0xc00009f980, {0x18d9084, 0x16}, {0x18c745a, 0x7})
/workspace/pkg/config/builder.go:233 +0x55
github.com/k8ssandra/k8ssandra-client/pkg/config.(*Builder).Build(0xc00015dcc0, {0x0?, 0x0?})
/workspace/pkg/config/builder.go:79 +0xb9
github.com/k8ssandra/k8ssandra-client/cmd/kubectl-k8ssandra/config.(*builderOptions).Run(0x0?)
/workspace/cmd/kubectl-k8ssandra/config/builder.go:82 +0x94
github.com/k8ssandra/k8ssandra-client/cmd/kubectl-k8ssandra/config.NewBuilderCmd.func1(0xc00011a000?, {0x18c5428?, 0x0?, 0x0?})
/workspace/cmd/kubectl-k8ssandra/config/builder.go:49 +0x1d
github.com/spf13/cobra.(*Command).execute(0xc00011a000, {0x2789b18, 0x0, 0x0})
/go/pkg/mod/github.com/spf13/cobra@v1.6.0/command.go:916 +0x862
github.com/spf13/cobra.(*Command).ExecuteC(0xc000004900)
/go/pkg/mod/github.com/spf13/cobra@v1.6.0/command.go:1040 +0x3bd
github.com/spf13/cobra.(*Command).Execute(0x1b25880?)
/go/pkg/mod/github.com/spf13/cobra@v1.6.0/command.go:968 +0x19
main.main()
/workspace/cmd/kubectl-k8ssandra/main.go:18 +0xed
This is intended to replace the cass-config-builder for Cassandra 4.1.x and up.
https://gist.github.com/burmanm/86970c2e8ef299d7b0de0616d31d5b90
Fixes k8ssandra/cass-operator#512