Skip to content

Conversation

@eshulman2
Copy link
Contributor

Add schedulerHints field with support for:

  • serverGroupRef - schedule server in a server group
  • differentHostServerRefs / sameHostServerRefs - affinity/anti-affinity with other servers
  • query, targetCell, differentCell, buildNearHostIP - advanced placement options
  • additionalProperties - arbitrary scheduler hints

NOTE! this change MOVED the ServerGroupRef inside the ServerSchedulerHints

Add SchedulerHints to server controller
NOTE! this change MOVED the ServerGroupRef inside the ServerSchedulerHints
Copy link
Contributor

@winiciusallan winiciusallan left a comment

Choose a reason for hiding this comment

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

Looks good overall! Great job! Just a few comments. Let me know what you think.

// able to host the server.
// +kubebuilder:validation:MaxLength:=1024
// +optional
Query *string `json:"query,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we don't need this pointer, since the zero-value isn't useful here. What do you think?

// targetCell is a cell name where the server will be placed.
// +kubebuilder:validation:MaxLength:=255
// +optional
TargetCell *string `json:"targetCell,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for this pointer.

// buildNearHostIP specifies a subnet of compute nodes to host the server.
// +kubebuilder:validation:MaxLength:=255
// +optional
BuildNearHostIP *string `json:"buildNearHostIP,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

And this one too.

Comment on lines +44 to +45
schedulerHints:
serverGroupRef: server-create-full
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add all the available fields for SchedulerHints that you've implemented on this test case, so we can test all working together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver:major Breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants