From 708b455960c5bb29e521cf7c546040e0fecee158 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Thu, 16 Sep 2021 12:14:28 +0200 Subject: [PATCH 01/14] change log.level on the fly --- pkg/apis/deployment/v1/plan.go | 2 + pkg/deployment/reconcile/action_helper.go | 2 +- .../action_runtime_container_args_udpate.go | 289 ++++++++++++++++++ pkg/deployment/reconcile/plan_executor.go | 2 +- pkg/deployment/rotation/arangod_containers.go | 26 +- .../rotation/arangod_containers_test.go | 2 + pkg/deployment/rotation/check.go | 1 + pkg/util/k8sutil/pair.go | 14 + pkg/util/strings.go | 17 ++ 9 files changed, 351 insertions(+), 4 deletions(-) create mode 100644 pkg/deployment/reconcile/action_runtime_container_args_udpate.go diff --git a/pkg/apis/deployment/v1/plan.go b/pkg/apis/deployment/v1/plan.go index f7b66b076..a689ec3a5 100644 --- a/pkg/apis/deployment/v1/plan.go +++ b/pkg/apis/deployment/v1/plan.go @@ -169,6 +169,8 @@ const ( // Runtime Updates // ActionTypeRuntimeContainerImageUpdate updates container image in runtime ActionTypeRuntimeContainerImageUpdate ActionType = "RuntimeContainerImageUpdate" + // ActionTypeRuntimeContainerArgsLogLevelUpdate updates the container's executor arguments. + ActionTypeRuntimeContainerArgsLogLevelUpdate ActionType = "RuntimeContainerArgsLogLevelUpdate" ) const ( diff --git a/pkg/deployment/reconcile/action_helper.go b/pkg/deployment/reconcile/action_helper.go index 8ce21d571..7c90373d9 100644 --- a/pkg/deployment/reconcile/action_helper.go +++ b/pkg/deployment/reconcile/action_helper.go @@ -102,7 +102,7 @@ func (a actionImpl) Timeout(deploymentSpec api.DeploymentSpec) time.Duration { return a.timeout(deploymentSpec) } -// Return the MemberID used / created in this action +// MemberID returns the member ID used / created in the current action. func (a actionImpl) MemberID() string { return *a.memberIDRef } diff --git a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go new file mode 100644 index 000000000..56da29dd0 --- /dev/null +++ b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go @@ -0,0 +1,289 @@ +// +// DISCLAIMER +// +// Copyright 2021 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 Tomasz Mielech +// + +package reconcile + +import ( + "context" + "fmt" + "strings" + + "github.com/pkg/errors" + "github.com/rs/zerolog" + core "k8s.io/api/core/v1" + + api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1" + "github.com/arangodb/kube-arangodb/pkg/deployment/rotation" + "github.com/arangodb/kube-arangodb/pkg/util/arangod" + "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" +) + +func init() { + registerAction(api.ActionTypeRuntimeContainerArgsLogLevelUpdate, runtimeContainerArgsUpdate) +} + +func runtimeContainerArgsUpdate(log zerolog.Logger, action api.Action, actionCtx ActionContext) Action { + a := &actionRuntimeContainerArgsUpdate{} + + a.actionImpl = newActionImplDefRef(log, action, actionCtx, defaultTimeout) + + return a +} + +var _ ActionReloadCachedStatus = &actionRuntimeContainerArgsUpdate{} +var _ ActionPost = &actionRuntimeContainerArgsUpdate{} + +type actionRuntimeContainerArgsUpdate struct { + // actionImpl implement timeout and member id functions + actionImpl +} + +// Post updates arguments for the specific Arango member. +func (a actionRuntimeContainerArgsUpdate) Post(ctx context.Context) error { + + a.log.Info().Msgf("Updating container args") + + m, ok := a.actionCtx.GetMemberStatusByID(a.action.MemberID) + if !ok { + a.log.Info().Msg("member is gone already") + return nil + } + + memberName := m.ArangoMemberName(a.actionCtx.GetName(), a.action.Group) + member, ok := a.actionCtx.GetCachedStatus().ArangoMember(memberName) + if !ok { + return errors.Errorf("ArangoMember %s not found", memberName) + } + + containerName, ok := a.action.GetParam(rotation.ContainerName) + if !ok { + a.log.Warn().Msgf("Unable to find action's param %s", rotation.ContainerName) + return nil + } + + updateMemberStatusArgs := func(obj *api.ArangoMember, s *api.ArangoMemberStatus) bool { + if obj.Spec.Template == nil || s.Template == nil || + obj.Spec.Template.PodSpec == nil || s.Template.PodSpec == nil { + a.log.Info().Msgf("Nil Member definition") + return false + } + + if len(obj.Spec.Template.PodSpec.Spec.Containers) != len(s.Template.PodSpec.Spec.Containers) { + a.log.Info().Msgf("Invalid size of containers") + return false + } + + for id := range obj.Spec.Template.PodSpec.Spec.Containers { + if obj.Spec.Template.PodSpec.Spec.Containers[id].Name == containerName { + if s.Template.PodSpec.Spec.Containers[id].Name != containerName { + a.log.Info().Msgf("Invalid order of containers") + return false + } + + // TODO what if additional log level was added in the meantime + s.Template.PodSpec.Spec.Containers[id].Args = obj.Spec.Template.PodSpec.Spec.Containers[id].Args + return true + } + } + + a.log.Info().Msgf("can not find the container") + + return false + } + + err := a.actionCtx.WithArangoMemberStatusUpdate(ctx, member.GetNamespace(), member.GetName(), updateMemberStatusArgs) + if err != nil { + return errors.WithMessage(err, "Error while updating member status") + } + + return nil +} + +// ReloadCachedStatus reloads the inspector cache when the action is done. +func (a actionRuntimeContainerArgsUpdate) ReloadCachedStatus() bool { + return true +} + +// Start starts the action for changing conditions on the provided member. +func (a actionRuntimeContainerArgsUpdate) Start(ctx context.Context) (bool, error) { + + m, ok := a.actionCtx.GetMemberStatusByID(a.action.MemberID) + if !ok { + a.log.Info().Msg("member is gone already") + return true, nil + } + + if !m.Phase.IsReady() { + a.log.Info().Msg("Member is not ready, unable to run update operation") + return true, nil + } + + containerName, ok := a.action.GetParam(rotation.ContainerName) + if !ok { + return true, nil + } + + memberName := m.ArangoMemberName(a.actionCtx.GetName(), a.action.Group) + member, ok := a.actionCtx.GetCachedStatus().ArangoMember(memberName) + if !ok { + // TODO only event is created on the top of that func. + return false, errors.Errorf("ArangoMember %s not found", memberName) + } + + pod, ok := a.actionCtx.GetCachedStatus().Pod(m.PodName) + if !ok { + a.log.Info().Str("podName", m.PodName).Msg("pod is not present") + return true, nil + } + + var op cmpContainer = func(containerSpec core.Container, containerStatus core.Container) error { + //diff := util.GetDifference(containerSpec.Args, containerStatus.Args) + topicsLogLevel := map[string]string{} + + for _, arg := range containerSpec.Args { + if !strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { + continue + } + + // TODO + a.log.Info().Str("arg", arg).Msg("TEST TODO the arg has changed") + + logLevelOption := k8sutil.ExtractStringToOptionPair(arg) + if len(logLevelOption.Value) > 0 { + logValueOption := k8sutil.ExtractStringToOptionPair(logLevelOption.Value) + if len(logValueOption.Value) > 0 { + // It is the topic log, e.g.: --log.level=request=INFO. + topicsLogLevel[logValueOption.Key] = logValueOption.Value + } else { + // It is the general log, e.g.: --log.level=INFO. + topicsLogLevel["general"] = logLevelOption.Value + } + } + } + + // TODO how to disable old log topics? + // TODO how to set the general log level + + if err := a.setLogLevel(ctx, topicsLogLevel); err != nil { + return errors.WithMessage(err, "can not set log level") + } + + return nil + } + + if err := checkContainer(member, pod, containerName, op); err != nil && err != api.NotFoundError { + return false, errors.WithMessagef(err, "can not check the container %s", containerName) + } + + return true, nil +} + +type cmpContainer func(spec core.Container, status core.Container) error + +func checkContainer(member *api.ArangoMember, pod *core.Pod, containerName string, action cmpContainer) error { + spec, status, err := validateMemberAndPod(member, pod) + if err != nil { + return err + } + + ok, id := getIndexContainer(pod, spec, status, containerName) + if !ok { + return api.NotFoundError + } + + return action(spec.Spec.Containers[id], status.Spec.Containers[id]) +} + +// getIndexContainer returns the index of the container from the list of containers. +func getIndexContainer(pod *core.Pod, spec *core.PodTemplateSpec, status *core.PodTemplateSpec, + containerName string) (bool, int) { + + for id := range pod.Spec.Containers { + if pod.Spec.Containers[id].Name != spec.Spec.Containers[id].Name || + pod.Spec.Containers[id].Name != status.Spec.Containers[id].Name || + pod.Spec.Containers[id].Name != containerName { + + return true, id + } + } + + return false, 0 +} + +func validateMemberAndPod(member *api.ArangoMember, pod *core.Pod) (*core.PodTemplateSpec, *core.PodTemplateSpec, error) { + + if member.Spec.Template == nil || member.Spec.Template.PodSpec == nil { + return nil, nil, fmt.Errorf("member spec is not present") + } + + if member.Status.Template == nil || member.Status.Template.PodSpec == nil { + return nil, nil, fmt.Errorf("member status is not present") + } + + if len(pod.Spec.Containers) != len(member.Spec.Template.PodSpec.Spec.Containers) { + return nil, nil, fmt.Errorf("spec container count is not equal") + } + + if len(pod.Spec.Containers) != len(member.Status.Template.PodSpec.Spec.Containers) { + return nil, nil, fmt.Errorf("status container count is not equal") + } + + return member.Spec.Template.PodSpec, member.Status.Template.PodSpec, nil +} + +// CheckProgress returns always true because it does not have to wait for any result. +func (a actionRuntimeContainerArgsUpdate) CheckProgress(_ context.Context) (bool, bool, error) { + return true, false, nil +} + +// setLogLevel sets the log's levels for the specific server. +func (a actionRuntimeContainerArgsUpdate) setLogLevel(ctx context.Context, logLevels map[string]string) error { + if len(logLevels) == 0 { + return nil + } + + ctxChild, cancel := context.WithTimeout(ctx, arangod.GetRequestTimeout()) + defer cancel() + cli, err := a.actionCtx.GetServerClient(ctxChild, a.action.Group, a.action.MemberID) + if err != nil { + return err + } + conn := cli.Connection() + + req, err := conn.NewRequest("PUT", "_admin/log/level") + if err != nil { + return err + } + + if _, err := req.SetBody(logLevels); err != nil { + return err + } + + ctxChild, cancel = context.WithTimeout(ctx, arangod.GetRequestTimeout()) + defer cancel() + resp, err := conn.Do(ctxChild, req) + if err != nil { + return err + } + + return resp.CheckStatus(200) +} diff --git a/pkg/deployment/reconcile/plan_executor.go b/pkg/deployment/reconcile/plan_executor.go index 93a3c3ff2..7a0c6240b 100644 --- a/pkg/deployment/reconcile/plan_executor.go +++ b/pkg/deployment/reconcile/plan_executor.go @@ -188,7 +188,7 @@ func (d *Reconciler) executePlan(ctx context.Context, cachedStatus inspectorInte if err := getActionPost(action, ctx); err != nil { log.Err(err).Msgf("Post action failed") - return nil, true, errors.WithStack(err) + return nil, false, errors.WithStack(err) } } else { if plan[0].StartTime.IsZero() { diff --git a/pkg/deployment/rotation/arangod_containers.go b/pkg/deployment/rotation/arangod_containers.go index 1883d76a4..1b891e328 100644 --- a/pkg/deployment/rotation/arangod_containers.go +++ b/pkg/deployment/rotation/arangod_containers.go @@ -1,7 +1,7 @@ // // DISCLAIMER // -// Copyright 2020 ArangoDB GmbH, Cologne, Germany +// Copyright 2020-2021 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. @@ -17,10 +17,17 @@ // // Copyright holder is ArangoDB GmbH, Cologne, Germany // +// Author Adam Janikowski +// Author Tomasz Mielech +// package rotation import ( + "strings" + + "github.com/arangodb/kube-arangodb/pkg/util" + api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1" "github.com/arangodb/kube-arangodb/pkg/util" "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" @@ -42,7 +49,22 @@ func containersCompare(deploymentSpec api.DeploymentSpec, group api.ServerGroup, for id := range a { if ac, bc := &a[id], &b[id]; ac.Name == k8sutil.ServerContainerName && ac.Name == bc.Name { - // Nothing to do + foundLogLevelDifference := false + for _, arg := range util.GetDifference(ac.Args, bc.Args) { + if strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { + foundLogLevelDifference = true + } + } + + if !foundLogLevelDifference { + continue + } + + plan = append(plan, builder.NewAction(api.ActionTypeRuntimeContainerArgsLogLevelUpdate). + AddParam(ContainerName, ac.Name)) + + bc.Args = ac.Args + mode = mode.And(InPlaceRotation) } else if ac.Name == bc.Name { if ac.Image != bc.Image { // Image changed diff --git a/pkg/deployment/rotation/arangod_containers_test.go b/pkg/deployment/rotation/arangod_containers_test.go index f788a82e0..e83bacdf7 100644 --- a/pkg/deployment/rotation/arangod_containers_test.go +++ b/pkg/deployment/rotation/arangod_containers_test.go @@ -144,3 +144,5 @@ func Test_InitContainers(t *testing.T) { runTestCases(t)(testCases...) }) } + +// TODO unit tests for changed args diff --git a/pkg/deployment/rotation/check.go b/pkg/deployment/rotation/check.go index 628c2f149..df2312e14 100644 --- a/pkg/deployment/rotation/check.go +++ b/pkg/deployment/rotation/check.go @@ -41,6 +41,7 @@ const ( EnforcedRotation ) +// And returns the higher value of the rotation mode. func (m Mode) And(b Mode) Mode { if m > b { return m diff --git a/pkg/util/k8sutil/pair.go b/pkg/util/k8sutil/pair.go index 406294387..39abf3489 100644 --- a/pkg/util/k8sutil/pair.go +++ b/pkg/util/k8sutil/pair.go @@ -162,3 +162,17 @@ func (o OptionPair) CompareTo(other OptionPair) int { func NewOptionPair(pairs ...OptionPair) OptionPairs { return pairs } + +// TODO test desc +func ExtractStringToOptionPair(arg string) OptionPair { + trimmed := strings.Trim(arg, " ") + index := strings.Index(trimmed, "=") + if index < 0 { + return OptionPair{Key: trimmed} + } + + return OptionPair{ + Key: trimmed[0:index], + Value: trimmed[index:], + } +} diff --git a/pkg/util/strings.go b/pkg/util/strings.go index 20a5446a9..52bc76126 100644 --- a/pkg/util/strings.go +++ b/pkg/util/strings.go @@ -63,3 +63,20 @@ func PrefixStringArray(a []string, prefix string) []string { return b } + +// GetDifference returns the elements in `compareWhat` that are not in `compareTo`. +func GetDifference(compareWhat, compareTo []string) []string { + mb := make(map[string]struct{}, len(compareTo)) + for _, x := range compareTo { + mb[x] = struct{}{} + } + + var diff []string + for _, x := range compareWhat { + if _, found := mb[x]; !found { + diff = append(diff, x) + } + } + + return diff +} From 5e17927e2536212efe7b69de8acd959b5de5f0cd Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Thu, 16 Sep 2021 20:32:27 +0200 Subject: [PATCH 02/14] fix checking the container name --- .../action_runtime_container_args_udpate.go | 21 ++++++++----------- pkg/deployment/reconcile/plan_executor.go | 3 +-- pkg/deployment/rotation/arangod_containers.go | 4 +--- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go index 56da29dd0..f4e9d8aa0 100644 --- a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go +++ b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go @@ -145,7 +145,6 @@ func (a actionRuntimeContainerArgsUpdate) Start(ctx context.Context) (bool, erro memberName := m.ArangoMemberName(a.actionCtx.GetName(), a.action.Group) member, ok := a.actionCtx.GetCachedStatus().ArangoMember(memberName) if !ok { - // TODO only event is created on the top of that func. return false, errors.Errorf("ArangoMember %s not found", memberName) } @@ -164,9 +163,6 @@ func (a actionRuntimeContainerArgsUpdate) Start(ctx context.Context) (bool, erro continue } - // TODO - a.log.Info().Str("arg", arg).Msg("TEST TODO the arg has changed") - logLevelOption := k8sutil.ExtractStringToOptionPair(arg) if len(logLevelOption.Value) > 0 { logValueOption := k8sutil.ExtractStringToOptionPair(logLevelOption.Value) @@ -180,6 +176,7 @@ func (a actionRuntimeContainerArgsUpdate) Start(ctx context.Context) (bool, erro } } + // TODO // TODO how to disable old log topics? // TODO how to set the general log level @@ -205,8 +202,8 @@ func checkContainer(member *api.ArangoMember, pod *core.Pod, containerName strin return err } - ok, id := getIndexContainer(pod, spec, status, containerName) - if !ok { + id := getIndexContainer(pod, spec, status, containerName) + if id < 0 { return api.NotFoundError } @@ -215,18 +212,18 @@ func checkContainer(member *api.ArangoMember, pod *core.Pod, containerName strin // getIndexContainer returns the index of the container from the list of containers. func getIndexContainer(pod *core.Pod, spec *core.PodTemplateSpec, status *core.PodTemplateSpec, - containerName string) (bool, int) { + containerName string) int { for id := range pod.Spec.Containers { - if pod.Spec.Containers[id].Name != spec.Spec.Containers[id].Name || - pod.Spec.Containers[id].Name != status.Spec.Containers[id].Name || - pod.Spec.Containers[id].Name != containerName { + if pod.Spec.Containers[id].Name == spec.Spec.Containers[id].Name || + pod.Spec.Containers[id].Name == status.Spec.Containers[id].Name || + pod.Spec.Containers[id].Name == containerName { - return true, id + return id } } - return false, 0 + return -1 } func validateMemberAndPod(member *api.ArangoMember, pod *core.Pod) (*core.PodTemplateSpec, *core.PodTemplateSpec, error) { diff --git a/pkg/deployment/reconcile/plan_executor.go b/pkg/deployment/reconcile/plan_executor.go index 7a0c6240b..ddb61afdb 100644 --- a/pkg/deployment/reconcile/plan_executor.go +++ b/pkg/deployment/reconcile/plan_executor.go @@ -206,8 +206,7 @@ func (d *Reconciler) executeAction(ctx context.Context, log zerolog.Logger, plan // Not started yet ready, err := action.Start(ctx) if err != nil { - log.Debug().Err(err). - Msg("Failed to start action") + log.Error().Err(err).Msg("Failed to start action") return false, false, false, errors.WithStack(err) } diff --git a/pkg/deployment/rotation/arangod_containers.go b/pkg/deployment/rotation/arangod_containers.go index 1b891e328..aea681b56 100644 --- a/pkg/deployment/rotation/arangod_containers.go +++ b/pkg/deployment/rotation/arangod_containers.go @@ -26,8 +26,6 @@ package rotation import ( "strings" - "github.com/arangodb/kube-arangodb/pkg/util" - api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1" "github.com/arangodb/kube-arangodb/pkg/util" "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" @@ -39,7 +37,7 @@ const ( ContainerImage = "image" ) -func containersCompare(deploymentSpec api.DeploymentSpec, group api.ServerGroup, spec, status *core.PodSpec) compareFunc { +func containersCompare(_ api.DeploymentSpec, _ api.ServerGroup, spec, status *core.PodSpec) compareFunc { return func(builder api.ActionBuilder) (mode Mode, plan api.Plan, err error) { a, b := spec.Containers, status.Containers From d8b53828be710b39ee7b4a09529be3aa7895d49d Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Fri, 17 Sep 2021 14:58:06 +0200 Subject: [PATCH 03/14] use commands instead of args --- .../action_runtime_container_args_udpate.go | 11 ++---- pkg/deployment/rotation/arangod_containers.go | 13 ++++--- .../rotation/arangod_containers_test.go | 34 ++++++++++++++++--- pkg/deployment/rotation/compare.go | 24 ++++++------- pkg/deployment/rotation/utils_test.go | 6 ++++ pkg/util/k8sutil/pair.go | 4 +-- 6 files changed, 60 insertions(+), 32 deletions(-) diff --git a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go index f4e9d8aa0..0ad91b92a 100644 --- a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go +++ b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go @@ -99,8 +99,7 @@ func (a actionRuntimeContainerArgsUpdate) Post(ctx context.Context) error { return false } - // TODO what if additional log level was added in the meantime - s.Template.PodSpec.Spec.Containers[id].Args = obj.Spec.Template.PodSpec.Spec.Containers[id].Args + s.Template.PodSpec.Spec.Containers[id].Command = obj.Spec.Template.PodSpec.Spec.Containers[id].Command return true } } @@ -155,10 +154,9 @@ func (a actionRuntimeContainerArgsUpdate) Start(ctx context.Context) (bool, erro } var op cmpContainer = func(containerSpec core.Container, containerStatus core.Container) error { - //diff := util.GetDifference(containerSpec.Args, containerStatus.Args) topicsLogLevel := map[string]string{} - for _, arg := range containerSpec.Args { + for _, arg := range containerSpec.Command { if !strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { continue } @@ -176,14 +174,11 @@ func (a actionRuntimeContainerArgsUpdate) Start(ctx context.Context) (bool, erro } } - // TODO - // TODO how to disable old log topics? - // TODO how to set the general log level - if err := a.setLogLevel(ctx, topicsLogLevel); err != nil { return errors.WithMessage(err, "can not set log level") } + a.log.Info().Interface("topics", topicsLogLevel).Msg("send log level to the ArangoDB") return nil } diff --git a/pkg/deployment/rotation/arangod_containers.go b/pkg/deployment/rotation/arangod_containers.go index aea681b56..ded68f755 100644 --- a/pkg/deployment/rotation/arangod_containers.go +++ b/pkg/deployment/rotation/arangod_containers.go @@ -47,21 +47,24 @@ func containersCompare(_ api.DeploymentSpec, _ api.ServerGroup, spec, status *co for id := range a { if ac, bc := &a[id], &b[id]; ac.Name == k8sutil.ServerContainerName && ac.Name == bc.Name { - foundLogLevelDifference := false - for _, arg := range util.GetDifference(ac.Args, bc.Args) { + onlyLogLevelArgsChanged := false + for _, arg := range util.GetDifference(ac.Command, bc.Command) { if strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { - foundLogLevelDifference = true + onlyLogLevelArgsChanged = true + } else { + onlyLogLevelArgsChanged = false + break } } - if !foundLogLevelDifference { + if !onlyLogLevelArgsChanged { continue } plan = append(plan, builder.NewAction(api.ActionTypeRuntimeContainerArgsLogLevelUpdate). AddParam(ContainerName, ac.Name)) - bc.Args = ac.Args + bc.Command = ac.Command mode = mode.And(InPlaceRotation) } else if ac.Name == bc.Name { if ac.Image != bc.Image { diff --git a/pkg/deployment/rotation/arangod_containers_test.go b/pkg/deployment/rotation/arangod_containers_test.go index e83bacdf7..09d12987d 100644 --- a/pkg/deployment/rotation/arangod_containers_test.go +++ b/pkg/deployment/rotation/arangod_containers_test.go @@ -23,11 +23,10 @@ package rotation import ( "testing" - "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" - v1 "k8s.io/api/core/v1" api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1" + "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" ) func Test_ArangoDContainers_SidecarImages(t *testing.T) { @@ -52,6 +51,35 @@ func Test_ArangoDContainers_SidecarImages(t *testing.T) { api.NewAction(api.ActionTypeRuntimeContainerImageUpdate, 0, ""), }, }, + { + name: "Only log level arguments of the ArangoDB server have been changed", + spec: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, + []string{"--log.level=INFO", "--log.level=requests=error"})), + status: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), + expectedMode: InPlaceRotation, + expectedPlan: api.Plan{ + api.NewAction(api.ActionTypeRuntimeContainerArgsLogLevelUpdate, 0, ""), + }, + }, + { + name: "Only log level arguments of the Sidecar have been changed", + spec: buildPodSpec(addContainerWithArgs("sidecar", + []string{"--log.level=INFO", "--log.level=requests=error"})), + status: buildPodSpec(addContainerWithArgs("sidecar", []string{"--log.level=INFO"})), + expectedMode: GracefulRotation, + }, + { + name: "ArangoDB server arguments have not been changed", + spec: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), + status: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), + }, + { + name: "Not only log level arguments of the ArangoDB server have been changed", + spec: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, []string{"--log.level=INFO", + "--server.endpoint=localhost"})), + status: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), + expectedMode: GracefulRotation, + }, } runTestCases(t)(testCases...) @@ -144,5 +172,3 @@ func Test_InitContainers(t *testing.T) { runTestCases(t)(testCases...) }) } - -// TODO unit tests for changed args diff --git a/pkg/deployment/rotation/compare.go b/pkg/deployment/rotation/compare.go index 71c01db8f..0c1632885 100644 --- a/pkg/deployment/rotation/compare.go +++ b/pkg/deployment/rotation/compare.go @@ -21,8 +21,6 @@ package rotation import ( - "encoding/json" - api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1" "github.com/arangodb/kube-arangodb/pkg/deployment/resources" "github.com/rs/zerolog" @@ -51,7 +49,9 @@ func compareFuncs(builder api.ActionBuilder, f ...compareFunc) (mode Mode, plan return } -func compare(log zerolog.Logger, deploymentSpec api.DeploymentSpec, member api.MemberStatus, group api.ServerGroup, spec, status *api.ArangoMemberPodTemplate) (mode Mode, plan api.Plan, err error) { +func compare(log zerolog.Logger, deploymentSpec api.DeploymentSpec, member api.MemberStatus, group api.ServerGroup, + spec, status *api.ArangoMemberPodTemplate) (mode Mode, plan api.Plan, err error) { + if spec.Checksum == status.Checksum { return SkippedRotation, nil, nil } @@ -79,21 +79,19 @@ func compare(log zerolog.Logger, deploymentSpec api.DeploymentSpec, member api.M return SkippedRotation, nil, err } - newSpec, err := api.GetArangoMemberPodTemplate(podStatus, checksum) + newStatus, err := api.GetArangoMemberPodTemplate(podStatus, checksum) if err != nil { log.Err(err).Msg("Error while getting template") return SkippedRotation, nil, err } - if spec.RotationNeeded(newSpec) { - l := log.Info().Str("id", member.ID).Str("Before", spec.PodSpecChecksum) - if d, err := json.Marshal(status); err == nil { - l = l.Str("status", string(d)) - } - if d, err := json.Marshal(newSpec); err == nil { - l = l.Str("spec", string(d)) - } - l.Msgf("Pod needs rotation - templates does not match") + if spec.RotationNeeded(newStatus) { + log.Info().Str("before", spec.PodSpecChecksum). + Str("id", member.ID). + Interface("spec", spec). + Interface("status", newStatus). + Msg("Pod needs rotation - templates does not match") + return GracefulRotation, nil, nil } diff --git a/pkg/deployment/rotation/utils_test.go b/pkg/deployment/rotation/utils_test.go index 14c82d8f3..0556bac24 100644 --- a/pkg/deployment/rotation/utils_test.go +++ b/pkg/deployment/rotation/utils_test.go @@ -127,6 +127,12 @@ func addSidecarWithImage(name, image string) podSpecBuilder { }) } +func addContainerWithArgs(name string, args []string) podSpecBuilder { + return addContainer(name, func(c *core.Container) { + c.Args = args + }) +} + type deploymentBuilder func(depl *api.DeploymentSpec) func buildDeployment(b ...deploymentBuilder) api.DeploymentSpec { diff --git a/pkg/util/k8sutil/pair.go b/pkg/util/k8sutil/pair.go index 39abf3489..79cfc4d47 100644 --- a/pkg/util/k8sutil/pair.go +++ b/pkg/util/k8sutil/pair.go @@ -163,7 +163,7 @@ func NewOptionPair(pairs ...OptionPair) OptionPairs { return pairs } -// TODO test desc +// TODO test desc + UT func ExtractStringToOptionPair(arg string) OptionPair { trimmed := strings.Trim(arg, " ") index := strings.Index(trimmed, "=") @@ -173,6 +173,6 @@ func ExtractStringToOptionPair(arg string) OptionPair { return OptionPair{ Key: trimmed[0:index], - Value: trimmed[index:], + Value: trimmed[index+1:], } } From 12e657fb7ea0ac28d1ffda9752145b33437119d1 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Fri, 17 Sep 2021 15:43:40 +0200 Subject: [PATCH 04/14] add unit tests --- pkg/util/k8sutil/pair.go | 8 ++-- pkg/util/k8sutil/pair_test.go | 78 +++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 pkg/util/k8sutil/pair_test.go diff --git a/pkg/util/k8sutil/pair.go b/pkg/util/k8sutil/pair.go index 79cfc4d47..12938d2e4 100644 --- a/pkg/util/k8sutil/pair.go +++ b/pkg/util/k8sutil/pair.go @@ -163,16 +163,16 @@ func NewOptionPair(pairs ...OptionPair) OptionPairs { return pairs } -// TODO test desc + UT +// ExtractStringToOptionPair extracts command line argument into the OptionPair. func ExtractStringToOptionPair(arg string) OptionPair { - trimmed := strings.Trim(arg, " ") + trimmed := strings.TrimLeft(arg, " ") index := strings.Index(trimmed, "=") if index < 0 { - return OptionPair{Key: trimmed} + return OptionPair{Key: strings.TrimRight(trimmed, " ")} } return OptionPair{ - Key: trimmed[0:index], + Key: strings.Trim(trimmed[0:index], " "), Value: trimmed[index+1:], } } diff --git a/pkg/util/k8sutil/pair_test.go b/pkg/util/k8sutil/pair_test.go new file mode 100644 index 000000000..94dec5017 --- /dev/null +++ b/pkg/util/k8sutil/pair_test.go @@ -0,0 +1,78 @@ +// +// DISCLAIMER +// +// Copyright 2021 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 Tomasz Mielech +// + +package k8sutil + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestExtractStringToOptionPair(t *testing.T) { + tests := map[string]struct { + arg string + want OptionPair + }{ + "empty argument": {}, + "not trimmed key argument on the left side": { + arg: " --log.level=requests=debug", + want: OptionPair{ + Key: "--log.level", + Value: "requests=debug", + }, + }, + "key argument not trimmed on the both sides": { + arg: " --log.level =requests=debug", + want: OptionPair{ + Key: "--log.level", + Value: "requests=debug", + }, + }, + "key argument not trimmed on the both sides without value": { + arg: " --log.level ", + want: OptionPair{ + Key: "--log.level", + }, + }, + "key argument not trimmed on the both sides without value with equal": { + arg: " --log.level =", + want: OptionPair{ + Key: "--log.level", + }, + }, + "key argument not trimmed on the right side with some value": { + arg: "--log.level = value ", + want: OptionPair{ + Key: "--log.level", + Value: " value ", + }, + }, + } + + for testName, testCase := range tests { + t.Run(testName, func(t *testing.T) { + got := ExtractStringToOptionPair(testCase.arg) + assert.Equal(t, testCase.want, got) + }) + } +} From acedcb2b06c84f43103fe04c87c78728f253566d Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Sat, 18 Sep 2021 22:00:34 +0200 Subject: [PATCH 05/14] turn off removed log levels topics --- .../action_runtime_container_args_udpate.go | 45 +++++++++++++------ 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go index 0ad91b92a..68fb9072f 100644 --- a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go +++ b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go @@ -33,6 +33,7 @@ import ( api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1" "github.com/arangodb/kube-arangodb/pkg/deployment/rotation" + "github.com/arangodb/kube-arangodb/pkg/util" "github.com/arangodb/kube-arangodb/pkg/util/arangod" "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" ) @@ -156,21 +157,18 @@ func (a actionRuntimeContainerArgsUpdate) Start(ctx context.Context) (bool, erro var op cmpContainer = func(containerSpec core.Container, containerStatus core.Container) error { topicsLogLevel := map[string]string{} - for _, arg := range containerSpec.Command { - if !strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { - continue + // Set log levels to INFO for topics which were removed from the spec. + statusDiff := util.GetDifference(containerStatus.Command, containerSpec.Command) + for _, arg := range statusDiff { + if ok, topic, _ := getTopicAndLevel(arg); ok { + topicsLogLevel[topic] = "INFO" } + } - logLevelOption := k8sutil.ExtractStringToOptionPair(arg) - if len(logLevelOption.Value) > 0 { - logValueOption := k8sutil.ExtractStringToOptionPair(logLevelOption.Value) - if len(logValueOption.Value) > 0 { - // It is the topic log, e.g.: --log.level=request=INFO. - topicsLogLevel[logValueOption.Key] = logValueOption.Value - } else { - // It is the general log, e.g.: --log.level=INFO. - topicsLogLevel["general"] = logLevelOption.Value - } + // Set log levels from the provided spec. + for _, arg := range containerSpec.Command { + if ok, topic, value := getTopicAndLevel(arg); ok { + topicsLogLevel[topic] = value } } @@ -279,3 +277,24 @@ func (a actionRuntimeContainerArgsUpdate) setLogLevel(ctx context.Context, logLe return resp.CheckStatus(200) } + +// getTopicAndLevel returns topics and log level from the argument. +func getTopicAndLevel(arg string) (bool, string, string) { + if !strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { + return false, "", "" + } + + logLevelOption := k8sutil.ExtractStringToOptionPair(arg) + if len(logLevelOption.Value) > 0 { + logValueOption := k8sutil.ExtractStringToOptionPair(logLevelOption.Value) + if len(logValueOption.Value) > 0 { + // It is the topic log, e.g.: --log.level=request=INFO. + return true, logValueOption.Key, logValueOption.Value + } else { + // It is the general log, e.g.: --log.level=INFO. + return true, "general", logLevelOption.Value + } + } + + return false, "", "" +} From 10d5706b454e7707b881e0ec1852a88c63f76a68 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Sun, 19 Sep 2021 13:10:47 +0200 Subject: [PATCH 06/14] compare status args to the spec args --- pkg/deployment/rotation/arangod_containers.go | 36 +++++++++++++------ pkg/util/strings.go | 6 ++-- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/pkg/deployment/rotation/arangod_containers.go b/pkg/deployment/rotation/arangod_containers.go index ded68f755..7c48715fd 100644 --- a/pkg/deployment/rotation/arangod_containers.go +++ b/pkg/deployment/rotation/arangod_containers.go @@ -47,17 +47,7 @@ func containersCompare(_ api.DeploymentSpec, _ api.ServerGroup, spec, status *co for id := range a { if ac, bc := &a[id], &b[id]; ac.Name == k8sutil.ServerContainerName && ac.Name == bc.Name { - onlyLogLevelArgsChanged := false - for _, arg := range util.GetDifference(ac.Command, bc.Command) { - if strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { - onlyLogLevelArgsChanged = true - } else { - onlyLogLevelArgsChanged = false - break - } - } - - if !onlyLogLevelArgsChanged { + if !IsOnlyLogLevelChanged(ac.Command, bc.Command) { continue } @@ -123,3 +113,27 @@ func initContainersCompare(deploymentSpec api.DeploymentSpec, group api.ServerGr return } } + +// IsOnlyLogLevelChanged returns true when status and spec log level arguments are different. +// If any other argument than --log.level is different false is returned. +func IsOnlyLogLevelChanged(specArgs, statusArgs []string) bool { + diffSpec := util.GetDifference(specArgs, statusArgs) + for _, arg := range diffSpec { + if !strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { + return false + } + } + + diffStatus := util.GetDifference(statusArgs, specArgs) + for _, arg := range diffStatus { + if !strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { + return false + } + } + + if len(diffSpec) == 0 && len(diffStatus) == 0 { + return false + } + + return true +} diff --git a/pkg/util/strings.go b/pkg/util/strings.go index 52bc76126..310746f6d 100644 --- a/pkg/util/strings.go +++ b/pkg/util/strings.go @@ -66,14 +66,14 @@ func PrefixStringArray(a []string, prefix string) []string { // GetDifference returns the elements in `compareWhat` that are not in `compareTo`. func GetDifference(compareWhat, compareTo []string) []string { - mb := make(map[string]struct{}, len(compareTo)) + compareToMap := make(map[string]struct{}, len(compareTo)) for _, x := range compareTo { - mb[x] = struct{}{} + compareToMap[x] = struct{}{} } var diff []string for _, x := range compareWhat { - if _, found := mb[x]; !found { + if _, found := compareToMap[x]; !found { diff = append(diff, x) } } From e5ea22f712b878c4f444a0facdc96b421039020f Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Sun, 19 Sep 2021 21:15:52 +0200 Subject: [PATCH 07/14] update checksum for the status --- .../action_runtime_container_args_udpate.go | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go index 68fb9072f..e8868f1f6 100644 --- a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go +++ b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go @@ -32,6 +32,7 @@ import ( core "k8s.io/api/core/v1" api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1" + "github.com/arangodb/kube-arangodb/pkg/deployment/resources" "github.com/arangodb/kube-arangodb/pkg/deployment/rotation" "github.com/arangodb/kube-arangodb/pkg/util" "github.com/arangodb/kube-arangodb/pkg/util/arangod" @@ -61,8 +62,6 @@ type actionRuntimeContainerArgsUpdate struct { // Post updates arguments for the specific Arango member. func (a actionRuntimeContainerArgsUpdate) Post(ctx context.Context) error { - a.log.Info().Msgf("Updating container args") - m, ok := a.actionCtx.GetMemberStatusByID(a.action.MemberID) if !ok { a.log.Info().Msg("member is gone already") @@ -81,31 +80,47 @@ func (a actionRuntimeContainerArgsUpdate) Post(ctx context.Context) error { return nil } + log := a.log.With().Str("containerName", containerName).Logger() updateMemberStatusArgs := func(obj *api.ArangoMember, s *api.ArangoMemberStatus) bool { if obj.Spec.Template == nil || s.Template == nil || obj.Spec.Template.PodSpec == nil || s.Template.PodSpec == nil { - a.log.Info().Msgf("Nil Member definition") + log.Info().Msgf("Nil Member definition") return false } if len(obj.Spec.Template.PodSpec.Spec.Containers) != len(s.Template.PodSpec.Spec.Containers) { - a.log.Info().Msgf("Invalid size of containers") + log.Info().Msgf("Invalid size of containers") return false } for id := range obj.Spec.Template.PodSpec.Spec.Containers { if obj.Spec.Template.PodSpec.Spec.Containers[id].Name == containerName { if s.Template.PodSpec.Spec.Containers[id].Name != containerName { - a.log.Info().Msgf("Invalid order of containers") + log.Info().Msgf("Invalid order of containers") return false } s.Template.PodSpec.Spec.Containers[id].Command = obj.Spec.Template.PodSpec.Spec.Containers[id].Command + groupSpec := a.actionCtx.GetSpec().GetServerGroupSpec(a.action.Group) + checksum, err := resources.ChecksumArangoPod(groupSpec, + resources.CreatePodFromTemplate(obj.Spec.Template.PodSpec)) + if err != nil { + log.Error().Err(err).Msg("Error while getting pod checksum") + return false + } + + newStatus, err := api.GetArangoMemberPodTemplate(s.Template.PodSpec, checksum) + if err != nil { + log.Error().Err(err).Msg("Error while getting template") + return false + } + s.Template = newStatus + log.Info().Msgf("Updating container args") return true } } - a.log.Info().Msgf("can not find the container") + log.Info().Msgf("can not find the container") return false } From a9a006e12dd632813c83eaa205b1c51661fa7447 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Mon, 20 Sep 2021 15:48:12 +0200 Subject: [PATCH 08/14] use default silent rotation --- .../action_runtime_container_args_udpate.go | 31 +++++-------------- pkg/deployment/rotation/arangod_containers.go | 4 +-- pkg/deployment/rotation/compare.go | 4 ++- pkg/util/strings.go | 4 +-- 4 files changed, 15 insertions(+), 28 deletions(-) diff --git a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go index e8868f1f6..3ea6eb034 100644 --- a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go +++ b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go @@ -32,9 +32,7 @@ import ( core "k8s.io/api/core/v1" api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1" - "github.com/arangodb/kube-arangodb/pkg/deployment/resources" "github.com/arangodb/kube-arangodb/pkg/deployment/rotation" - "github.com/arangodb/kube-arangodb/pkg/util" "github.com/arangodb/kube-arangodb/pkg/util/arangod" "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" ) @@ -101,20 +99,6 @@ func (a actionRuntimeContainerArgsUpdate) Post(ctx context.Context) error { } s.Template.PodSpec.Spec.Containers[id].Command = obj.Spec.Template.PodSpec.Spec.Containers[id].Command - groupSpec := a.actionCtx.GetSpec().GetServerGroupSpec(a.action.Group) - checksum, err := resources.ChecksumArangoPod(groupSpec, - resources.CreatePodFromTemplate(obj.Spec.Template.PodSpec)) - if err != nil { - log.Error().Err(err).Msg("Error while getting pod checksum") - return false - } - - newStatus, err := api.GetArangoMemberPodTemplate(s.Template.PodSpec, checksum) - if err != nil { - log.Error().Err(err).Msg("Error while getting template") - return false - } - s.Template = newStatus log.Info().Msgf("Updating container args") return true } @@ -172,13 +156,14 @@ func (a actionRuntimeContainerArgsUpdate) Start(ctx context.Context) (bool, erro var op cmpContainer = func(containerSpec core.Container, containerStatus core.Container) error { topicsLogLevel := map[string]string{} - // Set log levels to INFO for topics which were removed from the spec. - statusDiff := util.GetDifference(containerStatus.Command, containerSpec.Command) - for _, arg := range statusDiff { - if ok, topic, _ := getTopicAndLevel(arg); ok { - topicsLogLevel[topic] = "INFO" - } - } + // The below code can be used to set default log level when it is removed from the spec. + // The list of default log levels exists, but it is hardcoded in ArangoDB, and it is not possible to fetch it. + //statusDiff := util.Diff(containerStatus.Command, containerSpec.Command) + //for _, arg := range statusDiff { + // if ok, topic, _ := getTopicAndLevel(arg); ok { + // topicsLogLevel[topic] = "INFO" // Each topic has its own default log level. It is INFO for most of them. + // } + //} // Set log levels from the provided spec. for _, arg := range containerSpec.Command { diff --git a/pkg/deployment/rotation/arangod_containers.go b/pkg/deployment/rotation/arangod_containers.go index 7c48715fd..d42c854c2 100644 --- a/pkg/deployment/rotation/arangod_containers.go +++ b/pkg/deployment/rotation/arangod_containers.go @@ -117,14 +117,14 @@ func initContainersCompare(deploymentSpec api.DeploymentSpec, group api.ServerGr // IsOnlyLogLevelChanged returns true when status and spec log level arguments are different. // If any other argument than --log.level is different false is returned. func IsOnlyLogLevelChanged(specArgs, statusArgs []string) bool { - diffSpec := util.GetDifference(specArgs, statusArgs) + diffSpec := util.Diff(specArgs, statusArgs) for _, arg := range diffSpec { if !strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { return false } } - diffStatus := util.GetDifference(statusArgs, specArgs) + diffStatus := util.Diff(statusArgs, specArgs) for _, arg := range diffStatus { if !strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { return false diff --git a/pkg/deployment/rotation/compare.go b/pkg/deployment/rotation/compare.go index 0c1632885..036409164 100644 --- a/pkg/deployment/rotation/compare.go +++ b/pkg/deployment/rotation/compare.go @@ -56,7 +56,9 @@ func compare(log zerolog.Logger, deploymentSpec api.DeploymentSpec, member api.M return SkippedRotation, nil, nil } - mode = SkippedRotation + // If checksums are different and rotation is not needed and there are no changes between containers + // then silent rotation must be applied to adjust status checksum. + mode = SilentRotation podStatus := status.PodSpec.DeepCopy() diff --git a/pkg/util/strings.go b/pkg/util/strings.go index 310746f6d..7c4d36cbd 100644 --- a/pkg/util/strings.go +++ b/pkg/util/strings.go @@ -64,8 +64,8 @@ func PrefixStringArray(a []string, prefix string) []string { return b } -// GetDifference returns the elements in `compareWhat` that are not in `compareTo`. -func GetDifference(compareWhat, compareTo []string) []string { +// Diff returns the elements in `compareWhat` that are not in `compareTo`. +func Diff(compareWhat, compareTo []string) []string { compareToMap := make(map[string]struct{}, len(compareTo)) for _, x := range compareTo { compareToMap[x] = struct{}{} From c2a31fd445a19a8afe7e202db3457b8406dcbcd5 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Mon, 20 Sep 2021 16:36:25 +0200 Subject: [PATCH 09/14] fix unit tests --- .../rotation/arangod_containers_test.go | 65 ++++++++++--------- pkg/deployment/rotation/utils_test.go | 4 +- pkg/util/strings_test.go | 65 +++++++++++++++++++ 3 files changed, 103 insertions(+), 31 deletions(-) create mode 100644 pkg/util/strings_test.go diff --git a/pkg/deployment/rotation/arangod_containers_test.go b/pkg/deployment/rotation/arangod_containers_test.go index 09d12987d..11eadf4bb 100644 --- a/pkg/deployment/rotation/arangod_containers_test.go +++ b/pkg/deployment/rotation/arangod_containers_test.go @@ -51,35 +51,6 @@ func Test_ArangoDContainers_SidecarImages(t *testing.T) { api.NewAction(api.ActionTypeRuntimeContainerImageUpdate, 0, ""), }, }, - { - name: "Only log level arguments of the ArangoDB server have been changed", - spec: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, - []string{"--log.level=INFO", "--log.level=requests=error"})), - status: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), - expectedMode: InPlaceRotation, - expectedPlan: api.Plan{ - api.NewAction(api.ActionTypeRuntimeContainerArgsLogLevelUpdate, 0, ""), - }, - }, - { - name: "Only log level arguments of the Sidecar have been changed", - spec: buildPodSpec(addContainerWithArgs("sidecar", - []string{"--log.level=INFO", "--log.level=requests=error"})), - status: buildPodSpec(addContainerWithArgs("sidecar", []string{"--log.level=INFO"})), - expectedMode: GracefulRotation, - }, - { - name: "ArangoDB server arguments have not been changed", - spec: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), - status: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), - }, - { - name: "Not only log level arguments of the ArangoDB server have been changed", - spec: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, []string{"--log.level=INFO", - "--server.endpoint=localhost"})), - status: buildPodSpec(addContainerWithArgs(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), - expectedMode: GracefulRotation, - }, } runTestCases(t)(testCases...) @@ -172,3 +143,39 @@ func Test_InitContainers(t *testing.T) { runTestCases(t)(testCases...) }) } + +func Test_Container_Args(t *testing.T) { + testCases := []TestCase{ + { + name: "Only log level arguments of the ArangoDB server have been changed", + spec: buildPodSpec(addContainerWithCommand(k8sutil.ServerContainerName, + []string{"--log.level=INFO", "--log.level=requests=error"})), + status: buildPodSpec(addContainerWithCommand(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), + expectedMode: InPlaceRotation, + expectedPlan: api.Plan{ + api.NewAction(api.ActionTypeRuntimeContainerArgsLogLevelUpdate, 0, ""), + }, + }, + { + name: "Only log level arguments of the Sidecar have been changed", + spec: buildPodSpec(addContainerWithCommand("sidecar", + []string{"--log.level=INFO", "--log.level=requests=error"})), + status: buildPodSpec(addContainerWithCommand("sidecar", []string{"--log.level=INFO"})), + expectedMode: GracefulRotation, + }, + { + name: "ArangoDB server arguments have not been changed", + spec: buildPodSpec(addContainerWithCommand(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), + status: buildPodSpec(addContainerWithCommand(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), + }, + { + name: "Not only log level arguments of the ArangoDB server have been changed", + spec: buildPodSpec(addContainerWithCommand(k8sutil.ServerContainerName, []string{"--log.level=INFO", + "--server.endpoint=localhost"})), + status: buildPodSpec(addContainerWithCommand(k8sutil.ServerContainerName, []string{"--log.level=INFO"})), + expectedMode: GracefulRotation, + }, + } + + runTestCases(t)(testCases...) +} diff --git a/pkg/deployment/rotation/utils_test.go b/pkg/deployment/rotation/utils_test.go index 0556bac24..61886aa9f 100644 --- a/pkg/deployment/rotation/utils_test.go +++ b/pkg/deployment/rotation/utils_test.go @@ -127,9 +127,9 @@ func addSidecarWithImage(name, image string) podSpecBuilder { }) } -func addContainerWithArgs(name string, args []string) podSpecBuilder { +func addContainerWithCommand(name string, command []string) podSpecBuilder { return addContainer(name, func(c *core.Container) { - c.Args = args + c.Command = command }) } diff --git a/pkg/util/strings_test.go b/pkg/util/strings_test.go new file mode 100644 index 000000000..670ad2c8e --- /dev/null +++ b/pkg/util/strings_test.go @@ -0,0 +1,65 @@ +package util + +import ( + "github.com/stretchr/testify/assert" + "testing" +) + +func TestDiff(t *testing.T) { + type args struct { + compareWhat []string + compareTo []string + } + tests := map[string]struct { + args args + want []string + }{ + "two nil slices": {}, + "source slice is nil": { + args: args{ + compareWhat: nil, + compareTo: []string{"1"}, + }, + want: nil, + }, + "destination slice is nil": { + args: args{ + compareWhat: []string{"1"}, + }, + want: []string{"1"}, + }, + "source slice has more elements": { + args: args{ + compareWhat: []string{"1", "2"}, + compareTo: []string{"2"}, + }, + want: []string{"1"}, + }, + "destination slice has more elements": { + args: args{ + compareWhat: []string{"1", "2"}, + compareTo: []string{"1", "2", "3"}, + }, + }, + "destination and source slices have overlapping elements": { + args: args{ + compareWhat: []string{"1", "2", "3", "4", "5", "6", "7", "8", "9", "10"}, + compareTo: []string{"1", "3", "5", "7", "9"}, + }, + want: []string{"2", "4", "6", "8", "10"}, + }, + "destination slice contains source slice": { + args: args{ + compareWhat: []string{"1", "3", "5", "7", "9"}, + compareTo: []string{"1", "2", "3", "4", "5", "6", "7", "8", "9", "10"}, + }, + }, + } + + for testName, testCase := range tests { + t.Run(testName, func(t *testing.T) { + diff := Diff(testCase.args.compareWhat, testCase.args.compareTo) + assert.Equal(t, testCase.want, diff) + }) + } +} From 5d4bca4ea2e8a286203d653c6d7807e86fbc7f37 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Mon, 20 Sep 2021 19:29:59 +0200 Subject: [PATCH 10/14] add license --- pkg/util/strings_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/pkg/util/strings_test.go b/pkg/util/strings_test.go index 670ad2c8e..106a1d29d 100644 --- a/pkg/util/strings_test.go +++ b/pkg/util/strings_test.go @@ -1,3 +1,25 @@ +// +// DISCLAIMER +// +// Copyright 2021 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 Tomasz Mielech +// + package util import ( From 5d0a3ce19244cd4d3274a010a406c4e819792363 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Tue, 21 Sep 2021 08:39:17 +0200 Subject: [PATCH 11/14] adjust find in Makefile to POSIX --- Makefile | 6 +++--- pkg/util/strings_test.go | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 6ef914743..22c44b913 100644 --- a/Makefile +++ b/Makefile @@ -122,10 +122,10 @@ ifdef VERBOSE TESTVERBOSEOPTIONS := -v endif -EXCLUDE_DIRS := tests vendor .gobuild deps tools -SOURCES_QUERY := find $(SRCDIR) -name '*.go' -type f -not -path '$(SRCDIR)/tests/*' -not -path '$(SRCDIR)/vendor/*' -not -path '$(SRCDIR)/.gobuild/*' -not -path '$(SRCDIR)/deps/*' -not -path '$(SRCDIR)/tools/*' +EXCLUDE_DIRS := tests vendor .gobuild deps +SOURCES_QUERY := find ./ -type f -name '*.go' $(foreach EXCLUDE_DIR,$(EXCLUDE_DIRS), ! -path "./$(EXCLUDE_DIR)/*") ! -path './tools/*' SOURCES := $(shell $(SOURCES_QUERY)) -DASHBOARDSOURCES := $(shell find $(DASHBOARDDIR)/src -name '*.js' -not -path './test/*') $(DASHBOARDDIR)/package.json +DASHBOARDSOURCES := $(shell find $(DASHBOARDDIR)/src -name '*.js') $(DASHBOARDDIR)/package.json .DEFAULT_GOAL := all .PHONY: all diff --git a/pkg/util/strings_test.go b/pkg/util/strings_test.go index 106a1d29d..6a2a5636f 100644 --- a/pkg/util/strings_test.go +++ b/pkg/util/strings_test.go @@ -23,8 +23,9 @@ package util import ( - "github.com/stretchr/testify/assert" "testing" + + "github.com/stretchr/testify/assert" ) func TestDiff(t *testing.T) { From c2ee36f8802b1aa0002996b28757c07cba886df7 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Tue, 21 Sep 2021 09:01:19 +0200 Subject: [PATCH 12/14] adjust to v2 version --- pkg/apis/deployment/v2alpha1/plan.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/apis/deployment/v2alpha1/plan.go b/pkg/apis/deployment/v2alpha1/plan.go index b3aa59601..79affd7d1 100644 --- a/pkg/apis/deployment/v2alpha1/plan.go +++ b/pkg/apis/deployment/v2alpha1/plan.go @@ -171,6 +171,8 @@ const ( // Runtime Updates // ActionTypeRuntimeContainerImageUpdate updates container image in runtime ActionTypeRuntimeContainerImageUpdate ActionType = "RuntimeContainerImageUpdate" + // ActionTypeRuntimeContainerArgsLogLevelUpdate updates the container's executor arguments. + ActionTypeRuntimeContainerArgsLogLevelUpdate ActionType = "RuntimeContainerArgsLogLevelUpdate" ) const ( From 286b2a2f9a0ca968ae9b7e4eafcebfbc3bf8a8bb Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Tue, 21 Sep 2021 10:43:54 +0200 Subject: [PATCH 13/14] compare string slices in both ways --- Makefile | 4 +- .../action_runtime_container_args_udpate.go | 9 --- pkg/deployment/rotation/arangod_containers.go | 15 ++--- .../rotation/arangod_containers_test.go | 45 +++++++++++++- pkg/util/strings.go | 12 +++- pkg/util/strings_test.go | 62 ++++++++++++++++++- 6 files changed, 121 insertions(+), 26 deletions(-) diff --git a/Makefile b/Makefile index 22c44b913..c1801f603 100644 --- a/Makefile +++ b/Makefile @@ -122,8 +122,8 @@ ifdef VERBOSE TESTVERBOSEOPTIONS := -v endif -EXCLUDE_DIRS := tests vendor .gobuild deps -SOURCES_QUERY := find ./ -type f -name '*.go' $(foreach EXCLUDE_DIR,$(EXCLUDE_DIRS), ! -path "./$(EXCLUDE_DIR)/*") ! -path './tools/*' +EXCLUDE_DIRS := tests vendor .gobuild deps tools +SOURCES_QUERY := find ./ -type f -name '*.go' $(foreach EXCLUDE_DIR,$(EXCLUDE_DIRS), ! -path "./$(EXCLUDE_DIR)/*") SOURCES := $(shell $(SOURCES_QUERY)) DASHBOARDSOURCES := $(shell find $(DASHBOARDDIR)/src -name '*.js') $(DASHBOARDDIR)/package.json diff --git a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go index 3ea6eb034..ff3b596b0 100644 --- a/pkg/deployment/reconcile/action_runtime_container_args_udpate.go +++ b/pkg/deployment/reconcile/action_runtime_container_args_udpate.go @@ -156,15 +156,6 @@ func (a actionRuntimeContainerArgsUpdate) Start(ctx context.Context) (bool, erro var op cmpContainer = func(containerSpec core.Container, containerStatus core.Container) error { topicsLogLevel := map[string]string{} - // The below code can be used to set default log level when it is removed from the spec. - // The list of default log levels exists, but it is hardcoded in ArangoDB, and it is not possible to fetch it. - //statusDiff := util.Diff(containerStatus.Command, containerSpec.Command) - //for _, arg := range statusDiff { - // if ok, topic, _ := getTopicAndLevel(arg); ok { - // topicsLogLevel[topic] = "INFO" // Each topic has its own default log level. It is INFO for most of them. - // } - //} - // Set log levels from the provided spec. for _, arg := range containerSpec.Command { if ok, topic, value := getTopicAndLevel(arg); ok { diff --git a/pkg/deployment/rotation/arangod_containers.go b/pkg/deployment/rotation/arangod_containers.go index d42c854c2..1d453d91f 100644 --- a/pkg/deployment/rotation/arangod_containers.go +++ b/pkg/deployment/rotation/arangod_containers.go @@ -117,23 +117,16 @@ func initContainersCompare(deploymentSpec api.DeploymentSpec, group api.ServerGr // IsOnlyLogLevelChanged returns true when status and spec log level arguments are different. // If any other argument than --log.level is different false is returned. func IsOnlyLogLevelChanged(specArgs, statusArgs []string) bool { - diffSpec := util.Diff(specArgs, statusArgs) - for _, arg := range diffSpec { - if !strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { - return false - } + diff := util.DiffStrings(specArgs, statusArgs) + if len(diff) == 0 { + return false } - diffStatus := util.Diff(statusArgs, specArgs) - for _, arg := range diffStatus { + for _, arg := range diff { if !strings.HasPrefix(strings.TrimLeft(arg, " "), "--log.level") { return false } } - if len(diffSpec) == 0 && len(diffStatus) == 0 { - return false - } - return true } diff --git a/pkg/deployment/rotation/arangod_containers_test.go b/pkg/deployment/rotation/arangod_containers_test.go index 11eadf4bb..1855e9d6f 100644 --- a/pkg/deployment/rotation/arangod_containers_test.go +++ b/pkg/deployment/rotation/arangod_containers_test.go @@ -1,7 +1,7 @@ // // DISCLAIMER // -// Copyright 2020 ArangoDB GmbH, Cologne, Germany +// Copyright 2020-2021 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. @@ -17,12 +17,16 @@ // // Copyright holder is ArangoDB GmbH, Cologne, Germany // +// Author Adam Janikowski +// Author Tomasz Mielech +// package rotation import ( "testing" + "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1" @@ -179,3 +183,42 @@ func Test_Container_Args(t *testing.T) { runTestCases(t)(testCases...) } + +func TestIsOnlyLogLevelChanged(t *testing.T) { + type args struct { + specArgs []string + statusArgs []string + } + tests := map[string]struct { + args args + want bool + }{ + "log level not changed": { + args: args{ + specArgs: []string{"--log.level=INFO"}, + statusArgs: []string{"--log.level=INFO"}, + }, + }, + "log level changed": { + args: args{ + specArgs: []string{"--log.level=INFO", "--log.level=requests=DEBUG"}, + statusArgs: []string{"--log.level=INFO"}, + }, + want: true, + }, + "log level and server endpoint changed": { + args: args{ + specArgs: []string{"--log.level=INFO", "--log.level=requests=DEBUG", "--server.endpoint=localhost"}, + statusArgs: []string{"--log.level=INFO"}, + }, + }, + } + + for testName, testCase := range tests { + t.Run(testName, func(t *testing.T) { + got := IsOnlyLogLevelChanged(testCase.args.specArgs, testCase.args.statusArgs) + + assert.Equal(t, testCase.want, got) + }) + } +} diff --git a/pkg/util/strings.go b/pkg/util/strings.go index 7c4d36cbd..b7ac01451 100644 --- a/pkg/util/strings.go +++ b/pkg/util/strings.go @@ -64,8 +64,8 @@ func PrefixStringArray(a []string, prefix string) []string { return b } -// Diff returns the elements in `compareWhat` that are not in `compareTo`. -func Diff(compareWhat, compareTo []string) []string { +// DiffStringsOneWay returns the elements in `compareWhat` that are not in `compareTo`. +func DiffStringsOneWay(compareWhat, compareTo []string) []string { compareToMap := make(map[string]struct{}, len(compareTo)) for _, x := range compareTo { compareToMap[x] = struct{}{} @@ -80,3 +80,11 @@ func Diff(compareWhat, compareTo []string) []string { return diff } + +// DiffStrings returns the elements in `compareWhat` that are not in `compareTo` and +// elements in `compareTo` that are not in `compareFrom`. +func DiffStrings(compareWhat, compareTo []string) []string { + diff := DiffStringsOneWay(compareWhat, compareTo) + + return append(diff, DiffStringsOneWay(compareTo, compareWhat)...) +} diff --git a/pkg/util/strings_test.go b/pkg/util/strings_test.go index 6a2a5636f..3d94b6d2d 100644 --- a/pkg/util/strings_test.go +++ b/pkg/util/strings_test.go @@ -81,8 +81,68 @@ func TestDiff(t *testing.T) { for testName, testCase := range tests { t.Run(testName, func(t *testing.T) { - diff := Diff(testCase.args.compareWhat, testCase.args.compareTo) + diff := DiffStringsOneWay(testCase.args.compareWhat, testCase.args.compareTo) assert.Equal(t, testCase.want, diff) }) } } + +func TestDiffStrings(t *testing.T) { + type args struct { + compareWhat []string + compareTo []string + } + tests := map[string]struct { + args args + want []string + }{ + "two nil slices": {}, + "source slice is nil": { + args: args{ + compareTo: []string{"1"}, + }, + want: []string{"1"}, + }, + "destination slice is nil": { + args: args{ + compareWhat: []string{"1"}, + }, + want: []string{"1"}, + }, + "source slice has more elements": { + args: args{ + compareWhat: []string{"1", "2"}, + compareTo: []string{"2"}, + }, + want: []string{"1"}, + }, + "destination slice has more elements": { + args: args{ + compareWhat: []string{"1", "2"}, + compareTo: []string{"1", "2", "3"}, + }, + want: []string{"3"}, + }, + "destination and source slices have all different elements": { + args: args{ + compareWhat: []string{"1", "2", "3"}, + compareTo: []string{"4", "5", "6"}, + }, + want: []string{"1", "2", "3", "4", "5", "6"}, + }, + "destination and source slices have some overlapping elements": { + args: args{ + compareWhat: []string{"1", "2", "3", "4"}, + compareTo: []string{"3", "4", "5", "6"}, + }, + want: []string{"1", "2", "5", "6"}, + }, + } + for testName, testCase := range tests { + t.Run(testName, func(t *testing.T) { + got := DiffStrings(testCase.args.compareWhat, testCase.args.compareTo) + + assert.Equal(t, testCase.want, got) + }) + } +} From 07488b6d727f3acf9588616bbb26b7a02ec71576 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Tue, 21 Sep 2021 11:06:08 +0200 Subject: [PATCH 14/14] adjust Makefile --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index c1801f603..ffb98c58f 100644 --- a/Makefile +++ b/Makefile @@ -123,7 +123,7 @@ ifdef VERBOSE endif EXCLUDE_DIRS := tests vendor .gobuild deps tools -SOURCES_QUERY := find ./ -type f -name '*.go' $(foreach EXCLUDE_DIR,$(EXCLUDE_DIRS), ! -path "./$(EXCLUDE_DIR)/*") +SOURCES_QUERY := find ./ -type f -name '*.go' $(foreach EXCLUDE_DIR,$(EXCLUDE_DIRS), -not -path "./$(EXCLUDE_DIR)/*") SOURCES := $(shell $(SOURCES_QUERY)) DASHBOARDSOURCES := $(shell find $(DASHBOARDDIR)/src -name '*.js') $(DASHBOARDDIR)/package.json