Skip to content

Commit af15556

Browse files
manusamanusa
authored
fix: rolling().restart() doesn't remove preexistent pod template annotations
Signed-off-by: Marc Nuri <[email protected]>
1 parent 7dc21ea commit af15556

File tree

5 files changed

+144
-128
lines changed

5 files changed

+144
-128
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
### 7.2-SNAPSHOT
44

55
#### Bugs
6+
* Fix #6892: rolling().restart() doesn't remove preexistent pod template annotations
67

78
#### Improvements
89

‎kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/apps/v1/RollingUpdater.java‎

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,9 @@
4545
import java.security.NoSuchAlgorithmException;
4646
import java.time.ZoneOffset;
4747
import java.time.format.DateTimeFormatter;
48-
import java.util.Collections;
4948
import java.util.Date;
50-
import java.util.HashMap;
5149
import java.util.List;
50+
import java.util.Map;
5251
import java.util.Objects;
5352
import java.util.concurrent.CompletableFuture;
5453
import java.util.concurrent.Future;
@@ -60,7 +59,7 @@ public abstract class RollingUpdater<T extends HasMetadata, L> {
6059

6160
private static final Long DEFAULT_SERVER_GC_WAIT_TIMEOUT = 60 * 1000L; // 60 seconds
6261

63-
private static final transient Logger LOG = LoggerFactory.getLogger(RollingUpdater.class);
62+
private static final Logger LOG = LoggerFactory.getLogger(RollingUpdater.class);
6463

6564
protected final Client client;
6665
protected final String namespace;
@@ -186,33 +185,27 @@ public static <T extends HasMetadata> T restart(RollableScalableResourceOperatio
186185
}
187186

188187
public static List<Object> requestPayLoadForRolloutPause() {
189-
HashMap<String, Object> patch = new HashMap<>();
190-
patch.put("op", "add");
191-
patch.put("path", "/spec/paused");
192-
patch.put("value", true);
193-
return Collections.singletonList(patch);
188+
return List.of(Map.of(
189+
"op", "add",
190+
"path", "/spec/paused",
191+
"value", true));
194192
}
195193

196194
public static List<Object> requestPayLoadForRolloutResume() {
197-
HashMap<String, Object> patch = new HashMap<>();
198-
patch.put("op", "remove");
199-
patch.put("path", "/spec/paused");
200-
return Collections.singletonList(patch);
195+
return List.of(Map.of(
196+
"op", "remove",
197+
"path", "/spec/paused"));
201198
}
202199

203200
public static List<Object> requestPayLoadForRolloutRestart() {
204-
HashMap<String, Object> patch = new HashMap<>();
205-
HashMap<String, Object> value = new HashMap<>();
206-
patch.put("op", "replace");
207-
patch.put("path", "/spec/template/metadata/annotations");
208-
value.put("kubectl.kubernetes.io/restartedAt",
209-
new Date().toInstant().atOffset(ZoneOffset.UTC).format(DateTimeFormatter.ISO_LOCAL_DATE_TIME));
210-
patch.put("value", value);
211-
return Collections.singletonList(patch);
201+
return List.of(Map.of(
202+
"op", "add",
203+
"path", "/spec/template/metadata/annotations/kubectl.kubernetes.io~1restartedAt",
204+
"value", new Date().toInstant().atOffset(ZoneOffset.UTC).format(DateTimeFormatter.ISO_LOCAL_DATE_TIME)));
212205
}
213206

214207
/**
215-
* Lets wait until there are enough Ready pods of the given RC
208+
* Let's wait until there are enough Ready pods of the given RC
216209
*/
217210
private void waitUntilPodsAreReady(final T obj, final String namespace, final int requiredPodCount) {
218211
final AtomicInteger podCount = new AtomicInteger(0);

‎kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/DeploymentCrudTest.java‎

Lines changed: 122 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -17,111 +17,129 @@
1717

1818
import io.fabric8.kubernetes.api.model.apps.Deployment;
1919
import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder;
20-
import io.fabric8.kubernetes.api.model.apps.DeploymentList;
2120
import io.fabric8.kubernetes.client.KubernetesClient;
22-
import io.fabric8.kubernetes.client.dsl.FilterWatchListDeletable;
23-
import io.fabric8.kubernetes.client.dsl.RollableScalableResource;
2421
import io.fabric8.kubernetes.client.server.mock.EnableKubernetesMockClient;
22+
import org.assertj.core.api.InstanceOfAssertFactories;
2523
import org.junit.jupiter.api.DisplayName;
2624
import org.junit.jupiter.api.Test;
2725

28-
import java.util.Collections;
26+
import java.util.Map;
2927

28+
import static org.assertj.core.api.Assertions.assertThat;
29+
import static org.assertj.core.api.AssertionsForClassTypes.tuple;
3030
import static org.junit.jupiter.api.Assertions.assertEquals;
3131
import static org.junit.jupiter.api.Assertions.assertFalse;
3232
import static org.junit.jupiter.api.Assertions.assertNotNull;
3333
import static org.junit.jupiter.api.Assertions.assertNull;
34-
import static org.junit.jupiter.api.Assertions.assertTrue;
3534

3635
@EnableKubernetesMockClient(crud = true)
3736
class DeploymentCrudTest {
3837

3938
KubernetesClient client;
4039

4140
@Test
42-
void testCrud() {
43-
44-
Deployment deployment1 = new DeploymentBuilder().withNewMetadata()
45-
.withName("deployment1")
46-
.withNamespace("ns1")
47-
.addToLabels("testKey", "testValue")
48-
.endMetadata()
49-
.withNewSpec()
50-
.endSpec()
51-
.build();
52-
53-
Deployment deployment2 = new DeploymentBuilder().withNewMetadata()
54-
.withName("deployment2")
55-
.withNamespace("ns1")
56-
.addToLabels("testKey", "testValue")
57-
.endMetadata()
58-
.withNewSpec()
59-
.endSpec()
60-
.build();
61-
62-
Deployment deployment3 = new DeploymentBuilder().withNewMetadata()
63-
.withName("deployment3")
64-
.addToLabels("testKey", "testValue")
65-
.withNamespace("ns2")
66-
.endMetadata()
67-
.withNewSpec()
68-
.endSpec()
69-
.build();
70-
71-
client.apps().deployments().inNamespace("ns1").create(deployment1);
72-
client.apps().deployments().inNamespace("ns1").create(deployment2);
73-
client.apps().deployments().inNamespace("ns2").create(deployment3);
74-
75-
DeploymentList aDeploymentList = client.apps().deployments().inAnyNamespace().list();
76-
assertNotNull(aDeploymentList);
77-
assertEquals(3, aDeploymentList.getItems().size());
78-
79-
aDeploymentList = client.apps().deployments().inNamespace("ns1").list();
80-
assertNotNull(aDeploymentList);
81-
assertEquals(2, aDeploymentList.getItems().size());
82-
83-
FilterWatchListDeletable<Deployment, DeploymentList, RollableScalableResource<Deployment>> withLabels = client.apps()
84-
.deployments()
85-
.inAnyNamespace()
86-
.withLabels(Collections.singletonMap("testKey", "testValue"));
87-
aDeploymentList = withLabels.list();
88-
assertNotNull(aDeploymentList);
89-
assertEquals(3, aDeploymentList.getItems().size());
41+
void listInAnyNamespace() {
42+
for (var i = 1; i < 4; i++) {
43+
var deployment = new DeploymentBuilder().withNewMetadata()
44+
.withName("deployment-" + i)
45+
.withNamespace("ns-" + i)
46+
.addToLabels("testKey", "testValue-" + i)
47+
.endMetadata()
48+
.withNewSpec()
49+
.endSpec()
50+
.build();
51+
client.apps().deployments().resource(deployment).create();
52+
}
53+
assertThat(client.apps().deployments().inAnyNamespace().list().getItems())
54+
.hasSize(3)
55+
.extracting("metadata.name", "metadata.namespace")
56+
.containsExactlyInAnyOrder(
57+
tuple("deployment-1", "ns-1"),
58+
tuple("deployment-2", "ns-2"),
59+
tuple("deployment-3", "ns-3"));
60+
}
9061

91-
assertEquals(3, withLabels.resources().count());
62+
@Test
63+
void listInNamespace() {
64+
for (var i = 1; i < 4; i++) {
65+
var deployment = new DeploymentBuilder().withNewMetadata()
66+
.withName("deployment-" + i)
67+
.withNamespace("ns-" + i)
68+
.addToLabels("testKey", "testValue-" + i)
69+
.endMetadata()
70+
.withNewSpec()
71+
.endSpec()
72+
.build();
73+
client.apps().deployments().resource(deployment).create();
74+
}
75+
assertThat(client.apps().deployments().inNamespace("ns-1").list().getItems())
76+
.hasSize(1)
77+
.extracting("metadata.name", "metadata.namespace")
78+
.containsExactlyInAnyOrder(
79+
tuple("deployment-1", "ns-1"));
80+
}
9281

93-
boolean bDeleted = client.apps().deployments().inNamespace("ns2").withName("deployment3").delete().size() == 1;
94-
assertTrue(bDeleted);
82+
@Test
83+
void listWithLabels() {
84+
for (var i = 1; i < 4; i++) {
85+
var deployment = new DeploymentBuilder().withNewMetadata()
86+
.withName("deployment-" + i)
87+
.withNamespace("ns-" + i)
88+
.addToLabels("testKey", "testValue-" + i)
89+
.endMetadata()
90+
.withNewSpec()
91+
.endSpec()
92+
.build();
93+
client.apps().deployments().resource(deployment).create();
94+
}
95+
assertThat(client.apps().deployments().inAnyNamespace().withLabel("testKey", "testValue-2").list().getItems())
96+
.hasSize(1)
97+
.extracting("metadata.name", "metadata.namespace")
98+
.containsExactlyInAnyOrder(
99+
tuple("deployment-2", "ns-2"));
100+
}
95101

96-
deployment2 = client.apps().deployments()
97-
.inNamespace("ns1").withName("deployment2").edit(d -> new DeploymentBuilder(d)
98-
.editMetadata().addToLabels("key1", "value1").endMetadata()
99-
.build());
102+
@Test
103+
void delete() {
104+
client.apps().deployments()
105+
.resource(new DeploymentBuilder().withNewMetadata().withName("deployment-to-delete").endMetadata().build())
106+
.create();
107+
assertThat(client.apps().deployments().withName("deployment-to-delete").withTimeoutInMillis(100).delete())
108+
.singleElement()
109+
.extracting("name").isEqualTo("deployment-to-delete");
110+
}
100111

101-
assertNotNull(deployment2);
102-
assertEquals("value1", deployment2.getMetadata().getLabels().get("key1"));
112+
@Test
113+
void edit() {
114+
client.apps().deployments()
115+
.resource(new DeploymentBuilder().withNewMetadata().withName("deployment-to-edit").endMetadata().build())
116+
.create();
117+
var edited = client.apps().deployments().withName("deployment-to-edit").edit(d -> new DeploymentBuilder(d)
118+
.editMetadata().addToLabels("key1", "value1").endMetadata()
119+
.build());
120+
assertThat(edited).extracting("metadata.labels").isEqualTo(Map.of("key1", "value1"));
103121
}
104122

105123
@Test
106124
@DisplayName("Should replace Deployment in CRUD server")
107-
void testReplace() {
125+
void testUpdate() {
108126
// Given
109-
Deployment deployment1 = new DeploymentBuilder().withNewMetadata()
127+
var deployment = client.apps().deployments().inNamespace("ns1").resource(new DeploymentBuilder().withNewMetadata()
110128
.withName("d1")
111129
.withNamespace("ns1")
112130
.addToLabels("testKey", "testValue")
113131
.endMetadata()
114132
.withNewSpec()
115133
.endSpec()
116-
.build();
117-
deployment1 = client.apps().deployments().inNamespace("ns1").create(deployment1);
134+
.build())
135+
.create();
118136

119137
// When
120-
deployment1.getMetadata().getLabels().put("testKey", "one");
121-
client.apps().deployments().inNamespace("ns1").withName("d1").replace(deployment1);
122-
Deployment replacedDeployment = client.apps().deployments().inNamespace("ns1").withName("d1").get();
138+
deployment.getMetadata().getLabels().put("testKey", "one");
139+
client.apps().deployments().resource(deployment).update();
123140

124141
// Then
142+
Deployment replacedDeployment = client.apps().deployments().inNamespace("ns1").withName("d1").get();
125143
assertNotNull(replacedDeployment);
126144
assertFalse(replacedDeployment.getMetadata().getLabels().isEmpty());
127145
assertEquals("one", replacedDeployment.getMetadata().getLabels().get("testKey"));
@@ -131,15 +149,15 @@ void testReplace() {
131149
@DisplayName("Should pause resource")
132150
void testPause() {
133151
// Given
134-
Deployment deployment1 = new DeploymentBuilder().withNewMetadata()
152+
client.apps().deployments().inNamespace("ns1").resource(new DeploymentBuilder().withNewMetadata()
135153
.withName("d1")
136154
.withNamespace("ns1")
137155
.addToLabels("testKey", "testValue")
138156
.endMetadata()
139157
.withNewSpec()
140158
.endSpec()
141-
.build();
142-
client.apps().deployments().inNamespace("ns1").create(deployment1);
159+
.build())
160+
.create();
143161

144162
// When
145163
client.apps()
@@ -157,18 +175,17 @@ void testPause() {
157175

158176
@Test
159177
@DisplayName("Should resume rollout")
160-
void testRolloutResume() throws InterruptedException {
178+
void testRolloutResume() {
161179
// Given
162-
Deployment deployment1 = new DeploymentBuilder().withNewMetadata()
180+
client.apps().deployments().inNamespace("ns1").resource(new DeploymentBuilder().withNewMetadata()
163181
.withName("d1")
164182
.withNamespace("ns1")
165183
.addToLabels("testKey", "testValue")
166184
.endMetadata()
167185
.withNewSpec()
168-
.withPaused(true)
169186
.endSpec()
170-
.build();
171-
client.apps().deployments().inNamespace("ns1").create(deployment1);
187+
.build())
188+
.create();
172189

173190
// When
174191
client.apps()
@@ -186,35 +203,37 @@ void testRolloutResume() throws InterruptedException {
186203

187204
@Test
188205
@DisplayName("Should restart rollout")
189-
void testRolloutRestart() throws InterruptedException {
206+
void rolloutRestart() {
190207
// Given
191-
Deployment deployment1 = new DeploymentBuilder().withNewMetadata()
192-
.withName("d1")
193-
.withNamespace("ns1")
194-
.addToLabels("testKey", "testValue")
195-
.endMetadata()
196-
.withNewSpec()
197-
.withNewTemplate()
198-
.withNewMetadata()
199-
.addToAnnotations("", "")
200-
.endMetadata()
201-
.endTemplate()
202-
.endSpec()
203-
.build();
204-
client.apps().deployments().inNamespace("ns1").create(deployment1);
205-
208+
client.apps().deployments()
209+
.resource(new DeploymentBuilder().withNewMetadata().withName("deployment-to-restart").endMetadata().build())
210+
.create();
206211
// When
207-
client.apps()
208-
.deployments()
209-
.inNamespace("ns1")
210-
.withName("d1")
211-
.rolling()
212-
.restart();
213-
Deployment updatedDeployment = client.apps().deployments().inNamespace("ns1").withName("d1").get();
212+
client.apps().deployments().withName("deployment-to-restart").rolling().restart();
213+
// Then
214+
assertThat(client.apps().deployments().withName("deployment-to-restart").get())
215+
.extracting(d -> d.getSpec().getTemplate().getMetadata().getAnnotations().get("kubectl.kubernetes.io/restartedAt"))
216+
.asString().isNotBlank();
217+
}
214218

219+
@Test
220+
@DisplayName("Should restart rollout preserving annotations")
221+
void rolloutRestartPreservesAnnotations() {
222+
// Given
223+
client.apps().deployments()
224+
.resource(new DeploymentBuilder().withNewMetadata().withName("deployment-to-restart").endMetadata()
225+
.withNewSpec().withNewTemplate().withNewMetadata().addToAnnotations("original", "annotation").endMetadata()
226+
.endTemplate().endSpec()
227+
.build())
228+
.create();
229+
// When
230+
client.apps().deployments().withName("deployment-to-restart").rolling().restart();
215231
// Then
216-
assertNotNull(updatedDeployment);
217-
assertNotNull(
218-
updatedDeployment.getSpec().getTemplate().getMetadata().getAnnotations().get("kubectl.kubernetes.io/restartedAt"));
232+
assertThat(client.apps().deployments().withName("deployment-to-restart").get())
233+
.extracting("spec.template.metadata.annotations")
234+
.asInstanceOf(InstanceOfAssertFactories.map(String.class, String.class))
235+
.hasFieldOrPropertyWithValue("original", "annotation")
236+
.extracting(annotations -> annotations.get("kubectl.kubernetes.io/restartedAt"))
237+
.asString().isNotBlank();
219238
}
220239
}

0 commit comments

Comments
 (0)