From 587014fe79b908edf339a850bc983299784c5fa2 Mon Sep 17 00:00:00 2001 From: ajanikow <12255597+ajanikow@users.noreply.github.com> Date: Thu, 30 Jul 2020 08:48:38 +0000 Subject: [PATCH 1/3] Change Rotation by Annotation order --- .../v1/deployment_status_members.go | 54 ++++++++---- .../v1/deployment_status_members_test.go | 86 +++++++++++++++++++ .../reconcile/plan_builder_rotate_upgrade.go | 19 +++- 3 files changed, 142 insertions(+), 17 deletions(-) create mode 100644 pkg/apis/deployment/v1/deployment_status_members_test.go diff --git a/pkg/apis/deployment/v1/deployment_status_members.go b/pkg/apis/deployment/v1/deployment_status_members.go index d261a6026..be3c58526 100644 --- a/pkg/apis/deployment/v1/deployment_status_members.go +++ b/pkg/apis/deployment/v1/deployment_status_members.go @@ -84,23 +84,45 @@ func (ds DeploymentStatusMembers) ElementByID(id string) (MemberStatus, ServerGr // If the callback returns an error, this error is returned and the callback is // not called for the remaining groups. func (ds DeploymentStatusMembers) ForeachServerGroup(cb func(group ServerGroup, list MemberStatusList) error) error { - if err := cb(ServerGroupSingle, ds.Single); err != nil { - return maskAny(err) - } - if err := cb(ServerGroupAgents, ds.Agents); err != nil { - return maskAny(err) - } - if err := cb(ServerGroupDBServers, ds.DBServers); err != nil { - return maskAny(err) - } - if err := cb(ServerGroupCoordinators, ds.Coordinators); err != nil { - return maskAny(err) - } - if err := cb(ServerGroupSyncMasters, ds.SyncMasters); err != nil { - return maskAny(err) + return ds.ForeachServerInGroups(cb, AllServerGroups...) +} + +func (ds DeploymentStatusMembers) ForeachServerInGroups(cb func(group ServerGroup, list MemberStatusList) error, groups ...ServerGroup) error { + for _, group := range groups { + if err := ds.ForServerGroup(cb, group); err != nil { + return err + } } - if err := cb(ServerGroupSyncWorkers, ds.SyncWorkers); err != nil { - return maskAny(err) + + return nil +} + +func (ds DeploymentStatusMembers) ForServerGroup(cb func(group ServerGroup, list MemberStatusList) error, group ServerGroup) error { + switch group { + case ServerGroupSingle: + if err := cb(ServerGroupSingle, ds.Single); err != nil { + return maskAny(err) + } + case ServerGroupAgents: + if err := cb(ServerGroupAgents, ds.Agents); err != nil { + return maskAny(err) + } + case ServerGroupDBServers: + if err := cb(ServerGroupDBServers, ds.DBServers); err != nil { + return maskAny(err) + } + case ServerGroupCoordinators: + if err := cb(ServerGroupCoordinators, ds.Coordinators); err != nil { + return maskAny(err) + } + case ServerGroupSyncMasters: + if err := cb(ServerGroupSyncMasters, ds.SyncMasters); err != nil { + return maskAny(err) + } + case ServerGroupSyncWorkers: + if err := cb(ServerGroupSyncWorkers, ds.SyncWorkers); err != nil { + return maskAny(err) + } } return nil } diff --git a/pkg/apis/deployment/v1/deployment_status_members_test.go b/pkg/apis/deployment/v1/deployment_status_members_test.go new file mode 100644 index 000000000..ff6f8eb4b --- /dev/null +++ b/pkg/apis/deployment/v1/deployment_status_members_test.go @@ -0,0 +1,86 @@ +// +// DISCLAIMER +// +// Copyright 2020 ArangoDB GmbH, Cologne, Germany +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// Copyright holder is ArangoDB GmbH, Cologne, Germany +// +// Author Adam Janikowski +// + +package v1 + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func newMemberList() DeploymentStatusMembers { + return DeploymentStatusMembers{ + Single: MemberStatusList{{ID: ServerGroupSingle.AsRole()}}, + Agents: MemberStatusList{{ID: ServerGroupAgents.AsRole()}}, + DBServers: MemberStatusList{{ID: ServerGroupDBServers.AsRole()}}, + Coordinators: MemberStatusList{{ID: ServerGroupCoordinators.AsRole()}}, + SyncMasters: MemberStatusList{{ID: ServerGroupSyncMasters.AsRole()}}, + SyncWorkers: MemberStatusList{{ID: ServerGroupSyncWorkers.AsRole()}}, + } +} + +func Test_StatusMemberList_EnsureDefaultExecutionOrder(t *testing.T) { + statusMembers := newMemberList() + + order := AllServerGroups + + orderIndex := 0 + + statusMembers.ForeachServerGroup(func(group ServerGroup, list MemberStatusList) error { + require.True(t, orderIndex < len(order)) + + require.Equal(t, order[orderIndex], group) + + require.Len(t, list, 1) + + require.Equal(t, order[orderIndex].AsRole(), list[0].ID) + + orderIndex += 1 + + return nil + }) +} + +func Test_StatusMemberList_CustomExecutionOrder(t *testing.T) { + statusMembers := newMemberList() + + order := []ServerGroup{ + ServerGroupDBServers, + } + + orderIndex := 0 + + statusMembers.ForeachServerInGroups(func(group ServerGroup, list MemberStatusList) error { + require.True(t, orderIndex < len(order)) + + require.Equal(t, order[orderIndex], group) + + require.Len(t, list, 1) + + require.Equal(t, order[orderIndex].AsRole(), list[0].ID) + + orderIndex += 1 + + return nil + }, order...) +} diff --git a/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go b/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go index 8a3f460dd..72a8faa70 100644 --- a/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go +++ b/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go @@ -109,6 +109,21 @@ func createRotateOrUpgradePlanInternal(log zerolog.Logger, apiObject k8sutil.API // Only rotate/upgrade 1 pod at a time continue } + } + return nil + }) + + status.Members.ForeachServerInGroups(func(group api.ServerGroup, members api.MemberStatusList) error { + for _, m := range members { + if m.Phase != api.MemberPhaseCreated || m.PodName == "" { + // Only rotate when phase is created + continue + } + + pod, found := cachedStatus.Pod(m.PodName) + if !found { + continue + } if pod.Annotations != nil { if _, ok := pod.Annotations[deployment.ArangoDeploymentPodRotateAnnotation]; ok { @@ -116,8 +131,10 @@ func createRotateOrUpgradePlanInternal(log zerolog.Logger, apiObject k8sutil.API } } } + return nil - }) + }, api.ServerGroupSingle, api.ServerGroupAgents, api.ServerGroupCoordinators, // Change order of execution - Coordinators and Agents should be executed before DBServer to save time + api.ServerGroupDBServers, api.ServerGroupSyncMasters, api.ServerGroupSyncWorkers) if upgradeNotAllowed { context.CreateEvent(k8sutil.NewUpgradeNotAllowedEvent(apiObject, fromVersion, toVersion, fromLicense, toLicense)) From 2f4f64f54aa2c859a481c164881031c8dc173a71 Mon Sep 17 00:00:00 2001 From: ajanikow <12255597+ajanikow@users.noreply.github.com> Date: Thu, 30 Jul 2020 08:56:35 +0000 Subject: [PATCH 2/3] Do not rotate more than one member in single plan execution --- pkg/deployment/reconcile/plan_builder_rotate_upgrade.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go b/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go index 72a8faa70..ef70092bc 100644 --- a/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go +++ b/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go @@ -120,6 +120,11 @@ func createRotateOrUpgradePlanInternal(log zerolog.Logger, apiObject k8sutil.API continue } + if !newPlan.IsEmpty() { + // Only rotate/upgrade 1 pod at a time + continue + } + pod, found := cachedStatus.Pod(m.PodName) if !found { continue From cc5892fd1d17d76de07bd01e7ccf9ca35792d7a5 Mon Sep 17 00:00:00 2001 From: ajanikow <12255597+ajanikow@users.noreply.github.com> Date: Thu, 30 Jul 2020 09:55:58 +0000 Subject: [PATCH 3/3] RV --- CHANGELOG.md | 1 + .../reconcile/plan_builder_rotate_upgrade.go | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a91ba965..597bb518f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## [master](https://github.com/arangodb/kube-arangodb/tree/master) (N/A) - Add Labels and Annotations to ServiceMonitor +- Change rotation by annotation order (coordinator before dbserver) ## [1.0.4](https://github.com/arangodb/kube-arangodb/tree/1.0.4) (2020-07-28) - Add Encryption Key rotation feature for ArangoDB EE 3.7+ diff --git a/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go b/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go index ef70092bc..eba20009e 100644 --- a/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go +++ b/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go @@ -37,8 +37,19 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// createRotateOrUpgradePlan goes over all pods to check if an upgrade or rotate is needed. +var ( + // rotationByAnnotationOrder - Change order of execution - Coordinators and Agents should be executed before DBServer to save time + rotationByAnnotationOrder = []api.ServerGroup{ + api.ServerGroupSingle, + api.ServerGroupAgents, + api.ServerGroupCoordinators, + api.ServerGroupDBServers, + api.ServerGroupSyncMasters, + api.ServerGroupSyncWorkers, + } +) +// createRotateOrUpgradePlan goes over all pods to check if an upgrade or rotate is needed. func createRotateOrUpgradePlan(ctx context.Context, log zerolog.Logger, apiObject k8sutil.APIObject, spec api.DeploymentSpec, status api.DeploymentStatus, @@ -138,8 +149,7 @@ func createRotateOrUpgradePlanInternal(log zerolog.Logger, apiObject k8sutil.API } return nil - }, api.ServerGroupSingle, api.ServerGroupAgents, api.ServerGroupCoordinators, // Change order of execution - Coordinators and Agents should be executed before DBServer to save time - api.ServerGroupDBServers, api.ServerGroupSyncMasters, api.ServerGroupSyncWorkers) + }, rotationByAnnotationOrder...) if upgradeNotAllowed { context.CreateEvent(k8sutil.NewUpgradeNotAllowedEvent(apiObject, fromVersion, toVersion, fromLicense, toLicense))