Skip to content

Commit 9fb46bc

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #1571 from hasbro17/cherry-pick-1555-to-release-4.20
[release-4.20] OCPBUGS-77313: Wait for revision stability before removing etcd members
2 parents d4171c7 + d01fcc1 commit 9fb46bc

3 files changed

Lines changed: 212 additions & 0 deletions

File tree

‎pkg/operator/ceohelpers/common.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,16 @@ func VotingMemberIPListSet(ctx context.Context, cli etcdcli.EtcdClient) (sets.St
202202
return currentVotingMemberIPListSet, nil
203203
}
204204

205+
// MemberIPSetFromConfigMap extracts member IP addresses from the etcd-endpoints configmap
206+
// into a set for comparison with live etcd membership
207+
func MemberIPSetFromConfigMap(cm *corev1.ConfigMap) sets.String {
208+
memberIPs := sets.NewString()
209+
for _, ip := range cm.Data {
210+
memberIPs.Insert(ip)
211+
}
212+
return memberIPs
213+
}
214+
205215
// RevisionRolloutInProgress will return true if any node status reports its target revision is different from the current revision and the latest known revision.
206216
func RevisionRolloutInProgress(status operatorv1.StaticPodOperatorStatus) bool {
207217
latestRevision := status.LatestAvailableRevision

‎pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,45 @@ func (c *clusterMemberRemovalController) sync(ctx context.Context, syncCtx facto
112112
return nil
113113
}
114114

115+
// Removing multiple members in the middle of a revision rollout can cause API unavailability
116+
// when simultaneously deleting multiple machines with the controlplanemachineset "OnDelete" strategy,
117+
// i.e we have multiple machines pending deletion and multiple new replacements added
118+
//
119+
// This controller currently has conditions to allow scaling down unhealthy members whose machines
120+
// are pending deletion. However a revision rollout can temporarily make an etcd member seem unhealthy while
121+
// the etcd pod is reinstalled to the latest revision.
122+
// This is different from when the member is indefinitely unhealthy when the revision is stable.
123+
//
124+
// Additionally the EtcdEndpointsController pauses while a revision rollout is in progress
125+
// So initially if the etcd-endpoints configmap is updated from 3->4 when the first replacement machine
126+
// is added to the membership, a revision rollout will start and the configmap won't update in this period.
127+
// But the ClusterMemberRemovalController will still delete a seemingly unhealthy machine during rollout
128+
// The API servers on the old revision will neither see the new replacement etcd endpoint, and will also
129+
// be using a removed member's endpoint.
130+
//
131+
// Moreover the EtcdEndpointsController uses the live etcd membership list to make scale down considerations for etcd quorum so the etcd-endpoints configmap always lags behind it.
132+
//
133+
// So the EtcdEndpointsController skips until the revision is stable so we remove members one at a time and unhealthy members are truly unhealthy
134+
revisionStable, err := ceohelpers.IsRevisionStable(c.operatorClient)
135+
if err != nil {
136+
return fmt.Errorf("couldn't determine stability of revisions: %w", err)
137+
}
138+
if !revisionStable {
139+
klog.V(2).Infof("skipping due to revision in progress")
140+
return nil
141+
}
142+
143+
// Ensure the live etcd membership matches the etcd-endpoints configmap.
144+
// This prevents removing multiple members in quick succession before the configmap is updated
145+
// and propagated through a revision rollout.
146+
etcdEndpointsUpdated, err := c.isEtcdEndpointsUpdated(ctx)
147+
if err != nil {
148+
return fmt.Errorf("failed to check if etcd endpoints are updated: %w", err)
149+
}
150+
if !etcdEndpointsUpdated {
151+
return nil
152+
}
153+
115154
// only attempt to scale down if the machine API is functional
116155
if isFunctional, err := c.machineAPIChecker.IsFunctional(); err != nil {
117156
return err
@@ -513,3 +552,28 @@ func machineFailureDomainCountByIndex(machines []*machinev1beta1.Machine) map[in
513552
}
514553
return count
515554
}
555+
556+
// isEtcdEndpointsUpdated checks whether the live etcd membership matches the etcd-endpoints configmap.
557+
// This prevents removing multiple members in quick succession before the configmap is updated
558+
// and propagated through a revision rollout.
559+
func (c *clusterMemberRemovalController) isEtcdEndpointsUpdated(ctx context.Context) (bool, error) {
560+
liveVotingMemberIPs, err := ceohelpers.VotingMemberIPListSet(ctx, c.etcdClient)
561+
if err != nil {
562+
return false, fmt.Errorf("failed to get live voting member IPs: %w", err)
563+
}
564+
565+
etcdEndpointsConfigMap, err := c.configMapListerForTargetNamespace.Get("etcd-endpoints")
566+
if err != nil {
567+
return false, fmt.Errorf("failed to get etcd-endpoints configmap: %w", err)
568+
}
569+
570+
configMapMemberIPs := ceohelpers.MemberIPSetFromConfigMap(etcdEndpointsConfigMap)
571+
572+
if !liveVotingMemberIPs.Equal(configMapMemberIPs) {
573+
klog.V(2).Infof("skipping member removal: live etcd membership differs from etcd-endpoints configmap. Live members: %v, ConfigMap members: %v",
574+
liveVotingMemberIPs.List(), configMapMemberIPs.List())
575+
return false, nil
576+
}
577+
578+
return true, nil
579+
}

‎pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller_test.go‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1292,6 +1292,144 @@ func wellKnownMasterMachines() []runtime.Object {
12921292
}
12931293
}
12941294

1295+
func TestIsEtcdEndpointsUpdated(t *testing.T) {
1296+
scenarios := []struct {
1297+
name string
1298+
initialObjectsForConfigMapTargetNSLister []runtime.Object
1299+
initialEtcdMemberList []*etcdserverpb.Member
1300+
expectedResult bool
1301+
expectError bool
1302+
expectedErrorMsg string
1303+
}{
1304+
{
1305+
name: "live membership equals configmap membership",
1306+
initialObjectsForConfigMapTargetNSLister: []runtime.Object{
1307+
&corev1.ConfigMap{
1308+
ObjectMeta: metav1.ObjectMeta{Name: "etcd-endpoints", Namespace: "openshift-etcd"},
1309+
Data: map[string]string{
1310+
"m-1": "10.0.139.78",
1311+
"m-2": "10.0.139.79",
1312+
"m-3": "10.0.139.80",
1313+
},
1314+
},
1315+
},
1316+
initialEtcdMemberList: []*etcdserverpb.Member{
1317+
{Name: "m-1", ID: 1, PeerURLs: []string{"https://10.0.139.78:1234"}},
1318+
{Name: "m-2", ID: 2, PeerURLs: []string{"https://10.0.139.79:1234"}},
1319+
{Name: "m-3", ID: 3, PeerURLs: []string{"https://10.0.139.80:1234"}},
1320+
},
1321+
expectedResult: true,
1322+
expectError: false,
1323+
},
1324+
{
1325+
name: "live membership has more members than configmap (scaling up)",
1326+
initialObjectsForConfigMapTargetNSLister: []runtime.Object{
1327+
&corev1.ConfigMap{
1328+
ObjectMeta: metav1.ObjectMeta{Name: "etcd-endpoints", Namespace: "openshift-etcd"},
1329+
Data: map[string]string{
1330+
"m-1": "10.0.139.78",
1331+
"m-2": "10.0.139.79",
1332+
"m-3": "10.0.139.80",
1333+
},
1334+
},
1335+
},
1336+
initialEtcdMemberList: []*etcdserverpb.Member{
1337+
{Name: "m-1", ID: 1, PeerURLs: []string{"https://10.0.139.78:1234"}},
1338+
{Name: "m-2", ID: 2, PeerURLs: []string{"https://10.0.139.79:1234"}},
1339+
{Name: "m-3", ID: 3, PeerURLs: []string{"https://10.0.139.80:1234"}},
1340+
{Name: "m-4", ID: 4, PeerURLs: []string{"https://10.0.139.81:1234"}},
1341+
},
1342+
expectedResult: false,
1343+
expectError: false,
1344+
},
1345+
{
1346+
name: "live membership has fewer members than configmap (scaling down)",
1347+
initialObjectsForConfigMapTargetNSLister: []runtime.Object{
1348+
&corev1.ConfigMap{
1349+
ObjectMeta: metav1.ObjectMeta{Name: "etcd-endpoints", Namespace: "openshift-etcd"},
1350+
Data: map[string]string{
1351+
"m-1": "10.0.139.78",
1352+
"m-2": "10.0.139.79",
1353+
"m-3": "10.0.139.80",
1354+
"m-4": "10.0.139.81",
1355+
},
1356+
},
1357+
},
1358+
initialEtcdMemberList: []*etcdserverpb.Member{
1359+
{Name: "m-1", ID: 1, PeerURLs: []string{"https://10.0.139.78:1234"}},
1360+
{Name: "m-2", ID: 2, PeerURLs: []string{"https://10.0.139.79:1234"}},
1361+
{Name: "m-3", ID: 3, PeerURLs: []string{"https://10.0.139.80:1234"}},
1362+
},
1363+
expectedResult: false,
1364+
expectError: false,
1365+
},
1366+
{
1367+
name: "live membership differs from configmap (different IPs)",
1368+
initialObjectsForConfigMapTargetNSLister: []runtime.Object{
1369+
&corev1.ConfigMap{
1370+
ObjectMeta: metav1.ObjectMeta{Name: "etcd-endpoints", Namespace: "openshift-etcd"},
1371+
Data: map[string]string{
1372+
"m-1": "10.0.139.78",
1373+
"m-2": "10.0.139.79",
1374+
"m-3": "10.0.139.80",
1375+
},
1376+
},
1377+
},
1378+
initialEtcdMemberList: []*etcdserverpb.Member{
1379+
{Name: "m-1", ID: 1, PeerURLs: []string{"https://10.0.139.78:1234"}},
1380+
{Name: "m-2", ID: 2, PeerURLs: []string{"https://10.0.139.79:1234"}},
1381+
{Name: "m-4", ID: 4, PeerURLs: []string{"https://10.0.139.81:1234"}},
1382+
},
1383+
expectedResult: false,
1384+
expectError: false,
1385+
},
1386+
{
1387+
name: "configmap not found",
1388+
initialObjectsForConfigMapTargetNSLister: []runtime.Object{},
1389+
initialEtcdMemberList: []*etcdserverpb.Member{
1390+
{Name: "m-1", ID: 1, PeerURLs: []string{"https://10.0.139.78:1234"}},
1391+
},
1392+
expectedResult: false,
1393+
expectError: true,
1394+
expectedErrorMsg: "failed to get etcd-endpoints configmap",
1395+
},
1396+
}
1397+
1398+
for _, scenario := range scenarios {
1399+
t.Run(scenario.name, func(t *testing.T) {
1400+
// setup test data
1401+
configMapTargetNSIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
1402+
for _, initialObj := range scenario.initialObjectsForConfigMapTargetNSLister {
1403+
configMapTargetNSIndexer.Add(initialObj)
1404+
}
1405+
configMapTargetNSLister := corev1listers.NewConfigMapLister(configMapTargetNSIndexer).ConfigMaps("openshift-etcd")
1406+
1407+
fakeEtcdClient, err := etcdcli.NewFakeEtcdClient(scenario.initialEtcdMemberList)
1408+
require.NoError(t, err)
1409+
1410+
// create controller instance
1411+
target := clusterMemberRemovalController{
1412+
etcdClient: fakeEtcdClient,
1413+
configMapListerForTargetNamespace: configMapTargetNSLister,
1414+
}
1415+
1416+
// act
1417+
result, err := target.isEtcdEndpointsUpdated(context.TODO())
1418+
1419+
// assert
1420+
if scenario.expectError {
1421+
require.Error(t, err)
1422+
if scenario.expectedErrorMsg != "" {
1423+
require.Contains(t, err.Error(), scenario.expectedErrorMsg)
1424+
}
1425+
} else {
1426+
require.NoError(t, err)
1427+
require.Equal(t, scenario.expectedResult, result, "isEtcdEndpointsUpdated returned unexpected result")
1428+
}
1429+
})
1430+
}
1431+
}
1432+
12951433
var wellKnownReplicasCountSet = `
12961434
{
12971435
"controlPlane": {"replicas": 3}

0 commit comments

Comments
 (0)