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

Add State to ControlPlane.Spec.Crossplane for managing the crossplane state #103

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

sergenyalcin
Copy link
Member

@sergenyalcin sergenyalcin commented Jul 24, 2024

Description of your changes

This PR proposes adding the State API to the ControlPlane.Spec.Crossplane API to manage the crossplane related workloads states.. The idea is to have reconciliation management on resources at the ControlPlane level.

The default value is Running. Other possible value is Paused.

When the Paused value is set, all crossplane and provider workloads in the ControlPlane will be paused/scaled down.

When the Running value is set, all crossplane and provider workloads in the ControlPlane will be restarted/scaled up.

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

Tested with the controller that is responsible of the processing this API field.

@sergenyalcin sergenyalcin marked this pull request as ready for review July 24, 2024 16:13
Copy link
Contributor

@lsviben lsviben left a comment

Choose a reason for hiding this comment

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

Gave it a quick look. Need to check the one pager for a deeper review

apis/spaces/v1beta1/controlplane_types.go Outdated Show resolved Hide resolved
Comment on lines 209 to 210
// ReconciliationPolicy specifies the reconciliation policies on the
// crossplane and the workloads that work on the ControlPlane
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe smt like this:

Suggested change
// ReconciliationPolicy specifies the reconciliation policies on the
// crossplane and the workloads that work on the ControlPlane
// ReconciliationPolicy specifies the reconciliation policies of the
// ControlPlane's Crossplane and providers

Not sure if there are any other workloads then the provider?

Copy link
Member Author

@sergenyalcin sergenyalcin Jul 25, 2024

Choose a reason for hiding this comment

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

@lsviben The idea is pausing all crossplane and provider reconciliations on the CP not other ones. So, these workloads were mentioned.

// ControlPlane
// +kubebuilder:validation:Enum=None;AllCrossplane
// +kubebuilder:default=None
PausedControllers string `json:"pausedControllers,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

PausePolicy as an alterative to PausedControllers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Controller is not a concept in the product, neither in Crossplane.

Copy link
Contributor

Choose a reason for hiding this comment

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

like spec.crossplane.state: Paused | Running

Copy link
Contributor

Choose a reason for hiding this comment

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

And I have a possible 🔴 here: do we know what comes next with simulations? Do we have to stop providers one-by-one? Or keep Crossplane running, but disable all providers? We at least need a plan for that.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, this is not an API we're building for Simulation work, as discussed here, rather something independent. I agree that there could be overlaps, and we should better find an API that could serve for both purposes, but don't think we should we should block this work with simulation work.

Probably @negz is the best person to comment here on whether this API could be needed/useful for simulations or something totally independent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be happy to implement the @sttts's suggestion for this API:

spec.crossplane.state: Paused | Running With Running default.

How does that sound to you @turkenh @lsviben?

Copy link
Member

@negz negz Jul 26, 2024

Choose a reason for hiding this comment

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

Right now I'm "pausing" providers by using a DeploymentRuntimeConfig to scale them to zero replicas. This applies to all providers in the simulated control plane.

In a simulation there's no reason that I can think of to run the provider controller at all. So we want Crossplane core controllers to run, but not provider controllers.

In the design doc I discuss a ConnectedSimulation, which would run all MRs observe only. If we build that, it'll need Providers to run and will need to manipulate their management policy, rather than pausing them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented this -> spec.crossplane.state: Paused | Running Thanks @sttts for the suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. Have approved.

@jeanduplessis jeanduplessis added this to the 1.6.0 milestone Jul 25, 2024
apis/spaces/v1beta1/conditions.go Outdated Show resolved Hide resolved
// ControlPlane
// +kubebuilder:validation:Enum=None;AllCrossplane
// +kubebuilder:default=None
PausedControllers string `json:"pausedControllers,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

To be clear, this is not an API we're building for Simulation work, as discussed here, rather something independent. I agree that there could be overlaps, and we should better find an API that could serve for both purposes, but don't think we should we should block this work with simulation work.

Probably @negz is the best person to comment here on whether this API could be needed/useful for simulations or something totally independent.

@sergenyalcin sergenyalcin changed the title Add ReconciliationPolicy API to ControlPlane for managing the reconciliation/pause policies Add State to ControlPlane.Spec.Crossplane for managing the crossplane state Jul 26, 2024
apis/spaces/v1beta1/conditions.go Outdated Show resolved Hide resolved
apis/spaces/v1beta1/conditions.go Outdated Show resolved Hide resolved
@sergenyalcin sergenyalcin merged commit e4bd1bd into upbound:main Jul 29, 2024
5 checks passed
@sergenyalcin sergenyalcin deleted the pause-reconciliation branch July 29, 2024 08:54
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.

6 participants