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

interface: add APIs for rebalancing #912

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaelawyu
Copy link
Contributor

Description of your changes

This PR adds APIs for supporting rebalancing attempts in Fleet.

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

N/A

Special notes for your reviewer

N/A

@michaelawyu
Copy link
Contributor Author

Internal only: please also see the mini UX doc for further details about this API (ping me if a link hasn't been sent).

//
// The execution of the rebalacing operation is subject to the rollout strategy defined
// in the ClusterResourcePlacement object. This behavior is akin to editing the placement policy
// of the ClusterResourcePlacement object; disruption budget would not apply to this operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

"disruption budget would not apply to this operation" Why not? Is that because customers don't want disruption budget to be enforced, or we can't achieve it?

Copy link
Contributor

Choose a reason for hiding this comment

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

We are following K8s behavior https://kubernetes.io/docs/concepts/workloads/pods/disruptions/. PDB only protect against disruptions like eviction API. Also, since rebalance follows the same rollout logic in CRP, it won't violate the minimum number of copies policy

Copy link
Contributor

Choose a reason for hiding this comment

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

Disruption budget applies to all voluntary disruptions including "updating a deployment's pod template causing a restart", except for a few like deleting a deployment or a pod directly.

I thing this rebalance API belongs to voluntary disruptions.

Could you elaborate more on the k8s behavior you have in mind?

// * True: the rebalancing attempt is valid.
// * False: the rebalancing attempt is invalid; it might be targeting a CRP that does not
// exist yet, or has been marked for deletion.
// Note thate this ia a terminal state; once the rebalancing attempt is deemed invalid,
Copy link
Contributor

Choose a reason for hiding this comment

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

ia -> is

//
// The execution of the rebalacing operation is subject to the rollout strategy defined
// in the ClusterResourcePlacement object. This behavior is akin to editing the placement policy
// of the ClusterResourcePlacement object; disruption budget would not apply to this operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

We are following K8s behavior https://kubernetes.io/docs/concepts/workloads/pods/disruptions/. PDB only protect against disruptions like eviction API. Also, since rebalance follows the same rollout logic in CRP, it won't violate the minimum number of copies policy

// Executed rebalancing attempts might be kept around for a while for auditing purposes; the
// Fleet controllers might have a TTL set up for such objects and will garbage collect them
// automatically. For further information, see the Fleet documentation.
type ClusterResourcePlacementRebalance struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type ClusterResourcePlacementRebalance struct {
type PlacementRebalance struct {

//
// The execution of the rebalacing operation is subject to the rollout strategy defined
// in the ClusterResourcePlacement object. This behavior is akin to editing the placement policy
// of the ClusterResourcePlacement object; disruption budget would not apply to this operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Disruption budget applies to all voluntary disruptions including "updating a deployment's pod template causing a restart", except for a few like deleting a deployment or a pod directly.

I thing this rebalance API belongs to voluntary disruptions.

Could you elaborate more on the k8s behavior you have in mind?


// Status is the observed state of the ClusterResourcePlacementRebalance object.
// +optional
Status ClusterResourcePlacementRebalanceStatus `json:"status,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Status ClusterResourcePlacementRebalanceStatus `json:"status,omitempty"`
Status PlacementRebalanceStatus `json:"status,omitempty"`

Copy link
Contributor

@ryanzhang-oss ryanzhang-oss left a comment

Choose a reason for hiding this comment

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

please either close or finish this API.

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.

3 participants