Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve status spec #15

Closed
wants to merge 36 commits into from
Closed

Improve status spec #15

wants to merge 36 commits into from

Conversation

khaosdoctor
Copy link
Contributor

Fixes #9

@arschles
Copy link
Collaborator

@khaosdoctor let me know when you're ready for a review on this. I'll hold off for now since it's a draft.

@khaosdoctor
Copy link
Contributor Author

@arschles Just testing it now, but the code is ready. Will update the status here.

What I did was to create a condition_provider.go file that has a more fluent interface on creating new conditions to the httpso interface (based the types on Keda's original typing. Now the conditions can be created as follows:

condition:= *v1alpha1.CreateCondition(v1alpha1.Error,v1.ConditionFalse,v1alpha1.AppServiceTerminationError).SetMessage(err.Error())
httpso.AddCondition(condition).SaveStatus(logger, rec.Client)

@khaosdoctor khaosdoctor marked this pull request as ready for review February 1, 2021 20:30
khaosdoctor and others added 10 commits February 1, 2021 18:38
@khaosdoctor
Remove unused spec
02863cd 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Remove comments
6d086a5 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Set status types
0c62a74 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@arschles @khaosdoctor
Creating 2 interceptor services (#16)
a89e73f 
* logging

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* Adding helm delete functionality to makefile

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* Splitting up functionality, logs and more

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* checking error fetching services

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* Using the proper service name

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* enabling RBAC on endpoints so the scaler can access interceptor queue sizes

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* logs and TODOs

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* using IPs instead of hostnames in external scaler

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* backing off polling interval

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* logging on external scaler startup

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* removing superfluous logging in scaler

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* port and updating namespaces

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* the right namespace

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* adding grpc reflection

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* Makefile targets for creating/deleting example HTTPScaledObject

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* Removing namespace from example HTTPScaledObject file

it is specified when you add it from the make target

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* adding keda creation logic to Makefile (and install doc)

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* helm delete keda target

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

* Passing queue pinger around as a pointer

Otherwise the queue lengths are always reported as 0

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
@khaosdoctor
Created condition provider
32c946b 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Created condition types
8431ef4 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Fix hardcoded names
e52cd61 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Using new condition provider API
6576302 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Forgot to append to slice
b6f05d6 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Merge branch 'main' into main
322ece1 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Copy link
Contributor Author

Updated all commits due to DCO check, I think it needs to be re-run

@khaosdoctor
Remove runtime object
bc23980 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Update CRD definitions
3127c75 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Added configmaps
34958bd 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Remove temporary object
d830706 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Change deleted to terminated
ff68673 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Remove old files
a86ed2a 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Go FMT
3fdd462 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Update CRD with enums
e3ec719 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Log resource version
841e21f 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Update CRD
d056ea3 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Add resource version logging
e5a5985 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Remove broken make command
43155cf 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Add timestamp to condition
fe3c1e3 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Remove useless unknown condition
d1ed32f 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Remove unknown condition
f528cff 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Fix typo
974dc58 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
arschles
arschles requested changes Feb 2, 2021
View reviewed changes
@khaosdoctor
Receiving context as parameter
cfaff69 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Rename v1 to metav1
c497355 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Remove log message and final get
e303334 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Copy link
Contributor Author

@zroubalik @arschles Regarding the conditions, I think we should follow a different approach on this, please tell me what you think of it.

Instead of using conditions and status as we're doing now (almost as a log) I suggest we use the Kubernetes Event API to post events as logs and have only two conditions:

  • HTTPScaledObjectReady: Will be added when all the resources have been created
  • HTTPScaledObjectNotReady: Will be added when CRD is created or when an update is in progress, which will be distinguished in the reason field and then transitioned to the ready condition again

In the future, we can use the status field to add the ingress host and/or service IP for the user app.

All other logs such as: Creating Deployment, Creating Scaler and so on would be logged as events using the EventRecorder object. Does that make sense?

@zroubalik
Copy link
Member

@khaosdoctor bear in mind, that Events are removed from k8s etcd after some period of time and checking operator log could be pretty anoying as the log could be quite huge :) On the other hand checking Status on some resource is quite easy and the information there is pernament 🤷‍♂️

@khaosdoctor khaosdoctor mentioned this pull request Feb 3, 2021
Closed
@khaosdoctor
Copy link
Contributor Author

@khaosdoctor bear in mind, that Events are removed from k8s etcd after some period of time and checking operator log could be pretty anoying as the log could be quite huge :) On the other hand checking Status on some resource is quite easy and the information there is pernament 🤷‍♂️

Seeing from this perspective you're right, let's stick with the statuses 😅

@khaosdoctor
Commented unused test suite
a885d3f 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Updating controller to 0.8.4
a74babb 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Copy link
Contributor Author

Notice: a74babb upgrades controller version to 0.8.4 so #21 can be closed

@khaosdoctor
Add predicates to avoid infinite loops
7c425a7 
As @coderanger mentioned at the #keda-dev channel, everytime we update the status on the resource it is rescheduled to be reconciled again and it gets
us in an infinite loop unless we use predicates, which needed controller version to be at least on 0.6.4

Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Fix role name
0db7e73 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Update manager role
10c1448 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Go fmt
2b53940 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor
Copy link
Contributor Author

Ok, PR is done, everything working as expected:

image

@khaosdoctor
Add status ready condition
1c83ed0 
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@khaosdoctor khaosdoctor mentioned this pull request Feb 3, 2021
Closed
@khaosdoctor
Copy link
Contributor Author

Hey everyone. I'll close this PR because I'll open it in another branch so it does not make the development of new features blocked by it.

@khaosdoctor khaosdoctor closed this Feb 4, 2021
This was referenced Feb 4, 2021
Closed
Merged
kingdonb pushed a commit to kingdonb/http-add-on that referenced this pull request Jun 12, 2023
Merge pull request kedacore#15 from kingdon-ci/renovate/fluxcd-flux2-0.x
d16c340 
chore(deps): update dependency fluxcd/flux2 to v0.39.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik left review comments

@arschles arschles arschles requested changes

@ahmelsayed ahmelsayed Awaiting requested review from ahmelsayed

@tomkerkhove tomkerkhove Awaiting requested review from tomkerkhove

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve the status field of HTTPScaledObjects
3 participants
@khaosdoctor @arschles @zroubalik