-
Notifications
You must be signed in to change notification settings - Fork 175
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 proposal for new API v1alpha1.ScyllaDBDatacenter
#1994
Add proposal for new API v1alpha1.ScyllaDBDatacenter
#1994
Conversation
v1alpha1.ScyllaDBDatacenter
v1alpha1.ScyllaDBDatacenter
Marking as WIP after offline discussion to prepare API for new changes. |
c810bd3
to
5234f75
Compare
v1alpha1.ScyllaDBDatacenter
v1alpha1.ScyllaDBDatacenter
No longer WIP. |
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.
Left a couple of comments.
Please review the code snippets for inconsistent indentations, there's a couple of places where they're off.
Also I haven't noticed any mentions of a deprecation plan. Is the existing CRD going to be deprecated at some point? If so, how long are we going to keep it, when are we going to get rid of the conversion controller etc. Otherwise it should be mentioned in the doc either way imo.
3c5c77c
to
d72bd9e
Compare
d72bd9e
to
2e2ba9c
Compare
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.
Thanks for sending the proposal. A few comments to start with, I'll continue tomorrow.
108d01f
to
38ca580
Compare
// sysctls holds the sysctl properties to be applied during initialization given as a list of key=value pairs. | ||
// Example: fs.aio-max-nr=232323 | ||
// +optional | ||
Sysctls []string `json:"sysctls,omitempty"` |
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.
racks/zones could have different machine types or hardware - maybe this should be on the rack level - i guess a similar point can be made for some other fields on this level
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 don't think sysctls should be part of ScyllaDatacenter at all. These should belong to NodeConfig which sets up the nodes. I added them to be backward compatible.
Maybe we should get rid of it, the same way we got rid of Manager tasks and until a better alternative is available, have a support via annotation? WDYT?
We need to have any support because Scylla usually requires aio-max-nr to be increased.
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 haven't look deep enough to see whether some sysctls make sense per container, but I see that the majority should be per node. So maybe this would go away with #749. I agree that internal annotation let's us defer the decision. (Btw. it would be good to add all those reasons to the annotation const definitions, so we know when we can drop it.)
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 suppose we should file issues to track deprecation of fields that you are moving to annotation, unless they already have one like hostNetworking.
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.
Once we have everything finalized I will create issues for missing parts
38ca580
to
4f9039b
Compare
4f9039b
to
1fc46f3
Compare
a4b7bbb
to
c8c4554
Compare
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.
last nits / typos, rest lgtm
thanks for the updates
fca5d7b
to
7354b55
Compare
lgtm |
@tnozicka: GitHub didn't allow me to assign the following users: rzetelski. Note that only scylladb members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This proposal aims to introduce new API called `v1alpha1.ScyllaDBDatacenter` that will replace existing `v1.ScyllaCluster`. The new API is going to be backward compatible and will contain fixes for mistakes we did in the past and new improvements.
7354b55
to
dcd4174
Compare
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.
/lgtm
Thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rzetelskik, tnozicka, zimnx The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This proposal aims to introduce new API called
v1alpha1.ScyllaDBDatacenter
that will eventually replace existingv1.ScyllaCluster
. The new API offers the same features as existing one, but it will contain fixes for mistakes we did in the past, as well as several UX improvements.This PR is a place for discussion. If you find something wrong or something that could be improved within current API please raise it here.
Resolves #1978