Skip to content

Commit 541edbf

Browse files
willthamesfabianvf
willthames
authored and
fabianvf
committed
Apply should work if resources change in between (#339)
* Apply should work if resources change in between In the scenario: * apply a deployment with replicas set to 1 * scale the deployment to 0 replicas * apply the deployment with some changes, but replicas set to 1 The deployment should be applied with replicas set to 1. Fixing that problem had a massive knock on effect to how we manage lists, but also likely removed a number of more subtle bugs. We use strategic merge information to better merge lists now, and increase the number of tests in the test suite to validate that * Fix apply for strategic merge of lists
1 parent 271a4ad commit 541edbf

File tree

4 files changed

+270
-12
lines changed

4 files changed

+270
-12
lines changed

‎openshift/dynamic/apply.py‎

Lines changed: 90 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,54 @@
1+
from collections import OrderedDict
12
from copy import deepcopy
23
import json
34

4-
from openshift.dynamic.exceptions import NotFoundError
5+
from openshift.dynamic.exceptions import NotFoundError, ApplyException
56

67
LAST_APPLIED_CONFIG_ANNOTATION = 'kubectl.kubernetes.io/last-applied-configuration'
78

9+
POD_SPEC_SUFFIXES = {
10+
'containers': 'name',
11+
'initContainers': 'name',
12+
'ephemeralContainers': 'name',
13+
'volumes': 'name',
14+
'imagePullSecrets': 'name',
15+
'containers.volumeMounts': 'mountPath',
16+
'containers.volumeDevices': 'devicePath',
17+
'containers.envVars': 'name',
18+
'containers.ports': 'containerPort',
19+
'initContainers.volumeMounts': 'mountPath',
20+
'initContainers.volumeDevices': 'devicePath',
21+
'initContainers.envVars': 'name',
22+
'initContainers.ports': 'containerPort',
23+
'ephemeralContainers.volumeMounts': 'mountPath',
24+
'ephemeralContainers.volumeDevices': 'devicePath',
25+
'ephemeralContainers.envVars': 'name',
26+
'ephemeralContainers.ports': 'containerPort',
27+
}
28+
29+
POD_SPEC_PREFIXES = [
30+
'Pod.spec',
31+
'Deployment.spec.template.spec',
32+
'DaemonSet.spec.template.spec',
33+
'StatefulSet.spec.template.spec',
34+
'Job.spec.template.spec',
35+
'Cronjob.spec.jobTemplate.spec.template.spec',
36+
]
37+
38+
# patch merge keys taken from generated.proto files under
39+
# staging/src/k8s.io/api in kubernetes/kubernetes
40+
STRATEGIC_MERGE_PATCH_KEYS = {
41+
'Service.spec.ports': 'port',
42+
'ServiceAccount.secrets': 'name',
43+
}
44+
45+
STRATEGIC_MERGE_PATCH_KEYS.update(
46+
{"%s.%s" % (prefix, key): value
47+
for prefix in POD_SPEC_PREFIXES
48+
for key, value in POD_SPEC_SUFFIXES.items()}
49+
)
50+
51+
852
def apply_object(resource, definition):
953
desired_annotation = dict(
1054
metadata=dict(
@@ -50,7 +94,7 @@ def apply(resource, definition):
5094
# deletions, and then apply delta to deletions as a patch, which should be strictly additive.
5195
def merge(last_applied, desired, actual):
5296
deletions = get_deletions(last_applied, desired)
53-
delta = get_delta(actual, desired)
97+
delta = get_delta(last_applied, actual, desired, desired['kind'])
5498
return dict_merge(deletions, delta)
5599

56100

@@ -70,6 +114,39 @@ def dict_merge(a, b):
70114
return result
71115

72116

117+
def list_to_dict(lst, key, position):
118+
result = OrderedDict()
119+
for item in lst:
120+
try:
121+
result[item[key]] = item
122+
except KeyError:
123+
raise ApplyException("Expected key '%s' not found in position %s" % (key, position))
124+
return result
125+
126+
127+
# list_merge applies a strategic merge to a set of lists if the patchMergeKey is known
128+
# each item in the list is compared based on the patchMergeKey - if two values with the
129+
# same patchMergeKey differ, we take the keys that are in last applied, compare the
130+
# actual and desired for those keys, and update if any differ
131+
def list_merge(last_applied, actual, desired, position):
132+
result = list()
133+
if position in STRATEGIC_MERGE_PATCH_KEYS and last_applied:
134+
patch_merge_key = STRATEGIC_MERGE_PATCH_KEYS[position]
135+
last_applied_dict = list_to_dict(last_applied, patch_merge_key, position)
136+
actual_dict = list_to_dict(actual, patch_merge_key, position)
137+
desired_dict = list_to_dict(desired, patch_merge_key, position)
138+
for key in desired_dict:
139+
if key not in actual_dict or key not in last_applied_dict:
140+
result.append(desired_dict[key])
141+
else:
142+
deletions = set(last_applied_dict[key].keys()) - set(desired_dict[key].keys())
143+
result.append(dict_merge({k: v for k, v in actual_dict[key].items() if k not in deletions},
144+
desired_dict[key]))
145+
return result
146+
else:
147+
return desired
148+
149+
73150
def get_deletions(last_applied, desired):
74151
patch = {}
75152
for k, last_applied_value in last_applied.items():
@@ -83,14 +160,23 @@ def get_deletions(last_applied, desired):
83160
return patch
84161

85162

86-
def get_delta(actual, desired):
163+
def get_delta(last_applied, actual, desired, position=None):
87164
patch = {}
165+
88166
for k, desired_value in desired.items():
167+
if position:
168+
this_position = "%s.%s" % (position, k)
89169
actual_value = actual.get(k)
90170
if actual_value is None:
91171
patch[k] = desired_value
92172
elif isinstance(desired_value, dict):
93-
p = get_delta(actual_value, desired_value)
173+
p = get_delta(last_applied.get(k), actual_value, desired_value, this_position)
94174
if p:
95175
patch[k] = p
176+
elif isinstance(desired_value, list):
177+
p = list_merge(last_applied.get(k), actual_value, desired_value, this_position)
178+
if p:
179+
patch[k] = [item for item in p if item]
180+
elif actual_value != desired_value:
181+
patch[k] = desired_value
96182
return patch

‎openshift/dynamic/client.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
from .apply import apply
88
from .discovery import EagerDiscoverer, LazyDiscoverer
9-
from .exceptions import api_exception, KubernetesValidateMissing
9+
from .exceptions import api_exception, KubernetesValidateMissing, ApplyException
1010
from .resource import Resource, ResourceList, Subresource, ResourceInstance, ResourceField
1111

1212
try:
@@ -139,7 +139,11 @@ def apply(self, resource, body=None, name=None, namespace=None):
139139
raise ValueError("name is required to apply {}.{}".format(resource.group_version, resource.kind))
140140
if resource.namespaced:
141141
body['metadata']['namespace'] = self.ensure_namespace(resource, namespace, body)
142-
return apply(resource, body)
142+
try:
143+
return apply(resource, body)
144+
except ApplyException as e:
145+
raise ValueError("Could not apply strategic merge to %s/%s: %s" %
146+
(body['kind'], body['metadata']['name'], e))
143147

144148
def watch(self, resource, namespace=None, name=None, label_selector=None, field_selector=None, resource_version=None, timeout=None):
145149
"""

‎openshift/dynamic/exceptions.py‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,9 @@ class ResourceNotUniqueError(Exception):
6969
class KubernetesValidateMissing(Exception):
7070
""" kubernetes-validate is not installed """
7171

72+
class ApplyException(Exception):
73+
""" Could not apply patch """
74+
7275
# HTTP Errors
7376
class BadRequestError(DynamicApiError):
7477
""" 400: StatusBadRequest """

‎test/unit/test_apply.py‎

Lines changed: 171 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
1-
# Test ConfigMapHash and SecretHash equivalents
2-
# tests based on https://github.com/kubernetes/kubernetes/pull/49961
3-
41
from openshift.dynamic.apply import merge
52

63
tests = [
4+
75
dict(
86
last_applied = dict(
97
kind="ConfigMap",
@@ -59,7 +57,7 @@
5957
metadata=dict(name="foo"),
6058
spec=dict(ports=[dict(port=8080, name="http")])
6159
),
62-
expected = {}
60+
expected = dict(spec=dict(ports=[dict(port=8080, protocol='TCP', name="http")]))
6361
),
6462
dict(
6563
last_applied = dict(
@@ -79,7 +77,62 @@
7977
),
8078
expected = dict(spec=dict(ports=[dict(port=8081, name="http")]))
8179
),
82-
# This last one is based on a real world case where definition was mostly
80+
dict(
81+
last_applied = dict(
82+
kind="Service",
83+
metadata=dict(name="foo"),
84+
spec=dict(ports=[dict(port=8080, name="http")])
85+
),
86+
actual = dict(
87+
kind="Service",
88+
metadata=dict(name="foo"),
89+
spec=dict(ports=[dict(port=8080, protocol='TCP', name="http")])
90+
),
91+
desired = dict(
92+
kind="Service",
93+
metadata=dict(name="foo"),
94+
spec=dict(ports=[dict(port=8443, name="https"), dict(port=8080, name="http")])
95+
),
96+
expected = dict(spec=dict(ports=[dict(port=8443, name="https"), dict(port=8080, name="http", protocol='TCP')]))
97+
),
98+
dict(
99+
last_applied = dict(
100+
kind="Service",
101+
metadata=dict(name="foo"),
102+
spec=dict(ports=[dict(port=8443, name="https"), dict(port=8080, name="http")])
103+
),
104+
actual = dict(
105+
kind="Service",
106+
metadata=dict(name="foo"),
107+
spec=dict(ports=[dict(port=8443, protocol='TCP', name="https"), dict(port=8080, protocol='TCP', name='http')])
108+
),
109+
desired = dict(
110+
kind="Service",
111+
metadata=dict(name="foo"),
112+
spec=dict(ports=[dict(port=8080, name="http")])
113+
),
114+
expected = dict(spec=dict(ports=[dict(port=8080, name="http", protocol='TCP')]))
115+
),
116+
dict(
117+
last_applied = dict(
118+
kind="Service",
119+
metadata=dict(name="foo"),
120+
spec=dict(ports=[dict(port=8443, name="https", madeup="xyz"), dict(port=8080, name="http")])
121+
),
122+
actual = dict(
123+
kind="Service",
124+
metadata=dict(name="foo"),
125+
spec=dict(ports=[dict(port=8443, protocol='TCP', name="https", madeup="xyz"), dict(port=8080, protocol='TCP', name='http')])
126+
),
127+
desired = dict(
128+
kind="Service",
129+
metadata=dict(name="foo"),
130+
spec=dict(ports=[dict(port=8443, name="https")])
131+
),
132+
expected = dict(spec=dict(ports=[dict(port=8443, name="https", protocol='TCP')]))
133+
),
134+
135+
# This next one is based on a real world case where definition was mostly
83136
# str type and everything else was mostly unicode type (don't ask me how)
84137
dict(
85138
last_applied = {
@@ -106,7 +159,119 @@
106159
},
107160
expected = dict()
108161
),
109-
162+
# apply a Deployment, then scale the Deployment (which doesn't affect last-applied)
163+
# then apply the Deployment again. Should un-scale the Deployment
164+
dict(
165+
last_applied = {
166+
'kind': u'Deployment',
167+
'spec': {
168+
'replicas': 1,
169+
'template': {
170+
'spec': {
171+
'containers': [
172+
{
173+
'name': 'this_must_exist',
174+
'envFrom': [
175+
{
176+
'configMapRef': {
177+
'name': 'config-xyz'
178+
}
179+
},
180+
{
181+
'secretRef': {
182+
'name': 'config-wxy'
183+
}
184+
}
185+
]
186+
}
187+
]
188+
}
189+
}
190+
},
191+
'metadata': {
192+
'namespace': 'apply',
193+
'name': u'apply-deployment'
194+
}
195+
},
196+
actual = {
197+
'kind': u'Deployment',
198+
'spec': {
199+
'replicas': 0,
200+
'template': {
201+
'spec': {
202+
'containers': [
203+
{
204+
'name': 'this_must_exist',
205+
'envFrom': [
206+
{
207+
'configMapRef': {
208+
'name': 'config-xyz'
209+
}
210+
},
211+
{
212+
'secretRef': {
213+
'name': 'config-wxy'
214+
}
215+
}
216+
]
217+
}
218+
]
219+
}
220+
}
221+
},
222+
'metadata': {
223+
'namespace': 'apply',
224+
'name': u'apply-deployment'
225+
}
226+
},
227+
desired = {
228+
'kind': u'Deployment',
229+
'spec': {
230+
'replicas': 1,
231+
'template': {
232+
'spec': {
233+
'containers': [
234+
{
235+
'name': 'this_must_exist',
236+
'envFrom': [
237+
{
238+
'configMapRef': {
239+
'name': 'config-abc'
240+
}
241+
}
242+
]
243+
}
244+
]
245+
}
246+
}
247+
},
248+
'metadata': {
249+
'namespace': 'apply',
250+
'name': u'apply-deployment'
251+
}
252+
},
253+
expected = {
254+
'spec' : {
255+
'replicas': 1,
256+
'template': {
257+
'spec': {
258+
'containers': [
259+
{
260+
'name': 'this_must_exist',
261+
'envFrom': [
262+
{
263+
'configMapRef': {
264+
'name': 'config-abc'
265+
}
266+
}
267+
]
268+
}
269+
]
270+
}
271+
}
272+
}
273+
}
274+
)
110275
]
111276

112277

0 commit comments

Comments
 (0)