From 3142c3e5cb14cd0d1bc5601d0a5ad25351ecb6ae Mon Sep 17 00:00:00 2001 From: travisn Date: Tue, 23 Jul 2019 16:40:44 -0600 Subject: [PATCH] simplify CR status updates for ceph and edgefs The status messages on the CRs had much duplicated code for logging an error if the status update failed. Now any failures around the status update are logged consistently within the updateClusterStatus method so there is no need to return an error and duplicate the error handling everywhere. Signed-off-by: travisn --- pkg/operator/ceph/cluster/controller.go | 50 ++++++----------------- pkg/operator/edgefs/cluster/controller.go | 49 ++++++---------------- 2 files changed, 25 insertions(+), 74 deletions(-) diff --git a/pkg/operator/ceph/cluster/controller.go b/pkg/operator/ceph/cluster/controller.go index 332623df9..033036462 100644 --- a/pkg/operator/ceph/cluster/controller.go +++ b/pkg/operator/ceph/cluster/controller.go @@ -233,11 +233,7 @@ func (c *ClusterController) onAdd(obj interface{}) { logger.Infof("starting cluster in namespace %s", cluster.Namespace) if c.devicesInUse && cluster.Spec.Storage.AnyUseAllDevices() { - message := "using all devices in more than one namespace not supported" - logger.Error(message) - if err := c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, cephv1.ClusterStateError, message); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", cluster.Namespace, err) - } + c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, cephv1.ClusterStateError, "using all devices in more than one namespace is not supported") return } @@ -326,11 +322,7 @@ func (c *ClusterController) onAdd(obj interface{}) { } } - err = c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, cephv1.ClusterStateCreating, "") - if err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", cluster.Namespace, err) - return false, nil - } + c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, cephv1.ClusterStateCreating, "") err = cluster.createInstance(c.rookImage, *cephVersion) if err != nil { @@ -339,11 +331,7 @@ func (c *ClusterController) onAdd(obj interface{}) { } // cluster is created, update the cluster CRD status now - err = c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, cephv1.ClusterStateCreated, "") - if err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", cluster.Namespace, err) - return false, nil - } + c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, cephv1.ClusterStateCreated, "") return true, nil }) @@ -353,10 +341,7 @@ func (c *ClusterController) onAdd(obj interface{}) { if !validOrchestration { message = fmt.Sprintf("giving up creating cluster in namespace %s", cluster.Namespace) } - logger.Error(message) - if err := c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, cephv1.ClusterStateError, message); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", cluster.Namespace, err) - } + c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, cephv1.ClusterStateError, message) return } @@ -582,11 +567,8 @@ func (c *ClusterController) onUpdate(oldObj, newObj interface{}) { return c.handleUpdate(newClust.Name, cluster) }) if err != nil { - message := fmt.Sprintf("giving up trying to update cluster in namespace %s after %s", cluster.Namespace, updateClusterTimeout) - logger.Error(message) - if err := c.updateClusterStatus(newClust.Namespace, newClust.Name, cephv1.ClusterStateError, message); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", newClust.Namespace, err) - } + c.updateClusterStatus(newClust.Namespace, newClust.Name, cephv1.ClusterStateError, + fmt.Sprintf("giving up trying to update cluster in namespace %s after %s", cluster.Namespace, updateClusterTimeout)) return } @@ -597,20 +579,14 @@ func (c *ClusterController) onUpdate(oldObj, newObj interface{}) { } func (c *ClusterController) handleUpdate(crdName string, cluster *cluster) (bool, error) { - if err := c.updateClusterStatus(cluster.Namespace, crdName, cephv1.ClusterStateUpdating, ""); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", cluster.Namespace, err) - return false, nil - } + c.updateClusterStatus(cluster.Namespace, crdName, cephv1.ClusterStateUpdating, "") if err := cluster.createInstance(c.rookImage, cluster.Info.CephVersion); err != nil { logger.Errorf("failed to update cluster in namespace %s. %+v", cluster.Namespace, err) return false, nil } - if err := c.updateClusterStatus(cluster.Namespace, crdName, cephv1.ClusterStateCreated, ""); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", cluster.Namespace, err) - return false, nil - } + c.updateClusterStatus(cluster.Namespace, crdName, cephv1.ClusterStateCreated, "") logger.Infof("succeeded updating cluster in namespace %s", cluster.Namespace) return true, nil @@ -820,13 +796,13 @@ func (c *ClusterController) removeFinalizer(obj interface{}) { } // updateClusterStatus updates the status of the cluster custom resource, whether it is being updated or is completed -func (c *ClusterController) updateClusterStatus(namespace, name string, state cephv1.ClusterState, message string) error { - logger.Infof("CephCluster %s status: %s", namespace, state) +func (c *ClusterController) updateClusterStatus(namespace, name string, state cephv1.ClusterState, message string) { + logger.Infof("CephCluster %s status: %s. %s", namespace, state, message) // get the most recent cluster CRD object cluster, err := c.context.RookClientset.CephV1().CephClusters(namespace).Get(name, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("failed to get cluster from namespace %s prior to updating its status: %+v", namespace, err) + logger.Errorf("failed to get cluster from namespace %s prior to updating its status: %+v", namespace, err) } // update the status on the retrieved cluster object @@ -834,10 +810,8 @@ func (c *ClusterController) updateClusterStatus(namespace, name string, state ce cluster.Status.State = state cluster.Status.Message = message if _, err := c.context.RookClientset.CephV1().CephClusters(namespace).Update(cluster); err != nil { - return fmt.Errorf("failed to update cluster %s status: %+v", namespace, err) + logger.Errorf("failed to update cluster %s status: %+v", namespace, err) } - - return nil } func ClusterOwnerRef(namespace, clusterID string) metav1.OwnerReference { diff --git a/pkg/operator/edgefs/cluster/controller.go b/pkg/operator/edgefs/cluster/controller.go index 7a81258c5..b4d33e073 100644 --- a/pkg/operator/edgefs/cluster/controller.go +++ b/pkg/operator/edgefs/cluster/controller.go @@ -133,11 +133,7 @@ func (c *ClusterController) onAdd(obj interface{}) { logger.Infof("starting cluster in namespace %s", cluster.Namespace) if c.devicesInUse && cluster.Spec.Storage.AnyUseAllDevices() { - message := "using all devices in more than one namespace not supported" - logger.Error(message) - if err := c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, edgefsv1beta1.ClusterStateError, message); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", cluster.Namespace, err) - } + c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, edgefsv1beta1.ClusterStateError, "using all devices in more than one namespace is not supported") return } @@ -147,10 +143,7 @@ func (c *ClusterController) onAdd(obj interface{}) { // Start the Rook cluster components. Retry several times in case of failure. err := wait.Poll(clusterCreateInterval, clusterCreateTimeout, func() (bool, error) { - if err := c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, edgefsv1beta1.ClusterStateCreating, ""); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", cluster.Namespace, err) - return false, nil - } + c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, edgefsv1beta1.ClusterStateCreating, "") err := cluster.createInstance(c.containerImage, false) if err != nil { @@ -159,19 +152,13 @@ func (c *ClusterController) onAdd(obj interface{}) { } // cluster is created, update the cluster CRD status now - if err := c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, edgefsv1beta1.ClusterStateCreated, ""); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", cluster.Namespace, err) - return false, nil - } + c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, edgefsv1beta1.ClusterStateCreated, "") return true, nil }) if err != nil { - message := fmt.Sprintf("giving up creating cluster in namespace %s after %s", cluster.Namespace, clusterCreateTimeout) - logger.Error(message) - if err := c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, edgefsv1beta1.ClusterStateError, message); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", cluster.Namespace, err) - } + c.updateClusterStatus(clusterObj.Namespace, clusterObj.Name, edgefsv1beta1.ClusterStateError, + fmt.Sprintf("giving up creating cluster in namespace %s after %s", cluster.Namespace, clusterCreateTimeout)) return } @@ -307,21 +294,15 @@ func (c *ClusterController) onUpdate(oldObj, newObj interface{}) { return c.handleUpdate(newCluster, cluster) }) if err != nil { - message := fmt.Sprintf("giving up trying to update cluster in namespace %s after %s", cluster.Namespace, updateClusterTimeout) - logger.Error(message) - if err := c.updateClusterStatus(newCluster.Namespace, newCluster.Name, edgefsv1beta1.ClusterStateError, message); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", newCluster.Namespace, err) - } + c.updateClusterStatus(newCluster.Namespace, newCluster.Name, edgefsv1beta1.ClusterStateError, + fmt.Sprintf("giving up trying to update cluster in namespace %s after %s", cluster.Namespace, updateClusterTimeout)) return } logger.Infof("cluster %s updated in namespace %s", newCluster.Name, newCluster.Namespace) } func (c *ClusterController) handleUpdate(newClust *edgefsv1beta1.Cluster, cluster *cluster) (bool, error) { - if err := c.updateClusterStatus(newClust.Namespace, newClust.Name, edgefsv1beta1.ClusterStateUpdating, ""); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", newClust.Namespace, err) - return false, nil - } + c.updateClusterStatus(newClust.Namespace, newClust.Name, edgefsv1beta1.ClusterStateUpdating, "") if newClust.Spec.EdgefsImageName != "" { c.containerImage = newClust.Spec.EdgefsImageName @@ -332,10 +313,7 @@ func (c *ClusterController) handleUpdate(newClust *edgefsv1beta1.Cluster, cluste return false, nil } - if err := c.updateClusterStatus(newClust.Namespace, newClust.Name, edgefsv1beta1.ClusterStateCreated, ""); err != nil { - logger.Errorf("failed to update cluster status in namespace %s: %+v", newClust.Namespace, err) - return false, nil - } + c.updateClusterStatus(newClust.Namespace, newClust.Name, edgefsv1beta1.ClusterStateCreated, "") logger.Infof("succeeded updating cluster in namespace %s", newClust.Namespace) return true, nil @@ -382,20 +360,19 @@ func (c *ClusterController) handleDelete(clust *edgefsv1beta1.Cluster, retryInte return nil } -func (c *ClusterController) updateClusterStatus(namespace, name string, state edgefsv1beta1.ClusterState, message string) error { +func (c *ClusterController) updateClusterStatus(namespace, name string, state edgefsv1beta1.ClusterState, message string) { // get the most recent cluster CRD object cluster, err := c.context.RookClientset.EdgefsV1beta1().Clusters(namespace).Get(name, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("failed to get cluster from namespace %s prior to updating its status: %+v", namespace, err) + logger.Errorf("failed to get cluster from namespace %s prior to updating its status: %+v", namespace, err) + return } // update the status on the retrieved cluster object cluster.Status = edgefsv1beta1.ClusterStatus{State: state, Message: message} if _, err := c.context.RookClientset.EdgefsV1beta1().Clusters(cluster.Namespace).Update(cluster); err != nil { - return fmt.Errorf("failed to update cluster %s status: %+v", cluster.Namespace, err) + logger.Errorf("failed to update cluster %s status: %+v", cluster.Namespace, err) } - - return nil } func isHostNetworkDefined(hostNetworkSpec edgefsv1beta1.NetworkSpec) bool {