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.

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

Create ScalingSchedule collector #315

Merged
merged 1 commit into from
May 21, 2021
Merged

Conversation

jonathanbeber
Copy link
Contributor

@jonathanbeber jonathanbeber commented May 21, 2021

One-line summary

Create ScalingSchedule and ClusterScalingSchedule collectors.

Description

This commit adds two new collectors to the adapter:

  • ClusterScalingScheduleCollector; and
  • ScalingScheduleCollector

Also, it introduces the required collectors plugins, initialization
logic in the server startup, documentation and deployment example
(including the helm chart). A new config flag is created,
-scaling-schedule, and allows to enable and to disable the collection
of such metrics. It's disabled by default.

This collectors are the required logic to utilise the CRDs introduced in
the #284 pull request. It makes use of the Kubernetes go-client
implementations of a Store and Reflector.

Types of Changes

What types of changes does your code introduce? Keep the ones that apply:

  • New feature (non-breaking change which adds functionality)

Review

List of tasks the reviewer must do to review the PR

  • Tests
  • Documentation
  • CHANGELOG

Deployment Notes

None by default, the feature is disabled by default.

@jonathanbeber
Copy link
Contributor Author

It's related also to kubernetes/kubernetes#50569 and kubernetes/kubernetes#49931

Copy link
Contributor

@mikkeloscar mikkeloscar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really great! I only added a few nitpicks.

Didn't you need to add some annotations to types.go for correctly generating the CRD clients or was this not needed after all? (e.g. +genclient:nonNamespaced)

pkg/collector/scaling_schedule_collector.go Outdated Show resolved Hide resolved
pkg/collector/scaling_schedule_collector.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pkg/server/start.go Show resolved Hide resolved
@jonathanbeber
Copy link
Contributor Author

Thanks a lot for the quick review, @mikkeloscar.

Didn't you need to add some annotations to types.go for correctly generating the CRD clients or was this not needed after all? (e.g. +genclient:nonNamespaced)

Not for us, but if someone else wants to use the clients, it would be "broke". I have a branch with that ready, just didn't want to make this PR even bigger since it won't change anything. In this PR we use the REST client.

This commit adds two new collectors to the adapter:
- ClusterScalingScheduleCollector; and
- ScalingScheduleCollector

Also, it introduces the required collectors plugins, initialization
logic in the server startup, documentation and deployment example
(including the helm chart). A new config flag is created,
`-scaling-schedule`, and allows to enable and to disable the collection
of such metrics. It's disabled by default.

This collectors are the required logic to utilise the CRDs introduced in
the #284 pull request. It makes use of the kubernetes go-client
implementations of a [Store][0] and [Reflector][1].

[0]: https://pkg.go.dev/k8s.io/client-go/tools/cache#Store
[1]: https://pkg.go.dev/k8s.io/client-go/tools/cache#Reflector

Signed-off-by: Jonathan Juares Beber <[email protected]>
@mikkeloscar
Copy link
Contributor

👍

1 similar comment
@jonathanbeber
Copy link
Contributor Author

👍

@jonathanbeber jonathanbeber merged commit 6f9aba8 into master May 21, 2021
@jonathanbeber jonathanbeber deleted the time-based-scaling branch May 21, 2021 14:46
Comment on lines +242 to +248
location, err := time.LoadLocation(schedule.Period.Timezone)
if schedule.Period.Timezone == "" || err != nil {
location, err = time.LoadLocation(defaultTimeZone)
if err != nil {
return nil, fmt.Errorf("unexpected error loading default location: %s", err.Error())
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read this correctly it would silently fallback to defaultTimezone in case of invalid timezone (e.g. typo) which could be a surprise for the user.

The if schedule.Period.Timezone == "" check does not make much sense as time.LoadLocation would return UTC for the empty timezone according to the docs https://pkg.go.dev/time#LoadLocation. Besides that I think default timezone (if still used) should be UTC as it is common for other timestamps in k8s.

IMO timezone should be either mandatory or empty by default which would result in UTC timezone and the logic should fail fast on error without any fallback unless I am missing something else here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timezone is required by CRD

@JCMais
Copy link

JCMais commented Nov 14, 2021

how to use the helm chart added here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants