-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃悰 Fix node deletion under managed control planes #3673
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -197,6 +197,16 @@ func GetControlPlaneMachinesFromList(machineList *clusterv1.MachineList) (res [] | |
return | ||
} | ||
|
||
// IsExternalManagedControlPlane returns a bool indicating whether the control plane referenced | ||
// in the passed Unstructured resource is an externally managed control plane such as AKS, EKS, GKE, etc. | ||
func IsExternalManagedControlPlane(controlPlane *unstructured.Unstructured) bool { | ||
managed, found, err := unstructured.NestedBool(controlPlane.Object, "status", "externalManagedControlPlane") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we document somewhere that manage control plane implementation should have this field (might be in https://cluster-api.sigs.k8s.io/developer/architecture/controllers/control-plane.html#required-status-fields) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 this isn't part of the contract currently AFAIK cc @alexeldeib There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 updating the documentation, just a note though the field is completely optional and there is a fallback to the current behavior during delete There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. What are thoughts on making this new field be an optional part of the contract to avoid a breaking change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an unusual case in that the field is really required for managed control planes like AKS, EKS, GKE, etc otherwise the Machine controller will suffer this pretty bad bug (instance termination without draining nodes), but not required at all for KubeadmControlPlane. No other optional status fields currently appear to modify functionality and are purely informational. If we make it a new required status field though, I think this would need to wait for a new minor version to be released. I'd like to get the bugfix released sooner rather than later if possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm 馃憤 to optional. Please update the control plane architecture doc linked above and add it to the list of optional status fields, assuming everyone else agrees. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if err != nil || !found { | ||
return false | ||
} | ||
return managed | ||
} | ||
|
||
// GetMachineIfExists gets a machine from the API server if it exists. | ||
func GetMachineIfExists(c client.Client, namespace, name string) (*clusterv1.Machine, error) { | ||
if c == nil { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small note: the AKS implementation doesn't have a separate control plane provider, see https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/19e80de4a7e92082ca0c5eb672e486801da23766/templates/cluster-template-aks.yaml
Would that still work as a field on the AzureManagedControlPlane https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/995da71c822c201cfa72cf99ae1b96652544e352/exp/api/v1alpha3/azuremanagedcontrolplane_types.go#L79 as part of the Azure infra provider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I think it should since we're using controlPlaneRef the same way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CecileRobertMichon Yes, as long as you reference the
AzureManagedControlPlane
in the cluster'scontrolPlaneRef
field you should be able to use the status in your resource as well. Since the check here in CAPI is using the unstructured API, the underlying type is not important. Whatever object is referenced bycontrolPlaneRef
just needs to have the status fieldexternalManagedControlPlane
if there are no control planeMachine
resources in the cluster.