From 827e32019ab2ab0c7050f8dfda41c49b374e737e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Wed, 9 Aug 2023 17:17:54 +0200 Subject: [PATCH] fix: use SNO check to disable node-removal controller node-removal controller wouldn't be necessary at the end of the lifecycle of SNO nodes as the entire cluster would go down Signed-off-by: jakobmoellerdev fix: allow correct indexing for status objects and run in integration tests Signed-off-by: jakobmoellerdev chore:update controller to ignore no nodeStatus on deviceClass as otherwise deletion gets stuck Signed-off-by: jakobmoellerdev fix: ensure correct rbac for new controller and refactor updateLVMClusterStatus to contain checks per deviceClass per Node Signed-off-by: jakobmoellerdev fix: ensure LVMVolumeGroupNodeStatus is removed by dedicated cleanup controller in case of multi-node --- .../lvms-operator.clusterserviceversion.yaml | 2 + config/rbac/role.yaml | 2 + .../lvmcluster_controller_integration_test.go | 11 +- controllers/lvmcluster_controller_watches.go | 1 + controllers/node/removal_controller.go | 108 ++++++++++++++++++ controllers/suite_test.go | 11 ++ main.go | 28 ++++- pkg/cluster/leaderelection.go | 31 +---- pkg/cluster/leaderelection_test.go | 2 +- pkg/cluster/sno.go | 33 ++++++ 10 files changed, 201 insertions(+), 28 deletions(-) create mode 100644 controllers/node/removal_controller.go create mode 100644 pkg/cluster/sno.go diff --git a/bundle/manifests/lvms-operator.clusterserviceversion.yaml b/bundle/manifests/lvms-operator.clusterserviceversion.yaml index fc2356dff..6f72addd7 100644 --- a/bundle/manifests/lvms-operator.clusterserviceversion.yaml +++ b/bundle/manifests/lvms-operator.clusterserviceversion.yaml @@ -146,6 +146,8 @@ spec: verbs: - get - list + - patch + - update - watch - apiGroups: - "" diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 498674867..8cd41078d 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -52,6 +52,8 @@ rules: verbs: - get - list + - patch + - update - watch - apiGroups: - "" diff --git a/controllers/lvmcluster_controller_integration_test.go b/controllers/lvmcluster_controller_integration_test.go index 12c2e5b6b..fd907c0cd 100644 --- a/controllers/lvmcluster_controller_integration_test.go +++ b/controllers/lvmcluster_controller_integration_test.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" ) var _ = Describe("LVMCluster controller", func() { @@ -189,7 +190,15 @@ var _ = Describe("LVMCluster controller", func() { // delete lvmVolumeGroupNodeStatus as it should be deleted by vgmanager // and if it is present lvmcluster reconciler takes it as vg is present on node - Expect(k8sClient.Delete(ctx, lvmVolumeGroupNodeStatusIn)).Should(Succeed()) + + // we will now remove the node which will cause the LVM cluster status to also lose that vg + Expect(k8sClient.Delete(ctx, nodeIn)).Should(Succeed()) + // deletion of LVMCluster CR and thus also the NodeStatus through the removal controller + Eventually(func() bool { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(lvmVolumeGroupNodeStatusIn), + &lvmv1alpha1.LVMVolumeGroupNodeStatus{}) + return errors.IsNotFound(err) + }, timeout, interval).Should(BeTrue()) // deletion of LVMCluster CR Eventually(func() bool { diff --git a/controllers/lvmcluster_controller_watches.go b/controllers/lvmcluster_controller_watches.go index 5235786ee..eb53f87ae 100644 --- a/controllers/lvmcluster_controller_watches.go +++ b/controllers/lvmcluster_controller_watches.go @@ -20,6 +20,7 @@ import ( "context" lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1" + appsv1 "k8s.io/api/apps/v1" storagev1 "k8s.io/api/storage/v1" "k8s.io/apimachinery/pkg/types" diff --git a/controllers/node/removal_controller.go b/controllers/node/removal_controller.go new file mode 100644 index 000000000..ab964a6c0 --- /dev/null +++ b/controllers/node/removal_controller.go @@ -0,0 +1,108 @@ +package node + +import ( + "context" + "fmt" + + lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1" + + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +const cleanupFinalizer = "lvm.topolvm.io/node-removal-hook" +const fieldOwner = "lvms" + +type RemovalController struct { + client.Client +} + +//+kubebuilder:rbac:groups=core,resources=nodes,verbs=get;list;patch;update;watch +//+kubebuilder:rbac:groups=lvm.topolvm.io,resources=lvmvolumegroupnodestatuses,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=lvm.topolvm.io,resources=lvmvolumegroupnodestatuses/status,verbs=get;update;patch +//+kubebuilder:rbac:groups=lvm.topolvm.io,resources=lvmvolumegroupnodestatuses/finalizers,verbs=update + +// Reconcile takes care of watching a node, adding a finalizer, and reacting to a removal request by deleting +// the unwanted LVMVolumeGroupNodeStatus that was associated with the node, before removing the finalizer. +// It does nothing on active Nodes. If it can be assumed that there will always be only one node (SNO), +// this controller should not be started. +func (r *RemovalController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + logger := log.FromContext(ctx) + + node := &v1.Node{} + if err := r.Get(ctx, req.NamespacedName, node); err != nil { + // we'll ignore not-found errors, since they can't be fixed by an immediate + // requeue (we'll need to wait for a new notification), and we can get them + // on deleted requests. + return ctrl.Result{}, client.IgnoreNotFound(err) + } + + if node.DeletionTimestamp.IsZero() { + // Add a finalizer in case the node is fresh or the controller newly deployed + if needsUpdate := controllerutil.AddFinalizer(node, cleanupFinalizer); needsUpdate { + if err := r.Update(ctx, node, client.FieldOwner(fieldOwner)); err != nil { + return ctrl.Result{}, fmt.Errorf("node finalizer could not be updated: %w", err) + } + } + // nothing to do here, the node exists and is happy, + // maybe there is a NodeVolumeGroupStatus but we don't care + return ctrl.Result{}, nil + } + + logger.Info("node getting deleted, removing leftover LVMVolumeGroupNodeStatus") + + vgNodeStatusList := &lvmv1alpha1.LVMVolumeGroupNodeStatusList{} + if err := r.Client.List(ctx, vgNodeStatusList, client.MatchingFields{"metadata.name": node.GetName()}); err != nil { + return ctrl.Result{}, fmt.Errorf("error retrieving fitting LVMVolumeGroupNodeStatus for Node %s: %w", node.GetName(), err) + } + + if len(vgNodeStatusList.Items) == 0 { + logger.Info("LVMVolumeGroupNodeStatus already deleted") + return ctrl.Result{}, nil + } + + for i := range vgNodeStatusList.Items { + if err := r.Client.Delete(ctx, &vgNodeStatusList.Items[i]); err != nil { + return ctrl.Result{}, fmt.Errorf("could not cleanup LVMVolumeGroupNodeStatus for Node %s: %w", node.GetName(), err) + } + } + + logger.Info("every LVMVolumeGroupNodeStatus for node was removed, removing finalizer to allow node removal") + if needsUpdate := controllerutil.RemoveFinalizer(node, cleanupFinalizer); needsUpdate { + if err := r.Update(ctx, node, client.FieldOwner(fieldOwner)); err != nil { + return ctrl.Result{}, fmt.Errorf("node finalizer could not be removed: %w", err) + } + } + + return ctrl.Result{}, nil +} + +// SetupWithManager sets up the controller with the Manager. +func (r *RemovalController) SetupWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr).For(&v1.Node{}).Watches(&lvmv1alpha1.LVMVolumeGroupNodeStatus{}, + handler.EnqueueRequestsFromMapFunc(r.getNodeForLVMVolumeGroupNodeStatus)).Complete(r) +} + +func (r *RemovalController) getNodeForLVMVolumeGroupNodeStatus(ctx context.Context, object client.Object) []reconcile.Request { + node := &v1.Node{} + node.SetName(object.GetName()) + + err := r.Client.Get(ctx, client.ObjectKeyFromObject(node), node) + if errors.IsNotFound(err) { + return []reconcile.Request{} + } + + if err != nil { + log.FromContext(ctx).Error(err, "could not get Node for LVMVolumeGroupNodeStatus", "LVMVolumeGroupNodeStatus", object.GetName()) + return []reconcile.Request{} + } + + return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: node.GetName()}}} +} diff --git a/controllers/suite_test.go b/controllers/suite_test.go index d324e3431..01a305afe 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -35,6 +35,7 @@ import ( snapapi "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" secv1client "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1" lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1" + "github.com/openshift/lvm-operator/controllers/node" topolvmv1 "github.com/topolvm/topolvm/api/v1" //+kubebuilder:scaffold:imports ) @@ -118,6 +119,16 @@ var _ = BeforeSuite(func() { }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) + err = (&node.RemovalController{ + Client: k8sManager.GetClient(), + }).SetupWithManager(k8sManager) + Expect(err).ToNot(HaveOccurred()) + + err = k8sManager.GetFieldIndexer().IndexField(context.Background(), &lvmv1alpha1.LVMVolumeGroupNodeStatus{}, "metadata.name", func(object client.Object) []string { + return []string{object.GetName()} + }) + Expect(err).ToNot(HaveOccurred(), "unable to create name index on LVMVolumeGroupNodeStatus") + go func() { defer GinkgoRecover() err = k8sManager.Start(ctx) diff --git a/main.go b/main.go index fae4c412d..f3b93c9d5 100644 --- a/main.go +++ b/main.go @@ -20,6 +20,7 @@ import ( "context" "flag" "fmt" + "github.com/openshift/lvm-operator/controllers/node" "os" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) @@ -32,6 +33,7 @@ import ( "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -89,7 +91,13 @@ func main() { } setupLog.Info("Watching namespace", "Namespace", operatorNamespace) - leaderElectionResolver, err := cluster.NewLeaderElectionResolver(ctrl.GetConfigOrDie(), scheme, enableLeaderElection, operatorNamespace) + setupClient, err := client.New(ctrl.GetConfigOrDie(), client.Options{Scheme: scheme}) + if err != nil { + setupLog.Error(err, "unable to initialize setup client for pre-manager startup checks") + os.Exit(1) + } + snoCheck := cluster.NewMasterSNOCheck(setupClient) + leaderElectionResolver, err := cluster.NewLeaderElectionResolver(snoCheck, enableLeaderElection, operatorNamespace) if err != nil { setupLog.Error(err, "unable to setup leader election") os.Exit(1) @@ -129,6 +137,24 @@ func main() { setupLog.Error(err, "unable to create controller", "controller", "LVMCluster") os.Exit(1) } + + if !snoCheck.IsSNO(context.Background()) { + setupLog.Info("starting node-removal controller to observe node removal in MultiNode") + if err = (&node.RemovalController{ + Client: mgr.GetClient(), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "NodeRemovalControlelr") + os.Exit(1) + } + } + + if err = mgr.GetFieldIndexer().IndexField(context.Background(), &lvmv1alpha1.LVMVolumeGroupNodeStatus{}, "metadata.name", func(object client.Object) []string { + return []string{object.GetName()} + }); err != nil { + setupLog.Error(err, "unable to create name index on LVMVolumeGroupNodeStatus") + os.Exit(1) + } + if err = (&lvmv1alpha1.LVMCluster{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "LVMCluster") os.Exit(1) diff --git a/pkg/cluster/leaderelection.go b/pkg/cluster/leaderelection.go index 943d14ce2..fa41a1d95 100644 --- a/pkg/cluster/leaderelection.go +++ b/pkg/cluster/leaderelection.go @@ -2,14 +2,8 @@ package cluster import ( "context" - "fmt" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/library-go/pkg/config/leaderelection" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/rest" - "os" - "sigs.k8s.io/controller-runtime/pkg/client" log "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -25,45 +19,32 @@ type LeaderElectionResolver interface { // on the amount of master nodes discovered in the cluster. If there is exactly one control-plane/master node, // the returned LeaderElectionResolver settings are optimized for SNO deployments. func NewLeaderElectionResolver( - config *rest.Config, - scheme *runtime.Scheme, + snoCheck SNOCheck, enableLeaderElection bool, operatorNamespace string, ) (LeaderElectionResolver, error) { - leaderElectionClient, err := client.New(config, client.Options{Scheme: scheme}) - if err != nil { - return nil, fmt.Errorf("cannot create leader election client: %w", err) - } - defaultElectionConfig := leaderelection.LeaderElectionDefaulting(configv1.LeaderElection{ Disable: !enableLeaderElection, }, operatorNamespace, "1136b8a6.topolvm.io") return &nodeLookupSNOLeaderElection{ - clnt: leaderElectionClient, + snoCheck: snoCheck, defaultElectionConfig: defaultElectionConfig, }, nil } type nodeLookupSNOLeaderElection struct { - clnt client.Client + snoCheck SNOCheck defaultElectionConfig configv1.LeaderElection } func (le *nodeLookupSNOLeaderElection) Resolve(ctx context.Context) (configv1.LeaderElection, error) { logger := log.FromContext(ctx) - nodes := &corev1.NodeList{} - if err := le.clnt.List(context.Background(), nodes, client.MatchingLabels{ - ControlPlaneIDLabel: "", - }); err != nil { - logger.Error(err, "unable to retrieve nodes for SNO check with lease configuration") - os.Exit(1) - } - if len(nodes.Items) != 1 { + if !le.snoCheck.IsSNO(ctx) { + logger.Info("Using default Multi-Node leader election settings optimized for high-availability") return le.defaultElectionConfig, nil } - logger.Info("Overwriting defaults with SNO leader election config as only a single node was discovered", - "node", nodes.Items[0].GetName()) + logger.Info("Overwriting defaults with SNO leader election config as only a single node was discovered") config := leaderelection.LeaderElectionSNOConfig(le.defaultElectionConfig) logger.Info("leader election config setup succeeded", "retry-period", config.RetryPeriod, diff --git a/pkg/cluster/leaderelection_test.go b/pkg/cluster/leaderelection_test.go index 49d36f1e8..80ec8bddb 100644 --- a/pkg/cluster/leaderelection_test.go +++ b/pkg/cluster/leaderelection_test.go @@ -84,7 +84,7 @@ func Test_nodeLookupSNOLeaderElection_Resolve(t *testing.T) { t.Run(tt.name, func(t *testing.T) { clnt := fake.NewClientBuilder().WithObjects(tt.nodes...).Build() le := &nodeLookupSNOLeaderElection{ - clnt: clnt, + snoCheck: NewMasterSNOCheck(clnt), defaultElectionConfig: leaderelection.LeaderElectionDefaulting(configv1.LeaderElection{}, "test", "test-leader-id"), } diff --git a/pkg/cluster/sno.go b/pkg/cluster/sno.go new file mode 100644 index 000000000..c67ef674b --- /dev/null +++ b/pkg/cluster/sno.go @@ -0,0 +1,33 @@ +package cluster + +import ( + "context" + corev1 "k8s.io/api/core/v1" + "os" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" +) + +type SNOCheck interface { + IsSNO(ctx context.Context) bool +} + +func NewMasterSNOCheck(clnt client.Client) SNOCheck { + return &masterSNOCheck{clnt: clnt} +} + +type masterSNOCheck struct { + clnt client.Client +} + +func (chk *masterSNOCheck) IsSNO(ctx context.Context) bool { + logger := log.FromContext(ctx) + nodes := &corev1.NodeList{} + if err := chk.clnt.List(context.Background(), nodes, client.MatchingLabels{ + ControlPlaneIDLabel: "", + }); err != nil { + logger.Error(err, "unable to retrieve nodes for SNO check with lease configuration") + os.Exit(1) + } + return nodes.Items != nil && len(nodes.Items) == 1 +}