Create node objects and bind pods to nodes #215

prateekgogia · 2021-02-02T23:40:44Z

Issue #, if available:

Description of changes:
Create node objects in kubernetes and bind pod to node.

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

cmd/controller/main.go

ellistarn · 2021-02-02T23:47:29Z

pkg/cloudprovider/aws/fleet/nodefactory.go

@@ -36,6 +36,9 @@ type NodeFactory struct {
 func (n *NodeFactory) For(ctx context.Context, instanceIds []*string) ([]*v1.Node, error) {
 	// Backoff retry is necessary here because EC2's APIs are eventually
 	// consistent. In most cases, this call will only be made once.
+	if len(instanceIds) == 0 {


Curious why you needed to add this? For libraries, I try to avoid "overly defensive" code where every function protects against any possible inputs. Is it possible to just ensure that instanceIds is never empty at the callsite?

If the instanceIds are empty, DescribeInstances returns list of all instances in the region. We should not be calling DescribeInstances with an empty list.

IMO if createFleetOutput.Instances is nil, something has gone wrong. Instead, does it make more sense to include this in your error handling of the CreateFleet response?

if err != nil
if len(createFleetOutput.Errors) != 0
if len(createFleetOutput.Instances) == 0

pkg/controllers/provisioner/v1alpha1/allocation/greedyallocator.go

pkg/controllers/provisioner/v1alpha1/controller.go

ellistarn · 2021-02-03T00:02:39Z

pkg/cloudprovider/aws/fleet/capacity.go

@@ -76,8 +76,8 @@ func (c *Capacity) Create(ctx context.Context, constraints *cloudprovider.Capaci
 			}},
 		}},
 	})
-	if err != nil {
-		return nil, fmt.Errorf("creating fleet, %w", err)
+	if err != nil || len(createFleetOutput.Errors) > 0 {


Wow this is such a funky API. Nice catch. I think this error message may end up being confusing. Perhaps we can split into two if statements?

if err != nil { return ... "creating fleet %w" } if len(createFleetOutput.Errors) { return ... "with create fleet errors %w" }

prateekgogia · 2021-02-03T00:14:33Z

Addressed the comments. PTAL.

ellistarn · 2021-02-03T00:37:19Z

cmd/controller/main.go

@@ -69,15 +68,20 @@ func main() {
 	metricsClientFactory := metricsclients.NewFactoryOrDie(options.PrometheusURI)
 	autoscalerFactory := autoscaler.NewFactoryOrDie(metricsClientFactory, manager.GetRESTMapper(), manager.GetConfig())

-	if err := manager.Register(
+	client, err := corev1.NewForConfig(manager.GetConfig())
+	log.PanicIfError(err, "creating client err")


nit: this will result in a weird error message.

ellistarn · 2021-02-03T00:40:37Z

pkg/cloudprovider/aws/fleet/capacity.go

+		return nil, fmt.Errorf("createFleetErrors %v", createFleetOutput.Errors)
+	}
+	if len(createFleetOutput.Instances) == 0 {
+		return nil, fmt.Errorf("no instance info in the createFleetOutput")


try to avoid error messages that refer to variable names (i.e. createFleetOutput). Instead, try to make them as plain english as possible w.r.t the concepts. In this example, I'd probably do:

"create fleet returned 0 instances"

Keep in mind, our error style is to wrap and contextualize the message with a bunch of commas, so in context, this will look something like

"failed to reconcile, while allocating capacity, create fleet returned 0 instances"

ellistarn · 2021-02-03T00:42:35Z

pkg/cloudprovider/aws/fleet/capacity.go

+		return nil, fmt.Errorf("creating fleet %w", err)
+	}
+	if len(createFleetOutput.Errors) > 0 {
+		return nil, fmt.Errorf("createFleetErrors %v", createFleetOutput.Errors)


Likewise. I'd do something like "got errors while creating fleet, %v".

Now that I think a bit further will this leak instances if any were successful but we got errors?

😲 Not sure if that can happen, will have to check, will need to either delete them or account for them some how.

A TODO is plenty

ellistarn

Looking forward to the demo!

* Create node objects and bind pods to nodes * fix error check * fix error check

* 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]>

* 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]>