diff --git a/pkg/operator/ceohelpers/quorum_check.go b/pkg/operator/ceohelpers/quorum_check.go index fce37040fb..e9687023b2 100644 --- a/pkg/operator/ceohelpers/quorum_check.go +++ b/pkg/operator/ceohelpers/quorum_check.go @@ -1,8 +1,13 @@ package ceohelpers import ( + "context" + "fmt" + configv1listers "github.com/openshift/client-go/config/listers/config/v1" "github.com/openshift/library-go/pkg/operator/v1helpers" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" corev1listers "k8s.io/client-go/listers/core/v1" "github.com/openshift/cluster-etcd-operator/pkg/etcdcli" @@ -13,6 +18,12 @@ type QuorumChecker interface { // loss of a single etcd member. Such loss is common during new static pod revision. // Returns True when it is absolutely safe, false if not. Error otherwise, which always indicates it is unsafe. IsSafeToUpdateRevision() (bool, error) + // IsSafeToRestartMember returns true when restarting the etcd member on targetNodeName cannot break quorum: + // the etcd cluster is currently quorum fault-tolerant AND no control plane node other than the target is + // cordoned or not ready. A cordoned control plane node signals an imminent drain/reboot (for instance a + // machine-config update); restarting a member while another node is about to go down can leave the cluster + // without quorum (OCPBUGS-100060). When unsafe, the returned reason explains why. + IsSafeToRestartMember(ctx context.Context, targetNodeName string) (bool, string, error) } // AlwaysSafeQuorumChecker can be used for testing and always returns that it is safe to update a revision @@ -24,12 +35,18 @@ func (c *AlwaysSafeQuorumChecker) IsSafeToUpdateRevision() (bool, error) { return true, nil } +// IsSafeToRestartMember always returns true +func (c *AlwaysSafeQuorumChecker) IsSafeToRestartMember(ctx context.Context, targetNodeName string) (bool, string, error) { + return true, "", nil +} + // QuorumCheck is just a convenience struct around bootstrap.go type QuorumCheck struct { namespaceLister corev1listers.NamespaceLister infraLister configv1listers.InfrastructureLister operatorClient v1helpers.StaticPodOperatorClient etcdClient etcdcli.AllMemberLister + nodeLister corev1listers.NodeLister } func (c *QuorumCheck) IsSafeToUpdateRevision() (bool, error) { @@ -41,17 +58,71 @@ func (c *QuorumCheck) IsSafeToUpdateRevision() (bool, error) { return true, nil } +func (c *QuorumCheck) IsSafeToRestartMember(ctx context.Context, targetNodeName string) (bool, string, error) { + scalingStrategy, err := GetBootstrapScalingStrategy(c.operatorClient, c.namespaceLister, c.infraLister) + if err != nil { + return false, "", fmt.Errorf("IsSafeToRestartMember failed to get bootstrap scaling strategy: %w", err) + } + if scalingStrategy == UnsafeScalingStrategy { + return true, "", nil + } + + // the cluster must currently tolerate the loss of one member + memberHealth, err := c.etcdClient.MemberHealth(ctx) + if err != nil { + return false, "", fmt.Errorf("IsSafeToRestartMember couldn't determine member health: %w", err) + } + // Two Node OpenShift with Fencing protects etcd via pacemaker; treat it as an exception to the fault + // tolerance rule, consistent with CheckSafeToScaleCluster. + if err := etcdcli.IsQuorumFaultTolerantErr(memberHealth); err != nil && + !(len(memberHealth) == 2 && (scalingStrategy == TwoNodeScalingStrategy || scalingStrategy == DelayedTwoNodeScalingStrategy)) { + return false, err.Error(), nil + } + + // no control plane node other than the target may be cordoned or not ready. Member health alone is not + // enough: a node that is about to reboot keeps reporting healthy until it actually goes down, while the + // cordon that precedes its drain is visible minutes in advance. + nodes, err := c.nodeLister.List(labels.Everything()) + if err != nil { + return false, "", fmt.Errorf("IsSafeToRestartMember failed to list control plane nodes: %w", err) + } + for _, node := range nodes { + if node.Name == targetNodeName { + continue + } + if node.Spec.Unschedulable { + return false, fmt.Sprintf("control plane node %q is cordoned, likely about to be drained and rebooted; restarting the etcd member on %q now could lose quorum", node.Name, targetNodeName), nil + } + if !isNodeReady(node) { + return false, fmt.Sprintf("control plane node %q is not ready; restarting the etcd member on %q now could lose quorum", node.Name, targetNodeName), nil + } + } + + return true, "", nil +} + +func isNodeReady(node *corev1.Node) bool { + for _, condition := range node.Status.Conditions { + if condition.Type == corev1.NodeReady { + return condition.Status == corev1.ConditionTrue + } + } + return false +} + func NewQuorumChecker( namespaceLister corev1listers.NamespaceLister, infraLister configv1listers.InfrastructureLister, operatorClient v1helpers.StaticPodOperatorClient, etcdClient etcdcli.AllMemberLister, + nodeLister corev1listers.NodeLister, ) QuorumChecker { c := &QuorumCheck{ namespaceLister, infraLister, operatorClient, etcdClient, + nodeLister, } return c } diff --git a/pkg/operator/ceohelpers/quorum_check_test.go b/pkg/operator/ceohelpers/quorum_check_test.go index 2239ab971e..a07c226ca0 100644 --- a/pkg/operator/ceohelpers/quorum_check_test.go +++ b/pkg/operator/ceohelpers/quorum_check_test.go @@ -1,6 +1,7 @@ package ceohelpers import ( + "context" "fmt" "testing" @@ -114,7 +115,8 @@ func TestQuorumCheck_IsSafeToUpdateRevision(t *testing.T) { corev1listers.NewNamespaceLister(indexer), configv1listers.NewInfrastructureLister(indexer), fakeOperatorClient, - fakeEtcdClient) + fakeEtcdClient, + corev1listers.NewNodeLister(indexer)) safe, err := quorumChecker.IsSafeToUpdateRevision() assert.Equal(t, scenario.expectedErr, err) @@ -122,3 +124,128 @@ func TestQuorumCheck_IsSafeToUpdateRevision(t *testing.T) { }) } } + +func TestQuorumCheck_IsSafeToRestartMember(t *testing.T) { + defaultEtcdMembers := []*etcdserverpb.Member{ + u.FakeEtcdMemberWithoutServer(0), + u.FakeEtcdMemberWithoutServer(1), + u.FakeEtcdMemberWithoutServer(2), + } + + defaultObjects := []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: operatorclient.TargetNamespace}, + }, + &configv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{Name: InfrastructureClusterName}, + Status: configv1.InfrastructureStatus{ + ControlPlaneTopology: configv1.HighlyAvailableTopologyMode}, + }, + } + + node := func(name string, ready bool, unschedulable bool) *corev1.Node { + status := corev1.ConditionTrue + if !ready { + status = corev1.ConditionFalse + } + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: corev1.NodeSpec{Unschedulable: unschedulable}, + Status: corev1.NodeStatus{Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: status}, + }}, + } + } + + scenarios := []struct { + name string + nodes []*corev1.Node + etcdMembers []*etcdserverpb.Member + targetNode string + + safe bool + reasonContains string + }{ + { + name: "all nodes ready and schedulable", + nodes: []*corev1.Node{node("master-0", true, false), node("master-1", true, false), node("master-2", true, false)}, + etcdMembers: defaultEtcdMembers, + targetNode: "master-1", + safe: true, + }, + { + name: "another master cordoned", + nodes: []*corev1.Node{node("master-0", true, true), node("master-1", true, false), node("master-2", true, false)}, + etcdMembers: defaultEtcdMembers, + targetNode: "master-1", + safe: false, reasonContains: "cordoned", + }, + { + name: "target itself cordoned is allowed", + nodes: []*corev1.Node{node("master-0", true, false), node("master-1", true, true), node("master-2", true, false)}, + etcdMembers: defaultEtcdMembers, + targetNode: "master-1", + safe: true, + }, + { + name: "another master not ready", + nodes: []*corev1.Node{node("master-0", false, false), node("master-1", true, false), node("master-2", true, false)}, + etcdMembers: defaultEtcdMembers, + targetNode: "master-1", + safe: false, reasonContains: "not ready", + }, + { + name: "quorum not fault tolerant", + nodes: []*corev1.Node{node("master-0", true, false), node("master-1", true, false), node("master-2", true, false)}, + etcdMembers: []*etcdserverpb.Member{ + u.FakeEtcdMemberWithoutServer(0), + u.FakeEtcdMemberWithoutServer(1), + }, + targetNode: "master-1", + safe: false, reasonContains: "quorum", + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( + &operatorv1.StaticPodOperatorSpec{ + OperatorSpec: operatorv1.OperatorSpec{ManagementState: operatorv1.Managed}, + }, + u.StaticPodOperatorStatus( + u.WithLatestRevision(3), + u.WithNodeStatusAtCurrentRevision(3), + u.WithNodeStatusAtCurrentRevision(3), + u.WithNodeStatusAtCurrentRevision(3), + ), + nil, + nil, + ) + fakeEtcdClient, err := etcdcli.NewFakeEtcdClient(scenario.etcdMembers) + require.NoError(t, err) + + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) + for _, obj := range defaultObjects { + require.NoError(t, indexer.Add(obj)) + } + nodeIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) + for _, n := range scenario.nodes { + require.NoError(t, nodeIndexer.Add(n)) + } + + quorumChecker := NewQuorumChecker( + corev1listers.NewNamespaceLister(indexer), + configv1listers.NewInfrastructureLister(indexer), + fakeOperatorClient, + fakeEtcdClient, + corev1listers.NewNodeLister(nodeIndexer)) + + safe, reason, err := quorumChecker.IsSafeToRestartMember(context.TODO(), scenario.targetNode) + require.NoError(t, err) + assert.Equal(t, scenario.safe, safe) + if scenario.reasonContains != "" { + assert.Contains(t, reason, scenario.reasonContains) + } + }) + } +} diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index b7f55896e2..a4b3264e24 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -293,7 +293,18 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle etcdNamespaceLister, configInformers.Config().V1().Infrastructures().Lister(), operatorClient, - quorumMemberClient) + quorumMemberClient, + controlPlaneNodeLister) + + // memberRestartChecker gates per-node installer pods (which restart the etcd member on that node). + // It deliberately uses the non-cached etcd client: the cached member health (60s TTL) can report a + // member as healthy well after its node started going down (OCPBUGS-100060). + memberRestartChecker := ceohelpers.NewQuorumChecker( + etcdNamespaceLister, + configInformers.Config().V1().Infrastructures().Lister(), + operatorClient, + etcdClient, + controlPlaneNodeLister) targetConfigReconciler := targetconfigcontroller.NewTargetConfigController( AlivenessChecker, @@ -342,6 +353,9 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle guardRolloutPreCheck, ). WithRevisionControllerPrecondition(quorumSafe). + WithInstallerPrecondition(func(ctx context.Context, nodeName string) (bool, string, error) { + return memberRestartChecker.IsSafeToRestartMember(ctx, nodeName) + }). WithOperandPodLabelSelector(labels.Set{"etcd": "true"}.AsSelector()) if hasArbiterTopology { staticPodBuilderControllers = staticPodBuilderControllers.WithExtraNodeSelector(arbiterNodeLabelSelector) diff --git a/vendor/github.com/openshift/library-go/pkg/operator/staticpod/controller/installer/installer_controller.go b/vendor/github.com/openshift/library-go/pkg/operator/staticpod/controller/installer/installer_controller.go index 80d0265a8d..f99aa62f12 100644 --- a/vendor/github.com/openshift/library-go/pkg/operator/staticpod/controller/installer/installer_controller.go +++ b/vendor/github.com/openshift/library-go/pkg/operator/staticpod/controller/installer/installer_controller.go @@ -106,6 +106,11 @@ type InstallerController struct { installerPodMutationFns []InstallerPodMutationFunc + // installerPrecondition, when set, is consulted immediately before an installer pod is created for a node. + // When it returns false the installer controller emits an InstallerPreconditionNotMet event and requeues + // instead of creating the installer pod (which would restart the operand static pod on that node). + installerPrecondition InstallerPreconditionFunc + startupMonitorEnabled func() (bool, error) factory *factory.Factory @@ -133,6 +138,24 @@ func (c *InstallerController) WithMinReadyDuration(minReadyDuration time.Duratio return c } +// installerPreconditionRequeueDuration is how long the installer controller waits before rechecking +// an unmet installer precondition. +const installerPreconditionRequeueDuration = 15 * time.Second + +// InstallerPreconditionFunc returns true when it is safe to create an installer pod (which will +// restart the operand static pod) on the given node. When it returns false with a reason, the +// installer controller requeues and retries later. An error makes the sync fail. +type InstallerPreconditionFunc func(ctx context.Context, nodeName string) (safe bool, reason string, err error) + +// WithInstallerPrecondition sets a precondition consulted immediately before creating an installer +// pod for a node. Operators can use it to delay operand restarts when doing so would be unsafe, +// for example when restarting an etcd member while another control plane node is being rebooted +// would lose quorum. +func (c *InstallerController) WithInstallerPrecondition(precondition InstallerPreconditionFunc) *InstallerController { + c.installerPrecondition = precondition + return c +} + func (c *InstallerController) WithCerts(certDir string, certConfigMaps, certSecrets []UnrevisionedResource) *InstallerController { c.certDir = certDir c.certConfigMaps = certConfigMaps @@ -513,6 +536,18 @@ func (c *InstallerController) manageInstallationPods(ctx context.Context, operat } } + if c.installerPrecondition != nil { + safe, reason, err := c.installerPrecondition(ctx, currNodeState.NodeName) + if err != nil { + return true, 0, nil, nil, err + } + if !safe { + c.eventRecorder.Warningf("InstallerPreconditionNotMet", "Delaying installer pod for revision %d on node %q: %s", + currNodeState.TargetRevision, currNodeState.NodeName, reason) + return true, installerPreconditionRequeueDuration, nil, nil, nil + } + } + if err := c.ensureInstallerPod(ctx, operatorSpec, currNodeState); err != nil { c.eventRecorder.Warningf("InstallerPodFailed", "Failed to create installer pod for revision %d count %d on node %q: %v", currNodeState.TargetRevision, currNodeState.LastFailedCount, currNodeState.NodeName, err) diff --git a/vendor/github.com/openshift/library-go/pkg/operator/staticpod/controllers.go b/vendor/github.com/openshift/library-go/pkg/operator/staticpod/controllers.go index 62722c66c3..4d0c4cef78 100644 --- a/vendor/github.com/openshift/library-go/pkg/operator/staticpod/controllers.go +++ b/vendor/github.com/openshift/library-go/pkg/operator/staticpod/controllers.go @@ -66,6 +66,7 @@ type staticPodOperatorControllerBuilder struct { installCommand []string installerPodMutationFunc installer.InstallerPodMutationFunc minReadyDuration time.Duration + installerPrecondition installer.InstallerPreconditionFunc enableStartMonitor func() (bool, error) // pruning information @@ -114,6 +115,9 @@ type Builder interface { WithUnrevisionedCerts(certDir string, certConfigMaps, certSecrets []installer.UnrevisionedResource) Builder WithInstaller(command []string) Builder WithMinReadyDuration(minReadyDuration time.Duration) Builder + // WithInstallerPrecondition sets a precondition consulted immediately before an installer pod is + // created for a node; see installer.InstallerPreconditionFunc. + WithInstallerPrecondition(precondition installer.InstallerPreconditionFunc) Builder WithStartupMonitor(enabledStartupMonitor func() (bool, error)) Builder // WithExtraNodeSelector Informs controllers to handle extra nodes as well as master nodes. @@ -185,6 +189,11 @@ func (b *staticPodOperatorControllerBuilder) WithMinReadyDuration(minReadyDurati return b } +func (b *staticPodOperatorControllerBuilder) WithInstallerPrecondition(precondition installer.InstallerPreconditionFunc) Builder { + b.installerPrecondition = precondition + return b +} + func (b *staticPodOperatorControllerBuilder) WithStartupMonitor(enabledStartupMonitor func() (bool, error)) Builder { b.enableStartMonitor = enabledStartupMonitor return b @@ -296,6 +305,8 @@ func (b *staticPodOperatorControllerBuilder) ToControllers() (manager.Controller b.installerPodMutationFunc, ).WithMinReadyDuration( b.minReadyDuration, + ).WithInstallerPrecondition( + b.installerPrecondition, ), 1) manager.WithController(installerstate.NewInstallerStateController(