-
Notifications
You must be signed in to change notification settings - Fork 266
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
Add easy support for single node sqlite with kine #5954
Add easy support for single node sqlite with kine #5954
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5954 +/- ##
===========================================
+ Coverage 9.86% 26.09% +16.22%
===========================================
Files 32 32
Lines 2686 2698 +12
===========================================
+ Hits 265 704 +439
+ Misses 2399 1948 -451
- Partials 22 46 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pkg/cli/cmds/server.go
Outdated
@@ -32,6 +32,11 @@ var ( | |||
Usage: "(components) Enable rke2 default cloud controller manager's service controller", | |||
EnvVar: "RKE2_ENABLE_SERVICELB", | |||
}, | |||
&cli.BoolTFlag{ | |||
Name: "cluster-init", |
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.
While the wording comes over from K3s, I don't thing that there is any reason for use to reuse a confusing name as cluster-init
. Its even more confusing that you need to "set it to false" for something to happen. Something like sqlite
or single-node-sqlite
could be clearer about what we intend.
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.
After more discussing, I've come around to just keeping it as is. The only people who will actually want/use this feature are likely coming from K3s anyways.
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.
@dereknola I think it is still a valid discussion, it isn't a very straightforward cli flag to be fair.
In my opinion after some thoughts is that we should stick with the single-node-sqlite
dec16e8
to
feee025
Compare
pkg/cli/cmds/server.go
Outdated
@@ -32,6 +32,11 @@ var ( | |||
Usage: "(components) Enable rke2 default cloud controller manager's service controller", | |||
EnvVar: "RKE2_ENABLE_SERVICELB", | |||
}, | |||
&cli.BoolFlag{ | |||
Name: "single-node-sqlite", |
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.
If we're not going to reuse the existing cluster-init flag then we need to be thoughtful about what we call this new flag. --single-node-sqlite isn't quite correct, as you can still have multiple nodes - it's just that you can only have a single server.
Would it break things if --disable-etcd without setting a --server address to join enabled sqlite, in the absence of etcd files on disk?
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.
when we receive disable-etcd
without --server
we return this error here
FATA[0000] invalid flag use; --server is required with --disable-etcd
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.
maybe we could set as single-server-sqlite
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.
when we receive disable-etcd without --server we return this error here
Right, but what if we made that the combination to enable sqlite, instead of adding a new flag?
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.
It work very well, I updated my PR with this combination.
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.
I like this combination as well, nice reuse of existing flags.
feee025
to
8660edb
Compare
8660edb
to
f12aa31
Compare
Signed-off-by: Vitor Savian <[email protected]> Disable etcd when single node with kine Signed-off-by: Vitor Savian <[email protected]>
f12aa31
to
fc354de
Compare
Proposed Changes
Types of Changes
Verification
you will need to
disable etcd
and not send theserver
flagTesting
Linked Issues
User-Facing Change
Further Comments