-
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🏃 Update status.controlPlaneInitialized from cluster controller #1356
🏃 Update status.controlPlaneInitialized from cluster controller #1356
Conversation
5b0cdc0
to
10edc48
Compare
10edc48
to
6d98d29
Compare
3eb208b
to
e8515e2
Compare
/test pull-cluster-api-test |
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.
Small nits, otherwise lgtm
controllers/cluster_controller.go
Outdated
|
||
// MachineToMachineSets is a handler.ToRequestsFunc to be used to enqeue requests for reconciliation | ||
// for Cluster to update its status.controlplaneInitialized field | ||
func (r *ClusterReconciler) MachineToCluster(o handler.MapObject) []ctrl.Request { |
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.
func (r *ClusterReconciler) MachineToCluster(o handler.MapObject) []ctrl.Request { | |
func (r *ClusterReconciler) machineToCluster(o handler.MapObject) []ctrl.Request { |
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 exported this to keep consistency with MachineToMachineSets
and MachineSetToDeployments
, whould we also un-export them too?
|
||
m, ok := o.Object.(*clusterv1.Machine) | ||
if !ok { | ||
r.Log.Error(errors.New("did not get machine"), "got", fmt.Sprintf("%T", o.Object)) |
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'm almost hesitant to suggest this, but I wonder if we should consider a panic
here - if we don't get the expected type, it's because of programmer error and the implication is that we'll never set ControlPlaneInitialized to true. It's probably easier to diagnose programmer error with a panic vs. digging through logs. Am I crazy? 😄
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 don't disagree, I'm +1 to use Fatal or panic
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.
This is what we are doing in other places, so we'd need to update all the existing use cases as well if we wanted to go down that path. (Not saying we shouldn't, just outside the scope of this PR).
e8515e2
to
a7a9129
Compare
/test pull-cluster-api-test |
} | ||
|
||
// getMachinesInCluster returns all of the Machine objects that belong to the cluster | ||
func (r *ClusterReconciler) getMachinesInCluster(ctx context.Context, namespace, name string) ([]*clusterv1.Machine, error) { |
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.
@vincepri @ncdc I created this function similar to this in machine controller. should move this as a common function elsewhere?
cluster-api/controllers/machine_controller.go
Lines 264 to 284 in 2a55573
func (r *MachineReconciler) getMachinesInCluster(ctx context.Context, namespace, name string) ([]*clusterv1.Machine, error) { | |
if name == "" { | |
return nil, nil | |
} | |
machineList := &clusterv1.MachineList{} | |
labels := map[string]string{clusterv1.MachineClusterLabelName: name} | |
if err := r.Client.List(ctx, machineList, client.InNamespace(namespace), client.MatchingLabels(labels)); err != nil { | |
return nil, errors.Wrap(err, "failed to list machines") | |
} | |
machines := []*clusterv1.Machine{} | |
for i := range machineList.Items { | |
m := &machineList.Items[i] | |
if m.DeletionTimestamp.IsZero() { | |
machines = append(machines, m) | |
} | |
} | |
return machines, 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.
Yes please! You can change the signature so it takes in the Client
. As for where to put it... I'm trying to avoid generic files/packages like util
or helpers
. Even though I just said let's avoid "helpers", maybe we put it in controllers/machine_helpers.go
? @vincepri wdyt?
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.
Either way should be fine, I'm also ok adding it in util/util.go
for now and remove it later, there are a bunch of other similar functions that take a Controller Runtime Client and return something, so this wouldn't be anything new.
If we want to make it private, controllers/helpers.go
or controllers/machine_helpers.go
as you suggested sounds good.
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.
lets address it in a separate pr after it is merged?
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.
ok
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.
2a3bcd9
to
e97d379
Compare
controllers/cluster_controller.go
Outdated
|
||
machines, err := r.getMachinesInCluster(ctx, cluster.Namespace, cluster.Name) | ||
if err != nil { | ||
r.Log.Error(err, "error getting machines in cluster", "cluster", cluster.Name) |
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.
We should either do "cluster", fmt.Sprintf("%s/%s", cluster.Namespace, cluster.Name)
, or include "namespace"
as a separate field.
@tahsinrahman sorry for the delay in reviewing - just added several comments. |
e97d379
to
5ca8ee2
Compare
5ca8ee2
to
fba571d
Compare
LGTM - @vincepri ? |
return nil | ||
} | ||
|
||
cluster, err := util.GetClusterFromMetadata(context.TODO(), r.Client, m.ObjectMeta) |
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.
Can we remove this query and just enqueue the request from the label name, and assume the same namespace?
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.
This is a code copy from the machine controller. The get is here so we can check control plane initialized below on line 365
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.
The method name threw me off a little controlPlaneMachineToCluster
, I'm fine leaving like this, but we could save an extra call and just reconcile again. In theory, it should be a noop.
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.
It pulls from the cache, so it's not a network hit. I guess it comes down to whether or not you want to avoid enqueuing if you know up front it's not relevant (non control plane machine).
if !util.IsControlPlaneMachine(m) { | ||
return nil | ||
} | ||
if m.Status.NodeRef == 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.
We can do an OR and merge the two ifs above
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'd prefer to leave it like this
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncdc, tahsinrahman The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1333