-
Notifications
You must be signed in to change notification settings - Fork 95
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: New ScalingSet CRD to deploy isolated interceptors+scalers #1014
base: main
Are you sure you want to change the base?
Conversation
4e793d6
to
6e1700a
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.
I like this approach very much, here is a couple of minor nitpicks from the first pass, I will take a deeper look later today
} | ||
|
||
// HTTPScalingSetStatus defines the observed state of HTTPScalingSet | ||
type HTTPScalingSetStatus struct{} |
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 am sure this is empty only because the PR is still WIP but for discussion sake mentioning controller may want to capture some conditions here, such as:
- unable to create interceptor/scaler
- kube-api rejects some resource creation
- failure to form the
externalScalerURI
- interceptor/scaler are not running with desired replica count and/or are unhealthy
- possibly getting some basic stats from interceptor admin endpoint?
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.
First pass of review
} | ||
|
||
// HTTPScalingSetStatus defines the observed state of HTTPScalingSet | ||
type HTTPScalingSetStatus struct{} |
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.
We should provide some status about the resource, at least Ready
condition
ctx context.Context, | ||
logger logr.Logger, | ||
cl client.Client, | ||
httpss metav1.Object, |
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.
We should consider using duck types instead of metav1.Object here, to add some type safety.
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.
@wozniakjan has also an idea on this.
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.
WDYM? We do almost the same in KEDA with ScaledObjects and ScaledJobs. Would be enough if I validate the type and return an error if the type isn't correct?
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 mentioned to @zroubalik earlier that maybe we could explore type aliasing or trivial type definitions. In this case, both ScalingSets
as well as ClusterScalingSets
seem to have exactly matching structure so maybe we could do something along the lines of either
type ClusterScalingSet ScalingSet
or
type ClusterScalingSet = ScalingSet
I had used the first one with controller-runtime v1 some eons ago successfully and haven't tried it ever since, but could be a good fit for this usecase unless controller-runtime became more strict regarding types.
The implementation went something like this: https://go.dev/play/p/sxZiO_BDw4z
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 didn't know that this works with kubebuilder but I can give a try because yes. both types are exactly the same, just cluster scoped or namespaced
I appreciate your feedback, I'll review it, but don't worry about reviewing the PR for the moment as it's still a WIP and there are a couple of pending stuff, like propagating the status info the the CRD, documentation, more test coverage , etc You can review it if you want, but there can be multiple changes until the final version :) |
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
I'm removing the changes applied to my local branch |
Signed-off-by: Jorge Turrado <[email protected]>
b58f3f4
to
6286ab0
Compare
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
I've been talking about this with @zroubalik and to not lock you with the code refactor, we will agreed with merging this PR as it is (it works actually) and I'll open a followup PR in a few days/weeks (before the release for sure) adding the pending stuff to complete this PR (such as but not exclusive):
|
This feature is great! |
@wozniakjan Hello! |
Provide a description of what has been changed
Checklist
README.md
docs/
directoryFixes #241