From ac404db9617047252ccc2b7ab9e6b51b49632886 Mon Sep 17 00:00:00 2001 From: ajanikow <12255597+ajanikow@users.noreply.github.com> Date: Wed, 15 Apr 2020 06:51:58 +0000 Subject: [PATCH 1/2] feature/fix_uptodate_condition --- CHANGELOG.md | 1 + pkg/apis/deployment/v1/plan.go | 2 + pkg/deployment/deployment_inspector.go | 64 +++++++++++++------ pkg/deployment/reconcile/action_idle.go | 60 +++++++++++++++++ pkg/deployment/reconcile/plan_builder.go | 24 ++++--- .../reconcile/plan_builder_rotate_upgrade.go | 7 +- 6 files changed, 126 insertions(+), 32 deletions(-) create mode 100644 pkg/deployment/reconcile/action_idle.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ddd043c59..fa547366f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Change Log ## [master](https://github.com/arangodb/kube-arangodb/tree/master) (N/A) +- Added additional checks in UpToDate condition - Added extended Rotation check for Cluster mode - Removed old rotation logic (rotation of ArangoDeployment may be enforced after Operator upgrade) - Added UpToDate condition in ArangoDeployment Status diff --git a/pkg/apis/deployment/v1/plan.go b/pkg/apis/deployment/v1/plan.go index 28ebfd364..609ccd547 100644 --- a/pkg/apis/deployment/v1/plan.go +++ b/pkg/apis/deployment/v1/plan.go @@ -33,6 +33,8 @@ import ( type ActionType string const ( + // ActionTypeIdle causes a plan to be recalculated. + ActionTypeIdle ActionType = "Idle" // ActionTypeAddMember causes a member to be added. ActionTypeAddMember ActionType = "AddMember" // ActionTypeRemoveMember causes a member to be removed. diff --git a/pkg/deployment/deployment_inspector.go b/pkg/deployment/deployment_inspector.go index 4e9cc1355..a176317d0 100644 --- a/pkg/deployment/deployment_inspector.go +++ b/pkg/deployment/deployment_inspector.go @@ -119,14 +119,8 @@ func (d *Deployment) inspectDeploymentWithError(ctx context.Context, lastInterva return minInspectionInterval, errors.Wrapf(err, "Calculation of spec failed") } else { condition, exists := status.Conditions.Get(api.ConditionTypeUpToDate) - if (checksum != status.AppliedVersion && (!exists || condition.IsTrue())) || - (checksum == status.AppliedVersion && (!exists || !condition.IsTrue())) { - if err = d.WithStatusUpdate(func(s *api.DeploymentStatus) bool { - if checksum == status.AppliedVersion { - return s.Conditions.Update(api.ConditionTypeUpToDate, true, "Everything is UpToDate", "Spec applied") - } - return s.Conditions.Update(api.ConditionTypeUpToDate, false, "Spec Changed", "Spec Object changed. Waiting until plan will be applied") - }); err != nil { + if checksum != status.AppliedVersion && (!exists || condition.IsTrue()) { + if err = d.updateCondition(api.ConditionTypeUpToDate, false, "Spec Changed", "Spec Object changed. Waiting until plan will be applied"); err != nil { return minInspectionInterval, errors.Wrapf(err, "Unable to update UpToDate condition") } @@ -185,8 +179,37 @@ func (d *Deployment) inspectDeploymentWithError(ctx context.Context, lastInterva } // Create scale/update plan - if err := d.reconciler.CreatePlan(); err != nil { + if err, updated := d.reconciler.CreatePlan(); err != nil { return minInspectionInterval, errors.Wrapf(err, "Plan creation failed") + } else if updated { + return minInspectionInterval, nil + } + + if d.apiObject.Status.Plan.IsEmpty() && status.AppliedVersion != checksum { + if err := d.WithStatusUpdate(func(s *api.DeploymentStatus) bool { + s.AppliedVersion = checksum + return true + }); err != nil { + return minInspectionInterval, errors.Wrapf(err, "Unable to update UpToDate condition") + } + + return minInspectionInterval, nil + } else if status.AppliedVersion == checksum { + if !status.Plan.IsEmpty() && status.Conditions.IsTrue(api.ConditionTypeUpToDate) { + if err = d.updateCondition(api.ConditionTypeUpToDate, false, "Plan is not empty", "There are pending operations in plan"); err != nil { + return minInspectionInterval, errors.Wrapf(err, "Unable to update UpToDate condition") + } + + return minInspectionInterval, nil + } + + if status.Plan.IsEmpty() && !status.Conditions.IsTrue(api.ConditionTypeUpToDate) { + if err = d.updateCondition(api.ConditionTypeUpToDate, true, "Spec is Up To Date", "Spec is Up To Date"); err != nil { + return minInspectionInterval, errors.Wrapf(err, "Unable to update UpToDate condition") + } + + return minInspectionInterval, nil + } } // Execute current step of scale/update plan @@ -196,18 +219,6 @@ func (d *Deployment) inspectDeploymentWithError(ctx context.Context, lastInterva } if retrySoon { nextInterval = minInspectionInterval - } else { - // Do not retry - so plan is empty - if status.AppliedVersion != checksum { - if err := d.WithStatusUpdate(func(s *api.DeploymentStatus) bool { - s.AppliedVersion = checksum - return true - }); err != nil { - return minInspectionInterval, errors.Wrapf(err, "Unable to update UpToDate condition") - } - - return minInspectionInterval, nil - } } // Create access packages @@ -279,3 +290,14 @@ func (d *Deployment) triggerInspection() { func (d *Deployment) triggerCRDInspection() { d.inspectCRDTrigger.Trigger() } + +func (d *Deployment) updateCondition(conditionType api.ConditionType, status bool, reason, message string) error { + d.deps.Log.Info().Str("condition", string(conditionType)).Bool("status", status).Str("reason", reason).Str("message", message).Msg("Updated condition") + if err := d.WithStatusUpdate(func(s *api.DeploymentStatus) bool { + return s.Conditions.Update(conditionType, status, reason, message) + }); err != nil { + return errors.Wrapf(err, "Unable to update condition") + } + + return nil +} diff --git a/pkg/deployment/reconcile/action_idle.go b/pkg/deployment/reconcile/action_idle.go new file mode 100644 index 000000000..9fa513841 --- /dev/null +++ b/pkg/deployment/reconcile/action_idle.go @@ -0,0 +1,60 @@ +// +// 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 reconcile + +import ( + "context" + + api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1" + "github.com/rs/zerolog" +) + +func init() { + registerAction(api.ActionTypeIdle, newIdleAction) +} + +// newIdleAction creates a new Action that implements the given +// planned Idle action. +func newIdleAction(log zerolog.Logger, action api.Action, actionCtx ActionContext) Action { + a := &actionIdle{} + + a.actionImpl = newActionImplDefRef(log, action, actionCtx, addMemberTimeout) + + return a +} + +// actionIdle implements an Idle. +type actionIdle struct { + // actionImpl implement timeout and member id functions + actionImpl + + // actionEmptyCheckProgress implement check progress with empty implementation + actionEmptyCheckProgress +} + +// Start performs the start of the action. +// Returns true if the action is completely finished, false in case +// the start time needs to be recorded and a ready condition needs to be checked. +func (a *actionIdle) Start(ctx context.Context) (bool, error) { + return true, nil +} diff --git a/pkg/deployment/reconcile/plan_builder.go b/pkg/deployment/reconcile/plan_builder.go index aca1bfe97..f122f1ad4 100644 --- a/pkg/deployment/reconcile/plan_builder.go +++ b/pkg/deployment/reconcile/plan_builder.go @@ -52,12 +52,12 @@ type upgradeDecision struct { // CreatePlan considers the current specification & status of the deployment creates a plan to // get the status in line with the specification. // If a plan already exists, nothing is done. -func (d *Reconciler) CreatePlan() error { +func (d *Reconciler) CreatePlan() (error, bool) { // Get all current pods pods, err := d.context.GetOwnedPods() if err != nil { d.log.Debug().Err(err).Msg("Failed to get owned pods") - return maskAny(err) + return maskAny(err), false } // Create plan @@ -69,19 +69,19 @@ func (d *Reconciler) CreatePlan() error { // If not change, we're done if !changed { - return nil + return nil, false } // Save plan if len(newPlan) == 0 { // Nothing to do - return nil + return nil, false } status.Plan = newPlan if err := d.context.UpdateStatus(status, lastVersion); err != nil { - return maskAny(err) + return maskAny(err), false } - return nil + return nil, true } func fetchAgency(log zerolog.Logger, @@ -112,6 +112,7 @@ func createPlan(log zerolog.Logger, apiObject k8sutil.APIObject, currentPlan api.Plan, spec api.DeploymentSpec, status api.DeploymentStatus, pods []v1.Pod, context PlanBuilderContext) (api.Plan, bool) { + if !currentPlan.IsEmpty() { // Plan already exists, complete that first return currentPlan, false @@ -176,7 +177,8 @@ func createPlan(log zerolog.Logger, apiObject k8sutil.APIObject, // Ensure that we were able to get agency info if len(plan) == 0 && agencyErr != nil { log.Err(agencyErr).Msg("unable to build further plan without access to agency") - return plan, false + return append(plan, + api.NewAction(api.ActionTypeIdle, api.ServerGroupUnknown, "")), true } // Check for cleaned out dbserver in created state @@ -200,7 +202,13 @@ func createPlan(log zerolog.Logger, apiObject k8sutil.APIObject, // Check for the need to rotate one or more members if plan.IsEmpty() { - plan = createRotateOrUpgradePlan(log, apiObject, spec, status, context, pods) + newPlan, idle := createRotateOrUpgradePlan(log, apiObject, spec, status, context, pods) + if idle { + plan = append(plan, + api.NewAction(api.ActionTypeIdle, api.ServerGroupUnknown, "")) + } else { + plan = append(plan, newPlan...) + } } // Check for the need to rotate TLS certificate of a members diff --git a/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go b/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go index d4678d3f1..e53b0c6a5 100644 --- a/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go +++ b/pkg/deployment/reconcile/plan_builder_rotate_upgrade.go @@ -35,7 +35,7 @@ import ( // createRotateOrUpgradePlan goes over all pods to check if an upgrade or rotate is needed. func createRotateOrUpgradePlan(log zerolog.Logger, apiObject k8sutil.APIObject, spec api.DeploymentSpec, - status api.DeploymentStatus, context PlanBuilderContext, pods []core.Pod) api.Plan { + status api.DeploymentStatus, context PlanBuilderContext, pods []core.Pod) (api.Plan, bool) { var newPlan api.Plan var upgradeNotAllowed bool @@ -103,12 +103,13 @@ func createRotateOrUpgradePlan(log zerolog.Logger, apiObject k8sutil.APIObject, } else if !newPlan.IsEmpty() { if clusterReadyForUpgrade(context) { // Use the new plan - return newPlan + return newPlan, false } else { log.Info().Msg("Pod needs upgrade but cluster is not ready. Either some shards are not in sync or some member is not ready.") + return nil, true } } - return nil + return nil, false } // podNeedsUpgrading decides if an upgrade of the pod is needed (to comply with From 4991f1c3c941bdb58779ea2489a469b0465e310d Mon Sep 17 00:00:00 2001 From: ajanikow <12255597+ajanikow@users.noreply.github.com> Date: Wed, 15 Apr 2020 07:36:03 +0000 Subject: [PATCH 2/2] Fix UT --- pkg/deployment/reconcile/plan_builder_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/deployment/reconcile/plan_builder_test.go b/pkg/deployment/reconcile/plan_builder_test.go index da7905fed..e8a311f91 100644 --- a/pkg/deployment/reconcile/plan_builder_test.go +++ b/pkg/deployment/reconcile/plan_builder_test.go @@ -776,7 +776,7 @@ func TestCreatePlan(t *testing.T) { if testCase.Helper != nil { testCase.Helper(testCase.context.ArangoDeployment) } - err := r.CreatePlan() + err, _ := r.CreatePlan() // Assert if testCase.ExpectedEvent != nil {