Skip to content

Commit 86f5e46

Browse files
ewoutpewoutp
authored
Merge pull request #206 from arangodb/feature/rotate-on-args-change
Rotate server on changed arguments
2 parents e37cc7b + e6d1040 commit 86f5e46

File tree

7 files changed

+339
-58
lines changed

7 files changed

+339
-58
lines changed

‎pkg/deployment/context_impl.go

+6
Original file line numberDiff line numberDiff line change
@@ -366,3 +366,9 @@ func (d *Deployment) DeleteSecret(secretName string) error {
366366
}
367367
return nil
368368
}
369+
370+
// GetExpectedPodArguments creates command line arguments for a server in the given group with given ID.
371+
func (d *Deployment) GetExpectedPodArguments(apiObject metav1.Object, deplSpec api.DeploymentSpec, group api.ServerGroup,
372+
agents api.MemberStatusList, id string) []string {
373+
return d.resources.GetExpectedPodArguments(apiObject, deplSpec, group, agents, id)
374+
}

‎pkg/deployment/reconcile/context.go

+4
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
driver "github.com/arangodb/go-driver"
3030
"github.com/arangodb/go-driver/agency"
3131
"k8s.io/api/core/v1"
32+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3233

3334
api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1alpha"
3435
"github.com/arangodb/kube-arangodb/pkg/util/k8sutil"
@@ -89,4 +90,7 @@ type Context interface {
8990
// DeleteSecret removes the Secret with given name.
9091
// If the secret does not exist, the error is ignored.
9192
DeleteSecret(secretName string) error
93+
// GetExpectedPodArguments creates command line arguments for a server in the given group with given ID.
94+
GetExpectedPodArguments(apiObject metav1.Object, deplSpec api.DeploymentSpec, group api.ServerGroup,
95+
agents api.MemberStatusList, id string) []string
9296
}

‎pkg/deployment/reconcile/plan_builder.go

+28-19
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
package reconcile
2424

2525
import (
26+
"strings"
27+
2628
"github.com/rs/zerolog"
2729
"github.com/rs/zerolog/log"
2830
"k8s.io/api/core/v1"
@@ -54,7 +56,8 @@ func (d *Reconciler) CreatePlan() error {
5456
apiObject := d.context.GetAPIObject()
5557
spec := d.context.GetSpec()
5658
status, lastVersion := d.context.GetStatus()
57-
newPlan, changed := createPlan(d.log, apiObject, status.Plan, spec, status, pods, d.context.GetTLSKeyfile, d.context.GetTLSCA, d.context.GetPvc, d.context.CreateEvent)
59+
ctx := newPlanBuilderContext(d.context)
60+
newPlan, changed := createPlan(d.log, apiObject, status.Plan, spec, status, pods, ctx)
5861

5962
// If not change, we're done
6063
if !changed {
@@ -79,10 +82,7 @@ func (d *Reconciler) CreatePlan() error {
7982
func createPlan(log zerolog.Logger, apiObject k8sutil.APIObject,
8083
currentPlan api.Plan, spec api.DeploymentSpec,
8184
status api.DeploymentStatus, pods []v1.Pod,
82-
getTLSKeyfile func(group api.ServerGroup, member api.MemberStatus) (string, error),
83-
getTLSCA func(string) (string, string, bool, error),
84-
getPVC func(pvcName string) (*v1.PersistentVolumeClaim, error),
85-
createEvent func(evt *k8sutil.Event)) (api.Plan, bool) {
85+
context PlanBuilderContext) (api.Plan, bool) {
8686
if len(currentPlan) > 0 {
8787
// Plan already exists, complete that first
8888
return currentPlan, false
@@ -165,7 +165,7 @@ func createPlan(log zerolog.Logger, apiObject k8sutil.APIObject,
165165
if decision.UpgradeNeeded && decision.UpgradeAllowed {
166166
plan = append(plan, createUpgradeMemberPlan(log, m, group, "Version upgrade")...)
167167
} else {
168-
rotNeeded, reason := podNeedsRotation(*p, apiObject, spec, group, status.Members.Agents, m.ID)
168+
rotNeeded, reason := podNeedsRotation(log, *p, apiObject, spec, group, status.Members.Agents, m.ID, context)
169169
if rotNeeded {
170170
plan = append(plan, createRotateMemberPlan(log, m, group, reason)...)
171171
}
@@ -179,17 +179,17 @@ func createPlan(log zerolog.Logger, apiObject k8sutil.APIObject,
179179

180180
// Check for the need to rotate TLS certificate of a members
181181
if len(plan) == 0 {
182-
plan = createRotateTLSServerCertificatePlan(log, spec, status, getTLSKeyfile)
182+
plan = createRotateTLSServerCertificatePlan(log, spec, status, context.GetTLSKeyfile)
183183
}
184184

185185
// Check for changes storage classes or requirements
186186
if len(plan) == 0 {
187-
plan = createRotateServerStoragePlan(log, apiObject, spec, status, getPVC, createEvent)
187+
plan = createRotateServerStoragePlan(log, apiObject, spec, status, context.GetPvc, context.CreateEvent)
188188
}
189189

190190
// Check for the need to rotate TLS CA certificate and all members
191191
if len(plan) == 0 {
192-
plan = createRotateTLSCAPlan(log, apiObject, spec, status, getTLSCA, createEvent)
192+
plan = createRotateTLSCAPlan(log, apiObject, spec, status, context.GetTLSCA, context.CreateEvent)
193193
}
194194

195195
// Return plan
@@ -241,12 +241,14 @@ func podNeedsUpgrading(p v1.Pod, spec api.DeploymentSpec, images api.ImageInfoLi
241241
// given pod differs from what it should be according to the
242242
// given deployment spec.
243243
// When true is returned, a reason for the rotation is already returned.
244-
func podNeedsRotation(p v1.Pod, apiObject metav1.Object, spec api.DeploymentSpec,
245-
group api.ServerGroup, agents api.MemberStatusList, id string) (bool, string) {
244+
func podNeedsRotation(log zerolog.Logger, p v1.Pod, apiObject metav1.Object, spec api.DeploymentSpec,
245+
group api.ServerGroup, agents api.MemberStatusList, id string,
246+
context PlanBuilderContext) (bool, string) {
246247
groupSpec := spec.GetServerGroupSpec(group)
247248

248249
// Check image pull policy
249-
if c, found := k8sutil.GetContainerByName(&p, k8sutil.ServerContainerName); found {
250+
c, found := k8sutil.GetContainerByName(&p, k8sutil.ServerContainerName)
251+
if found {
250252
if c.ImagePullPolicy != spec.GetImagePullPolicy() {
251253
return true, "Image pull policy changed"
252254
}
@@ -255,15 +257,15 @@ func podNeedsRotation(p v1.Pod, apiObject metav1.Object, spec api.DeploymentSpec
255257
}
256258

257259
// Check arguments
258-
/*expectedArgs := createArangodArgs(apiObject, spec, group, agents, id)
259-
if len(expectedArgs) != len(c.Args) {
260+
expectedArgs := strings.Join(context.GetExpectedPodArguments(apiObject, spec, group, agents, id), " ")
261+
actualArgs := strings.Join(getContainerArgs(c), " ")
262+
if expectedArgs != actualArgs {
263+
log.Debug().
264+
Str("actual-args", actualArgs).
265+
Str("expected-args", expectedArgs).
266+
Msg("Arguments changed. Rotation needed.")
260267
return true, "Arguments changed"
261268
}
262-
for i, a := range expectedArgs {
263-
if c.Args[i] != a {
264-
return true, "Arguments changed"
265-
}
266-
}*/
267269

268270
// Check service account
269271
if normalizeServiceAccountName(p.Spec.ServiceAccountName) != normalizeServiceAccountName(groupSpec.GetServiceAccountName()) {
@@ -352,3 +354,10 @@ func createUpgradeMemberPlan(log zerolog.Logger, member api.MemberStatus,
352354
}
353355
return plan
354356
}
357+
358+
func getContainerArgs(c v1.Container) []string {
359+
if len(c.Command) >= 1 {
360+
return c.Command[1:]
361+
}
362+
return c.Args
363+
}

‎pkg/deployment/reconcile/plan_builder_context.go

+53
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
//
2+
// DISCLAIMER
3+
//
4+
// Copyright 2018 ArangoDB GmbH, Cologne, Germany
5+
//
6+
// Licensed under the Apache License, Version 2.0 (the "License");
7+
// you may not use this file except in compliance with the License.
8+
// You may obtain a copy of the License at
9+
//
10+
// http://www.apache.org/licenses/LICENSE-2.0
11+
//
12+
// Unless required by applicable law or agreed to in writing, software
13+
// distributed under the License is distributed on an "AS IS" BASIS,
14+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
// See the License for the specific language governing permissions and
16+
// limitations under the License.
17+
//
18+
// Copyright holder is ArangoDB GmbH, Cologne, Germany
19+
//
20+
// Author Ewout Prangsma
21+
//
22+
23+
package reconcile
24+
25+
import (
26+
api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1alpha"
27+
"github.com/arangodb/kube-arangodb/pkg/util/k8sutil"
28+
"k8s.io/api/core/v1"
29+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
30+
)
31+
32+
// PlanBuilderContext contains context methods provided to plan builders.
33+
type PlanBuilderContext interface {
34+
// GetTLSKeyfile returns the keyfile encoded TLS certificate+key for
35+
// the given member.
36+
GetTLSKeyfile(group api.ServerGroup, member api.MemberStatus) (string, error)
37+
// GetTLSCA returns the TLS CA certificate in the secret with given name.
38+
// Returns: publicKey, privateKey, ownerByDeployment, error
39+
GetTLSCA(secretName string) (string, string, bool, error)
40+
// CreateEvent creates a given event.
41+
// On error, the error is logged.
42+
CreateEvent(evt *k8sutil.Event)
43+
// GetPvc gets a PVC by the given name, in the samespace of the deployment.
44+
GetPvc(pvcName string) (*v1.PersistentVolumeClaim, error)
45+
// GetExpectedPodArguments creates command line arguments for a server in the given group with given ID.
46+
GetExpectedPodArguments(apiObject metav1.Object, deplSpec api.DeploymentSpec, group api.ServerGroup,
47+
agents api.MemberStatusList, id string) []string
48+
}
49+
50+
// newPlanBuilderContext creates a PlanBuilderContext from the given context
51+
func newPlanBuilderContext(ctx Context) PlanBuilderContext {
52+
return ctx
53+
}

‎pkg/deployment/reconcile/plan_builder_test.go

+43-39
Original file line numberDiff line numberDiff line change
@@ -37,18 +37,40 @@ import (
3737
"github.com/arangodb/kube-arangodb/pkg/util/k8sutil"
3838
)
3939

40+
type testContext struct{}
41+
42+
// GetTLSKeyfile returns the keyfile encoded TLS certificate+key for
43+
// the given member.
44+
func (c *testContext) GetTLSKeyfile(group api.ServerGroup, member api.MemberStatus) (string, error) {
45+
return "", maskAny(fmt.Errorf("Not implemented"))
46+
}
47+
48+
// GetTLSCA returns the TLS CA certificate in the secret with given name.
49+
// Returns: publicKey, privateKey, ownerByDeployment, error
50+
func (c *testContext) GetTLSCA(secretName string) (string, string, bool, error) {
51+
return "", "", false, maskAny(fmt.Errorf("Not implemented"))
52+
}
53+
54+
// CreateEvent creates a given event.
55+
// On error, the error is logged.
56+
func (c *testContext) CreateEvent(evt *k8sutil.Event) {
57+
// not implemented
58+
}
59+
60+
// GetPvc gets a PVC by the given name, in the samespace of the deployment.
61+
func (c *testContext) GetPvc(pvcName string) (*v1.PersistentVolumeClaim, error) {
62+
return nil, maskAny(fmt.Errorf("Not implemented"))
63+
}
64+
65+
// GetExpectedPodArguments creates command line arguments for a server in the given group with given ID.
66+
func (c *testContext) GetExpectedPodArguments(apiObject metav1.Object, deplSpec api.DeploymentSpec, group api.ServerGroup,
67+
agents api.MemberStatusList, id string) []string {
68+
return nil // not implemented
69+
}
70+
4071
// TestCreatePlanSingleScale creates a `single` deployment to test the creating of scaling plan.
4172
func TestCreatePlanSingleScale(t *testing.T) {
42-
getTLSKeyfile := func(group api.ServerGroup, member api.MemberStatus) (string, error) {
43-
return "", maskAny(fmt.Errorf("Not implemented"))
44-
}
45-
getTLSCA := func(string) (string, string, bool, error) {
46-
return "", "", false, maskAny(fmt.Errorf("Not implemented"))
47-
}
48-
getPVC := func(pvcName string) (*v1.PersistentVolumeClaim, error) {
49-
return nil, maskAny(fmt.Errorf("Not implemented"))
50-
}
51-
createEvent := func(evt *k8sutil.Event) {}
73+
c := &testContext{}
5274
log := zerolog.Nop()
5375
spec := api.DeploymentSpec{
5476
Mode: api.NewMode(api.DeploymentModeSingle),
@@ -64,7 +86,7 @@ func TestCreatePlanSingleScale(t *testing.T) {
6486

6587
// Test with empty status
6688
var status api.DeploymentStatus
67-
newPlan, changed := createPlan(log, depl, nil, spec, status, nil, getTLSKeyfile, getTLSCA, getPVC, createEvent)
89+
newPlan, changed := createPlan(log, depl, nil, spec, status, nil, c)
6890
assert.True(t, changed)
6991
assert.Len(t, newPlan, 0) // Single mode does not scale
7092

@@ -75,7 +97,7 @@ func TestCreatePlanSingleScale(t *testing.T) {
7597
PodName: "something",
7698
},
7799
}
78-
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, getTLSKeyfile, getTLSCA, getPVC, createEvent)
100+
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, c)
79101
assert.True(t, changed)
80102
assert.Len(t, newPlan, 0) // Single mode does not scale
81103

@@ -90,23 +112,14 @@ func TestCreatePlanSingleScale(t *testing.T) {
90112
PodName: "something1",
91113
},
92114
}
93-
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, getTLSKeyfile, getTLSCA, getPVC, createEvent)
115+
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, c)
94116
assert.True(t, changed)
95117
assert.Len(t, newPlan, 0) // Single mode does not scale
96118
}
97119

98120
// TestCreatePlanActiveFailoverScale creates a `ActiveFailover` deployment to test the creating of scaling plan.
99121
func TestCreatePlanActiveFailoverScale(t *testing.T) {
100-
getTLSKeyfile := func(group api.ServerGroup, member api.MemberStatus) (string, error) {
101-
return "", maskAny(fmt.Errorf("Not implemented"))
102-
}
103-
getTLSCA := func(string) (string, string, bool, error) {
104-
return "", "", false, maskAny(fmt.Errorf("Not implemented"))
105-
}
106-
getPVC := func(pvcName string) (*v1.PersistentVolumeClaim, error) {
107-
return nil, maskAny(fmt.Errorf("Not implemented"))
108-
}
109-
createEvent := func(evt *k8sutil.Event) {}
122+
c := &testContext{}
110123
log := zerolog.Nop()
111124
spec := api.DeploymentSpec{
112125
Mode: api.NewMode(api.DeploymentModeActiveFailover),
@@ -123,7 +136,7 @@ func TestCreatePlanActiveFailoverScale(t *testing.T) {
123136

124137
// Test with empty status
125138
var status api.DeploymentStatus
126-
newPlan, changed := createPlan(log, depl, nil, spec, status, nil, getTLSKeyfile, getTLSCA, getPVC, createEvent)
139+
newPlan, changed := createPlan(log, depl, nil, spec, status, nil, c)
127140
assert.True(t, changed)
128141
require.Len(t, newPlan, 2)
129142
assert.Equal(t, api.ActionTypeAddMember, newPlan[0].Type)
@@ -136,7 +149,7 @@ func TestCreatePlanActiveFailoverScale(t *testing.T) {
136149
PodName: "something",
137150
},
138151
}
139-
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, getTLSKeyfile, getTLSCA, getPVC, createEvent)
152+
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, c)
140153
assert.True(t, changed)
141154
require.Len(t, newPlan, 1)
142155
assert.Equal(t, api.ActionTypeAddMember, newPlan[0].Type)
@@ -161,7 +174,7 @@ func TestCreatePlanActiveFailoverScale(t *testing.T) {
161174
PodName: "something4",
162175
},
163176
}
164-
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, getTLSKeyfile, getTLSCA, getPVC, createEvent)
177+
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, c)
165178
assert.True(t, changed)
166179
require.Len(t, newPlan, 2) // Note: Downscaling is only down 1 at a time
167180
assert.Equal(t, api.ActionTypeShutdownMember, newPlan[0].Type)
@@ -172,16 +185,7 @@ func TestCreatePlanActiveFailoverScale(t *testing.T) {
172185

173186
// TestCreatePlanClusterScale creates a `cluster` deployment to test the creating of scaling plan.
174187
func TestCreatePlanClusterScale(t *testing.T) {
175-
getTLSKeyfile := func(group api.ServerGroup, member api.MemberStatus) (string, error) {
176-
return "", maskAny(fmt.Errorf("Not implemented"))
177-
}
178-
getTLSCA := func(string) (string, string, bool, error) {
179-
return "", "", false, maskAny(fmt.Errorf("Not implemented"))
180-
}
181-
getPVC := func(pvcName string) (*v1.PersistentVolumeClaim, error) {
182-
return nil, maskAny(fmt.Errorf("Not implemented"))
183-
}
184-
createEvent := func(evt *k8sutil.Event) {}
188+
c := &testContext{}
185189
log := zerolog.Nop()
186190
spec := api.DeploymentSpec{
187191
Mode: api.NewMode(api.DeploymentModeCluster),
@@ -197,7 +201,7 @@ func TestCreatePlanClusterScale(t *testing.T) {
197201

198202
// Test with empty status
199203
var status api.DeploymentStatus
200-
newPlan, changed := createPlan(log, depl, nil, spec, status, nil, getTLSKeyfile, getTLSCA, getPVC, createEvent)
204+
newPlan, changed := createPlan(log, depl, nil, spec, status, nil, c)
201205
assert.True(t, changed)
202206
require.Len(t, newPlan, 6) // Adding 3 dbservers & 3 coordinators (note: agents do not scale now)
203207
assert.Equal(t, api.ActionTypeAddMember, newPlan[0].Type)
@@ -230,7 +234,7 @@ func TestCreatePlanClusterScale(t *testing.T) {
230234
PodName: "coordinator1",
231235
},
232236
}
233-
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, getTLSKeyfile, getTLSCA, getPVC, createEvent)
237+
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, c)
234238
assert.True(t, changed)
235239
require.Len(t, newPlan, 3)
236240
assert.Equal(t, api.ActionTypeAddMember, newPlan[0].Type)
@@ -267,7 +271,7 @@ func TestCreatePlanClusterScale(t *testing.T) {
267271
}
268272
spec.DBServers.Count = util.NewInt(1)
269273
spec.Coordinators.Count = util.NewInt(1)
270-
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, getTLSKeyfile, getTLSCA, getPVC, createEvent)
274+
newPlan, changed = createPlan(log, depl, nil, spec, status, nil, c)
271275
assert.True(t, changed)
272276
require.Len(t, newPlan, 5) // Note: Downscaling is done 1 at a time
273277
assert.Equal(t, api.ActionTypeCleanOutMember, newPlan[0].Type)

‎pkg/deployment/resources/pod_inspector.go

+13
Original file line numberDiff line numberDiff line change
@@ -247,3 +247,16 @@ func (r *Resources) InspectPods(ctx context.Context) error {
247247
}
248248
return nil
249249
}
250+
251+
// GetExpectedPodArguments creates command line arguments for a server in the given group with given ID.
252+
func (r *Resources) GetExpectedPodArguments(apiObject metav1.Object, deplSpec api.DeploymentSpec, group api.ServerGroup,
253+
agents api.MemberStatusList, id string) []string {
254+
if group.IsArangod() {
255+
return createArangodArgs(apiObject, deplSpec, group, agents, id, false)
256+
}
257+
if group.IsArangosync() {
258+
groupSpec := deplSpec.GetServerGroupSpec(group)
259+
return createArangoSyncArgs(apiObject, deplSpec, group, groupSpec, agents, id)
260+
}
261+
return nil
262+
}

0 commit comments

Comments
 (0)