ScheduledCapacity Metrics Producer API #194

njtran · 2021-01-12T19:58:17Z

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ellistarn · 2021-01-12T19:59:28Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

@@ -33,7 +33,7 @@ type MetricsProducerSpec struct {
 	ReservedCapacity *ReservedCapacitySpec `json:"reservedCapacity,omitempty"`
 	// ScheduledCapacity produces a metric according to a specified schedule.
 	// +optional
-	ScheduledCapacity *ScheduledCapacitySpec `json:"scheduledCapacity,omitempty"`
+	ScheduledCapacity *Schedule `json:"scheduledCapacity,omitempty"`


Should we also rename this variable and the json name? Does capacity still make sense?

For me, just schedule for the json does not scream MetricsProducer. I like scheduledCapacity here more.

Agree with Ellis, scheduleCapacity in Json doesn't sound correct. Since, this is user facing it might be confusing to some.

scheduledCapacity in any sense doesn't make sense, specifically because it has nothing to do with capacity. It could be used to schedule scaling for pods, nodes, birds, shoes, etc. Unless you have good reason to diverge, I'd keep the user facing field names and internal field names more or less the same. If "scheduled metrics producer" sounds odd to you, what sounds more correct?

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

ellistarn · 2021-01-12T20:49:13Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

+}
+
+// TimePattern is a strongly-typed version of crontabs
+type TimePattern struct {


I'd just call this struct Pattern

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

ellistarn · 2021-01-12T20:49:42Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

+	// List of Numbers 1-12 or
+	// List of 3-letter abbreviations i.e. "JAN, Feb, mar" or
+	// List of full months i.e. "January, february, MARCH"
+	Months *string `json:"month,omitempty"`


And this one

ellistarn · 2021-01-12T20:50:24Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

+	Minutes *string `json:"minute,omitempty"`
+	Hours   *string `json:"hour,omitempty"`
+	Days    *string `json:"day,omitempty"`
+	// List of Numbers 1-12 or


IMO we should limit this with "one clear way". I'd probably lean the fully spelled out January, Februrary, March, etc.. We can validate that. Let's minimize low impact work like supporting fuzzy matching here :P

I think I'd rather go with Jan, Feb, Mar format, as that is what Crontabs do as well. I agree with limiting options.

As long as there's a single, simple way to do things :)

ellistarn · 2021-01-12T20:51:06Z

pkg/apis/autoscaling/v1alpha1/metricsproducer_status.go

+	CurrentRecommendation *int32 `json:"currentRecommendation,omitempty"`
+
+	// The time the NextRecommendation's schedule will match
+	NextChangeTime *apis.VolatileTime `json:"nextChangeTime,omitempty"`


NextChangeTime -> NextTime?

I agree that it shouldn't be NextChangeTime, after thinking about it. I don't quite agree with NextTime. How about NextValueTime?

ellistarn · 2021-01-12T20:51:10Z

pkg/apis/autoscaling/v1alpha1/metricsproducer_status.go

@@ -48,6 +48,14 @@ type QueueStatus struct {
 }

 type ScheduledCapacityStatus struct {
+	// The current recommendation - the metric the MetricsProducer is emitting
+	CurrentRecommendation *int32 `json:"currentRecommendation,omitempty"`


Recommendation -> Value?

I like Value.

ellistarn

Excellent work. Enums discussed offline

prateekgogia · 2021-01-12T23:16:12Z

pkg/apis/autoscaling/v1alpha1/metricsproducer_status.go

+	NextValueTime *apis.VolatileTime `json:"nextValueTime,omitempty"`
+
+	// The next recommendation for the metric
+	NextValue *int32 `json:"nextRecommendation,omitempty"`


As a user if I will see the status, I am guessing I will see the json fields nextValueTime and nextRecommendation. It will be good to be consistent with something like -
nextRecommendationTime, nextRecommendation, or
nextRecommendationAt, nextRecommendation, or
nextUpdateAt, nextUpdateValue

prateekgogia · 2021-01-13T00:32:50Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

@@ -33,7 +33,7 @@ type MetricsProducerSpec struct {
 	ReservedCapacity *ReservedCapacitySpec `json:"reservedCapacity,omitempty"`
 	// ScheduledCapacity produces a metric according to a specified schedule.
 	// +optional
-	ScheduledCapacity *ScheduledCapacitySpec `json:"scheduledCapacity,omitempty"`
+	ScheduledCapacity *Schedule `json:"scheduledCapacity,omitempty"`


Agree with Ellis, scheduleCapacity in Json doesn't sound correct. Since, this is user facing it might be confusing to some.

JacobGabrielson · 2021-01-13T01:25:21Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

+	// +optional
+	Timezone *Timezone `json:"timezone,omitempty"`
+	// A schedule defaults to this value when no behaviors are active
+	DefaultReplicas int32 `json:"defaultReplicas"`
 }

 // ScheduledBehavior defines a crontab which sets the metric to a specific replica value on a schedule.


I wouldn't say crontab in the comment

JacobGabrielson · 2021-01-13T01:28:36Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

+	End      *TimeConfig `json:"end"`
+}
+
+type TimeConfig struct {


We should avoid having things called SomethingConfig if possible ... I'd call this TimeMatcher

JacobGabrielson · 2021-01-13T01:29:16Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

+// List of 3-letter abbreviations i.e. "Mon, Tue, Wed"
+type Weekdays string
+
+// Pattern is a strongly-typed version of crontabs


Can you add doc on what it means if a field is omitted? Also what are the rules about which fields can or cannot be specified?

Added. Let me know if that covers what you meant.

JacobGabrielson · 2021-01-13T01:30:01Z

pkg/apis/autoscaling/v1alpha1/metricsproducer_status.go

+	// The current recommendation - the metric the MetricsProducer is emitting
+	CurrentValue *int32 `json:"currentRecommendation,omitempty"`
+
+	// The time where CurrentValue will switch to NextValue


I'd say "time in the future when"

ellistarn · 2021-01-13T01:43:22Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

+
+type TimeConfig struct {
+	// Specify only one of Crontab or Pattern
+	Crontab *string  `json:"crontab,omitempty"`


Let's punt on Crontab for now and implement it in a followon.

ellistarn · 2021-01-13T01:44:22Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

+	End      *TimeConfig `json:"end"`
+}
+
+type TimeConfig struct {


I wouldn't have this struct at all, and simply just inline crontab or pattern. They're both string, at the end of the day.

ellistarn

LGTM

ellistarn · 2021-01-14T01:24:24Z

pkg/apis/autoscaling/v1alpha1/metricsproducer.go

 	// Behaviors may be layered to achieve complex scheduling autoscaling logic
 	Behaviors []ScheduledBehavior `json:"behaviors"`
+	// Defaults to UTC. Users will specify their schedules assuming this is their timezone


Should default to the cluster's local timezone.

Are you sure you're able to set a timezone for a cluster?

prateekgogia · 2021-01-14T03:01:28Z

LGTM, nice work!

* Switches presubmits to track main. (#139) * Switches presubmits to track main. * Address https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ * Added WORKING_GROUP (#140) * Readme edits + organization docs (#142) * readme and admin docs * faq edits * doc edits * edits and organization * readme edits * readme edit * Remove Queue reference from NodeGroup overprovisioning example (#145) * correct a typo in README.md (#143) * readme typo (#146) * Capitalise TERMS.md link (#147) * Update OWNERS (#150) * Update README.md (#149) * Fix installation to target `main` branch instead of `master` (#153) * Cleaned up FAQs, Working Group Invite, and other typos (#148) * Update OWNERS (#155) add tabern * Switches to Allocatable from Capacity and exposes more human readable (#154) values in capacity reservation status. * Update FAQs.md (#156) * Update DESIGN.md (#158) Fixed typos * Update README.md (#160) * Update README.md * Update README.md * Update README.md * Initial github banner image (#162) * Initial github banner image * Small image fix and add to readme * Remove repo name from top of readme * Smaller readme banner. Add repo preview * HA Bottoms out at 1 DesiredReplica for Utilization and Value metrics (#161) * Update README.md (#163) * changed consumer context from customer to user (#168) * changed consumer context from customer to user (#167) * Reorganized OWNERS (#169) * grammar & syntax updates in docs/DESIGN.md (#170) * Add unit test for scalable node group controller (#157) * Add unit test for scalable node group controller * Add some more tests * Add error checks for SetReplicas call * Fix Unit tests * rename fakeError * update tests * rename a few things * rename fake error object * Fixed pointers comparison * Simplify tests a little * Updated release image to use public ecr (#171) * Improved release automation. Cut pre-release for v0.1.1 (#174) * Changed desiredReplicas to be a pointer to allow for displaying scale to (#172) * Changed desiredReplicas to be a pointer to allow for displaying scale to zero. * Updated tests * Created a minimal Helm Chart (#175) * Implemented a minimal helm chart with release automation using Github Pages * Removed stray files * Updated chart * Updated install instructions (#176) * Updated install instructions * Fixed a typo * Improved calendar invite for Working Group (#177) * Add working group meeting notes (#180) * Add working group meeting notes (#182) * Create NOTICE (#178) * Fix errors in hack/quick-install.sh (#185) Fix issues: 1/ Command helm upgrade --install karpenter charts/karpenter fails. 2/ Uninstall: Current order of operation leaves deployment: karpenter and statefulset: prometheus-karpenter-monitor remaining: * Fixed a few typos in developer guide (#187) The developer guide hadn't been updated after some of the make commands changed name. Also corrected an environment variable name. * Add notes from working group meeting (#192) * ScheduledCapacity API (#194) * fixed a small typo in metricsproducer (#195) * Add notes from working group meeting (#196) * Implemented Scheduled Capacity Metrics Producer (#200) * fixing merge problems * Implemented Scheduled Capacity Metrics Producer * Design Doc for ScheduledCapacity (#197) * Delete karpenter.ics (#204) Deleted in ics invite favor of google calendar. * enable build on provisioner branch too (#206) * Validations for MetricsProducers (#211) * MetricsProducer Schedule Validation * cleaned up validation efforts for non-schedules * Added Validation checks in all resource reconcile loops * Add working group notes (#217) * add working group notes * add some more notes to WG notes * Rebase provisioner-work into main. (#219) * Provisioner api (#198) * First commit for provisioner spec * remove comented code * Added a simple controller skeleton (#199) * Adding capacity API client (#202) * Adding fleet API client * Add capacity provisioner and refine the interface definitions * Provisioner skeleton code (#203) * Wired up controller to main and trivial test (#205) * More tweaks to test wiring (#207) * Add end to end functionality for pending pods (#209) * add end to end functionality * simplify functions after PR feedback * simplify create fleet method * fix comments, remove new line char and remove private create method * Implemented cloud provider initialization (#208) * Implemented Cloud Provider initialization and discovery logic. * Implemented cloud provider initialization * Cloudprovider improvements: Lazy Initialization, IAM Separation, and Topology Support (#213) * Create node objects and bind pods to nodes (#215) * Create node objects and bind pods to nodes * fix error check * fix error check * Reoriented Capacity CloudProvider around BinPacking (#214) * Setup packing interface and refactored instance provider for launching constrained instances (#216) * Pausing on packing * Setup packing interface and refactored instance provider for launching constrained instances Co-authored-by: Prateek Gogia <[email protected]> * Defaults for Metrics Producers (#222) * Refactor cloud provider, add packing package (#221) * refactor cloud provider, add packing package * Refactored allocator and improve logging (#218) * Skeleton code for reallocator (#235) * Generated v0.1.2 release Co-authored-by: Nate Taber <[email protected]> Co-authored-by: Guy Templeton <[email protected]> Co-authored-by: dherman <[email protected]> Co-authored-by: Guy Templeton <[email protected]> Co-authored-by: Tom Kerkhove <[email protected]> Co-authored-by: alethatarn <[email protected]> Co-authored-by: Justin Garrison <[email protected]> Co-authored-by: Cameron Senese <[email protected]> Co-authored-by: Prateek Gogia <[email protected]> Co-authored-by: Prateek Gogia <[email protected]> Co-authored-by: Henri Yandell <[email protected]> Co-authored-by: Jacob Gabrielson <[email protected]> Co-authored-by: Nick Tran <[email protected]>

Bumps [github.com/samber/lo](https://github.com/samber/lo) from 1.34.0 to 1.37.0. - [Release notes](https://github.com/samber/lo/releases) - [Changelog](https://github.com/samber/lo/blob/master/CHANGELOG.md) - [Commits](samber/lo@v1.34.0...v1.37.0) --- updated-dependencies: - dependency-name: github.com/samber/lo dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

njtran requested review from ellistarn, JacobGabrielson and prateekgogia January 12, 2021 19:58

ellistarn reviewed Jan 12, 2021

View reviewed changes

ellistarn suggested changes Jan 12, 2021

View reviewed changes

prateekgogia reviewed Jan 13, 2021

View reviewed changes

JacobGabrielson suggested changes Jan 13, 2021

View reviewed changes

ellistarn reviewed Jan 13, 2021

View reviewed changes

ellistarn previously approved these changes Jan 14, 2021

View reviewed changes

njtran dismissed ellistarn’s stale review via 0823324 January 14, 2021 01:24

ellistarn reviewed Jan 14, 2021

View reviewed changes

ScheduledCapacity API

056fa99

JacobGabrielson approved these changes Jan 14, 2021

View reviewed changes

njtran merged commit 75a994e into aws:main Jan 14, 2021

njtran deleted the schedulesAPI branch February 25, 2021 01:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ScheduledCapacity Metrics Producer API #194

ScheduledCapacity Metrics Producer API #194

njtran commented Jan 12, 2021

ellistarn Jan 12, 2021

njtran Jan 12, 2021

prateekgogia Jan 13, 2021

ellistarn Jan 13, 2021

ellistarn Jan 12, 2021

ellistarn Jan 12, 2021

ellistarn Jan 12, 2021

njtran Jan 12, 2021

ellistarn Jan 12, 2021

ellistarn Jan 12, 2021

njtran Jan 12, 2021 •

edited

Loading

ellistarn Jan 12, 2021

njtran Jan 12, 2021

ellistarn left a comment

prateekgogia Jan 12, 2021

prateekgogia Jan 13, 2021

JacobGabrielson Jan 13, 2021

JacobGabrielson Jan 13, 2021

JacobGabrielson Jan 13, 2021

njtran Jan 14, 2021

JacobGabrielson Jan 13, 2021

ellistarn Jan 13, 2021

ellistarn Jan 13, 2021

ellistarn left a comment

ellistarn Jan 14, 2021

njtran Jan 14, 2021

prateekgogia commented Jan 14, 2021

ScheduledCapacity Metrics Producer API #194

ScheduledCapacity Metrics Producer API #194

Conversation

njtran commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njtran Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekgogia commented Jan 14, 2021

njtran Jan 12, 2021 •

edited

Loading