Skip to content

Commit 91b7ccf

Browse files
authored
Merge pull request #2182 from shiftstack/watch
Reduce reconciles and logs
2 parents e8bddb5 + 186c11d commit 91b7ccf

File tree

4 files changed

+98
-26
lines changed

4 files changed

+98
-26
lines changed

controllers/openstackcluster_controller.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -865,6 +865,11 @@ func (r *OpenStackClusterReconciler) SetupWithManager(ctx context.Context, mgr c
865865
}),
866866
builder.WithPredicates(predicates.ClusterUnpaused(ctrl.LoggerFrom(ctx))),
867867
).
868+
Watches(
869+
&infrav1alpha1.OpenStackServer{},
870+
handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &infrav1.OpenStackCluster{}),
871+
builder.WithPredicates(OpenStackServerReconcileComplete(log)),
872+
).
868873
WithEventFilter(predicates.ResourceNotPausedAndHasFilterLabel(ctrl.LoggerFrom(ctx), r.WatchFilterValue)).
869874
WithEventFilter(predicates.ResourceIsNotExternallyManaged(ctrl.LoggerFrom(ctx))).
870875
Complete(r)

controllers/openstackmachine_controller.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -188,35 +188,35 @@ func patchMachine(ctx context.Context, patchHelper *patch.Helper, openStackMachi
188188
}
189189

190190
func (r *OpenStackMachineReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
191+
log := ctrl.LoggerFrom(ctx)
192+
191193
return ctrl.NewControllerManagedBy(mgr).
192194
WithOptions(options).
193195
For(&infrav1.OpenStackMachine{}).
194196
Watches(
195197
&clusterv1.Machine{},
196198
handler.EnqueueRequestsFromMapFunc(util.MachineToInfrastructureMapFunc(infrav1.SchemeGroupVersion.WithKind("OpenStackMachine"))),
197199
).
198-
Watches(
199-
&infrav1.OpenStackCluster{},
200-
handler.EnqueueRequestsFromMapFunc(r.OpenStackClusterToOpenStackMachines(ctx)),
201-
).
202200
WithEventFilter(predicates.ResourceNotPausedAndHasFilterLabel(ctrl.LoggerFrom(ctx), r.WatchFilterValue)).
203201
Watches(
204202
&clusterv1.Cluster{},
205203
handler.EnqueueRequestsFromMapFunc(r.requeueOpenStackMachinesForUnpausedCluster(ctx)),
206-
builder.WithPredicates(predicates.ClusterUnpausedAndInfrastructureReady(ctrl.LoggerFrom(ctx))),
204+
builder.WithPredicates(predicates.ClusterUnpausedAndInfrastructureReady(log)),
207205
).
206+
// NOTE: we don't watch OpenStackCluster here, even though the
207+
// OpenStackMachine controller directly requires values from
208+
// OpenStackCluster. The reason is that we are already observing Cluster
209+
// with the ClusterUnpausedAndInfrastructureReady predicate. The only
210+
// fields in OpenStackCluster we are interested in are dependent on
211+
// InfrastructureReady, so we don't need to watch both.
208212
Watches(
209213
&ipamv1.IPAddressClaim{},
210214
handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &infrav1.OpenStackMachine{}),
211215
).
212-
// TODO(emilien) to optimize because it's not efficient to watch all OpenStackServer events.
213-
// We are only interested in certain state transitions of the OpenStackServer:
214-
// - when the server is deleted
215-
// - when the server becomes ready
216-
// For that we probably want to write Predicate functions for the OpenStackServer.
217216
Watches(
218217
&infrav1alpha1.OpenStackServer{},
219218
handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &infrav1.OpenStackMachine{}),
219+
builder.WithPredicates(OpenStackServerReconcileComplete(log)),
220220
).
221221
Complete(r)
222222
}

controllers/openstackserver_controller.go

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
"k8s.io/apimachinery/pkg/types"
3131
kerrors "k8s.io/apimachinery/pkg/util/errors"
3232
"k8s.io/client-go/tools/record"
33+
"k8s.io/klog/v2"
3334
"k8s.io/utils/ptr"
3435
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
3536
ipamv1 "sigs.k8s.io/cluster-api/exp/ipam/api/v1beta1"
@@ -42,7 +43,9 @@ import (
4243
"sigs.k8s.io/controller-runtime/pkg/client"
4344
"sigs.k8s.io/controller-runtime/pkg/controller"
4445
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
46+
"sigs.k8s.io/controller-runtime/pkg/event"
4547
"sigs.k8s.io/controller-runtime/pkg/handler"
48+
"sigs.k8s.io/controller-runtime/pkg/predicate"
4649
"sigs.k8s.io/controller-runtime/pkg/reconcile"
4750

4851
orcv1alpha1 "github.com/k-orc/openstack-resource-controller/api/v1alpha1"
@@ -285,9 +288,16 @@ func objToDependencyLabel(obj client.Object, scheme *runtime.Scheme) (string, er
285288
return "orc-dependency-" + prefixType + "/" + obj.GetName(), nil
286289
}
287290

291+
func IsServerTerminalError(server *infrav1alpha1.OpenStackServer) bool {
292+
if server.Status.InstanceState != nil && *server.Status.InstanceState == infrav1.InstanceStateError {
293+
return true
294+
}
295+
return false
296+
}
297+
288298
func (r *OpenStackServerReconciler) reconcileNormal(ctx context.Context, scope *scope.WithLogger, openStackServer *infrav1alpha1.OpenStackServer) (_ ctrl.Result, reterr error) {
289299
// If the OpenStackServer is in an error state, return early.
290-
if openStackServer.Status.InstanceState != nil && *openStackServer.Status.InstanceState == infrav1.InstanceStateError {
300+
if IsServerTerminalError(openStackServer) {
291301
scope.Logger().Info("Not reconciling server in error state. See openStackServer.status or previously logged error for details")
292302
return ctrl.Result{}, nil
293303
}
@@ -708,3 +718,56 @@ func (r *OpenStackServerReconciler) reconcileDeleteFloatingAddressFromPool(scope
708718
controllerutil.RemoveFinalizer(claim, infrav1.IPClaimMachineFinalizer)
709719
return r.Client.Update(context.Background(), claim)
710720
}
721+
722+
// OpenStackServerReconcileComplete returns a predicate that determines if a OpenStackServer has finished reconciling.
723+
func OpenStackServerReconcileComplete(log logr.Logger) predicate.Funcs {
724+
log = log.WithValues("predicate", "OpenStackServerReconcileComplete")
725+
726+
return predicate.Funcs{
727+
CreateFunc: func(e event.CreateEvent) bool {
728+
log = log.WithValues("eventType", "create")
729+
730+
server, ok := e.Object.(*infrav1alpha1.OpenStackServer)
731+
if !ok {
732+
log.V(4).Info("Expected OpenStackServer", "type", fmt.Sprintf("%T", e.Object))
733+
return false
734+
}
735+
log = log.WithValues("OpenStackServer", klog.KObj(server))
736+
737+
if server.Status.Ready || IsServerTerminalError(server) {
738+
log.V(6).Info("OpenStackServer finished reconciling, allowing further processing")
739+
return true
740+
}
741+
log.V(6).Info("OpenStackServer is still reconciling, blocking further processing")
742+
return false
743+
},
744+
UpdateFunc: func(e event.UpdateEvent) bool {
745+
log := log.WithValues("eventType", "update")
746+
747+
oldServer, ok := e.ObjectOld.(*infrav1alpha1.OpenStackServer)
748+
if !ok {
749+
log.V(4).Info("Expected OpenStackServer", "type", fmt.Sprintf("%T", e.ObjectOld))
750+
return false
751+
}
752+
log = log.WithValues("OpenStackServer", klog.KObj(oldServer))
753+
754+
newServer, ok := e.ObjectNew.(*infrav1alpha1.OpenStackServer)
755+
if !ok {
756+
log.V(4).Info("Expected OpenStackServer (new)", "type", fmt.Sprintf("%T", e.ObjectNew))
757+
return false
758+
}
759+
760+
oldFinished := oldServer.Status.Ready || IsServerTerminalError(oldServer)
761+
newFinished := newServer.Status.Ready || IsServerTerminalError(newServer)
762+
if !oldFinished && newFinished {
763+
log.V(6).Info("OpenStackServer finished reconciling, allowing further processing")
764+
return true
765+
}
766+
767+
log.V(4).Info("OpenStackServer is still reconciling, blocking further processing")
768+
return false
769+
},
770+
DeleteFunc: func(event.DeleteEvent) bool { return false },
771+
GenericFunc: func(event.GenericEvent) bool { return false },
772+
}
773+
}

pkg/cloud/services/networking/securitygroups.go

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -443,24 +443,28 @@ func (s *Service) reconcileGroupRules(desired *securityGroupSpec, observed *grou
443443
}
444444
}
445445

446-
s.scope.Logger().V(4).Info("Deleting rules not needed anymore for group", "name", observed.Name, "amount", len(rulesToDelete))
447-
for _, rule := range rulesToDelete {
448-
s.scope.Logger().V(6).Info("Deleting rule", "ID", rule, "name", observed.Name)
449-
err := s.client.DeleteSecGroupRule(rule)
450-
if err != nil {
451-
return err
446+
if len(rulesToDelete) > 0 {
447+
s.scope.Logger().V(4).Info("Deleting rules not needed anymore for group", "name", observed.Name, "amount", len(rulesToDelete))
448+
for _, rule := range rulesToDelete {
449+
s.scope.Logger().V(6).Info("Deleting rule", "ID", rule, "name", observed.Name)
450+
err := s.client.DeleteSecGroupRule(rule)
451+
if err != nil {
452+
return err
453+
}
452454
}
453455
}
454456

455-
s.scope.Logger().V(4).Info("Creating new rules needed for group", "name", observed.Name, "amount", len(rulesToCreate))
456-
for _, rule := range rulesToCreate {
457-
r := rule
458-
if r.RemoteGroupID == remoteGroupIDSelf {
459-
r.RemoteGroupID = observed.ID
460-
}
461-
err := s.createRule(observed.ID, r)
462-
if err != nil {
463-
return err
457+
if len(rulesToCreate) > 0 {
458+
s.scope.Logger().V(4).Info("Creating new rules needed for group", "name", observed.Name, "amount", len(rulesToCreate))
459+
for _, rule := range rulesToCreate {
460+
r := rule
461+
if r.RemoteGroupID == remoteGroupIDSelf {
462+
r.RemoteGroupID = observed.ID
463+
}
464+
err := s.createRule(observed.ID, r)
465+
if err != nil {
466+
return err
467+
}
464468
}
465469
}
466470

0 commit comments

Comments
 (0)