Add SchedulerHints to server controller #632
Conversation
winiciusallan
left a comment
winiciusallan
left a comment
There was a problem hiding this comment.
Looks good overall! Great job! Just a few comments. Let me know what you think.
internal/controllers/server/tests/server-create-full/00-create-resource.yaml
Show resolved
Hide resolved
eshulman2
force-pushed
the
shints
branch
10 times, most recently
from
7b0df63 to
bdf5e4f
Compare
| // will be created in the server group. | ||
| // +optional | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="serverGroupRef is immutable" | ||
| ServerGroupRef *KubernetesNameRef `json:"serverGroupRef,omitempty"` |
There was a problem hiding this comment.
I agree this should move in the ServerSchedulerHints struct, however, have you considered making this change backward compatible? This would require leaving the field at this level, that we mark as deprecated, and add some glue code to copy its value if the field in ServerSchedulerHints is unset.
Perhaps it's also a good time to introduce more backward incompatible changes in preparation for the next major release 🤷
There was a problem hiding this comment.
I was wondering that too. I think it makes sense. Also, we don't need to wait until the next major for get closer to CAPO parity.
We can create a issue so we don't lose the track.
There was a problem hiding this comment.
TBH I wasn't sure what is the best approach myself, I mainly wanted to avoid having to consolidate situations where the field was set in both making it confusing to use those fields. I believe that if we are on the verge of a new major we should keep it non-backward compatible and note that on the release and docs. In case the next major release is further away maybe we should have backward compatibility but I suspect this might be confusing to use and might result in unexpected results for costumers. I tend to lean towards keeping it non-backward compatible but I don't have a strong objections for making it BC either.
There was a problem hiding this comment.
Well, I can't provide enough feedback here, because I don't know the planned ORC release schedule, but since there are few changes for a new major release, I believe it will take a little longer.
I'll leave it up to both of you.
There was a problem hiding this comment.
Let's see what other potentially breaking changes we need to introduce for the CAPO feature parity work before making a decision.
eshulman2
force-pushed
the
shints
branch
2 times, most recently
from
f6e8392 to
8894f8f
Compare
eshulman2
force-pushed
the
shints
branch
6 times, most recently
from
4bc925c to
54a3ad6
Compare
eshulman2
force-pushed
the
shints
branch
2 times, most recently
from
ce7d860 to
d979876
Compare
There was a problem hiding this comment.
Thanks for your effort, @eshulman2!
To me, it is very close to a merge state, I just left a comment on the dependency that you added, let me know what you think.
mandre
left a comment
mandre
left a comment
There was a problem hiding this comment.
I believe this is almost ready. I would still like to keep this for a little longer in the queue to make up my mind about the breaking/non-breaking change.
| // will be created in the server group. | ||
| // +optional | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="serverGroupRef is immutable" | ||
| ServerGroupRef *KubernetesNameRef `json:"serverGroupRef,omitempty"` |
There was a problem hiding this comment.
Let's see what other potentially breaking changes we need to introduce for the CAPO feature parity work before making a decision.
- Add SchedulerHints to server controller - enable required nova filters for testing hints NOTE! this change MOVED the ServerGroupRef inside the ServerSchedulerHints
3 participants
Add schedulerHints field with support for:
NOTE! this change MOVED the ServerGroupRef inside the ServerSchedulerHints