Skip to content

Commit 04bd3fe

Browse files
LEDfanLEDfan
committed
Fix #32402: create new ShinyProxy server when going back to old config
1 parent 767e140 commit 04bd3fe

16 files changed

Lines changed: 116 additions & 82 deletions

File tree

‎docs/deployment/bases/clustered/crds/shinyproxy.crd.yaml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,8 @@ spec:
148148
type: string
149149
isLatestInstance:
150150
type: boolean
151+
revision:
152+
type: integer
151153
subresources:
152154
status: {}
153155
- name: v1alpha1
@@ -290,4 +292,3 @@ spec:
290292
type: boolean
291293
subresources:
292294
status: {}
293-

‎docs/deployment/bases/namespaced/crds/shinyproxy.crd.yaml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,8 @@ spec:
148148
type: string
149149
isLatestInstance:
150150
type: boolean
151+
revision:
152+
type: integer
151153
subresources:
152154
status: {}
153155
- name: v1alpha1
@@ -290,4 +292,3 @@ spec:
290292
type: boolean
291293
subresources:
292294
status: {}
293-

‎src/main/kotlin/eu/openanalytics/shinyproxyoperator/components/LabelFactory.kt‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@ object LabelFactory {
3030
return mapOf(
3131
APP_LABEL to APP_LABEL_VALUE,
3232
REALM_ID_LABEL to shinyProxy.realmId,
33-
INSTANCE_LABEL to hashOfSpec
33+
INSTANCE_LABEL to hashOfSpec,
34+
REVISION_LABEL to shinyProxyInstance.revision.toString()
3435
)
3536
}
3637

@@ -45,6 +46,7 @@ object LabelFactory {
4546
const val APP_LABEL_VALUE = "shinyproxy"
4647
const val REALM_ID_LABEL = "openanalytics.eu/sp-realm-id"
4748
const val INSTANCE_LABEL = "openanalytics.eu/sp-instance"
49+
const val REVISION_LABEL = "openanalytics.eu/sp-instance-revision"
4850
const val LATEST_INSTANCE_LABEL = "openanalytics.eu/sp-latest-instance"
4951
const val PROXIED_APP = "openanalytics.eu/sp-proxied-app"
5052

‎src/main/kotlin/eu/openanalytics/shinyproxyoperator/components/ResourceNameFactory.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,11 @@ object ResourceNameFactory {
3232
}
3333

3434
fun createNameForPod(shinyProxy: ShinyProxy, shinyProxyInstance: ShinyProxyInstance): String {
35-
return "sp-${shinyProxy.metadata.name}-pod-${shinyProxyInstance.hashOfSpec}".take(KUBE_RESOURCE_NAME_MAX_LENGTH)
35+
return "sp-${shinyProxy.metadata.name}-pod-${shinyProxyInstance.revision}-${shinyProxyInstance.hashOfSpec}".take(KUBE_RESOURCE_NAME_MAX_LENGTH)
3636
}
3737

3838
fun createNameForReplicaSet(shinyProxy: ShinyProxy, shinyProxyInstance: ShinyProxyInstance): String {
39-
return "sp-${shinyProxy.metadata.name}-rs-${shinyProxyInstance.hashOfSpec}".take(KUBE_RESOURCE_NAME_MAX_LENGTH)
39+
return "sp-${shinyProxy.metadata.name}-rs-${shinyProxyInstance.revision}-${shinyProxyInstance.hashOfSpec}".take(KUBE_RESOURCE_NAME_MAX_LENGTH)
4040
}
4141

4242
fun createNameForService(shinyProxy: ShinyProxy): String {

‎src/main/kotlin/eu/openanalytics/shinyproxyoperator/controller/PodRetriever.kt‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,7 @@ import io.fabric8.kubernetes.client.NamespacedKubernetesClient
2929
class PodRetriever(private val client: NamespacedKubernetesClient) {
3030

3131
fun getShinyProxyPods(shinyProxy: ShinyProxy, shinyProxyInstance: ShinyProxyInstance): List<Pod> {
32-
val labels = mapOf(
33-
LabelFactory.APP_LABEL to LabelFactory.APP_LABEL_VALUE,
34-
LabelFactory.INSTANCE_LABEL to shinyProxyInstance.hashOfSpec
35-
)
36-
return client.pods().inNamespace(shinyProxy.metadata.namespace).withLabels(labels).list().items
32+
return client.pods().inNamespace(shinyProxy.metadata.namespace).withLabels(LabelFactory.labelsForShinyProxyInstance(shinyProxy, shinyProxyInstance)).list().items
3733
}
3834

3935
}

‎src/main/kotlin/eu/openanalytics/shinyproxyoperator/controller/ShinyProxyController.kt‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -130,19 +130,23 @@ class ShinyProxyController(private val channel: Channel<ShinyProxyEvent>,
130130
if (existingInstance != null && existingInstance.isLatestInstance) {
131131
logger.warn { "${shinyProxy.logPrefix(existingInstance)} Trying to create new instance which already exists and is the latest instance" }
132132
return existingInstance
133-
} else if (existingInstance != null && !existingInstance.isLatestInstance) {
133+
}
134+
135+
val revision = if (existingInstance != null) {
134136
logger.info { "${shinyProxy.logPrefix(existingInstance)} Trying to create new instance which already exists and is not the latest instance. Therefore this instance will become the latest again" }
135137
// reconcile will take care of making this the latest instance again
136-
return existingInstance
138+
existingInstance.revision + 1
139+
} else {
140+
0
137141
}
138142

139143
// create new instance and add it to the list of instances
140144
// initial the instance is not the latest. Only when the ReplicaSet is created and fully running
141145
// the latestInstance marker will change to the new instance.
142-
val newInstance = ShinyProxyInstance(shinyProxy.hashOfCurrentSpec, false)
146+
val newInstance = ShinyProxyInstance(shinyProxy.hashOfCurrentSpec, false, revision)
143147
updateStatus(shinyProxy) {
144148
// Extra check, if this check is positive we have some bug
145-
val checkExistingInstance = it.status.instances.firstOrNull { instance -> instance.hashOfSpec == newInstance.hashOfSpec }
149+
val checkExistingInstance = it.status.instances.firstOrNull { instance -> instance.hashOfSpec == newInstance.hashOfSpec && instance.revision == newInstance.revision }
146150
if (checkExistingInstance != null) {
147151
// status has already been updated (e.g. after an HTTP 409 Conflict response)
148152
// remove the existing instance and add the new one, to ensure that all values are correct.
@@ -189,8 +193,7 @@ class ShinyProxyController(private val channel: Channel<ShinyProxyEvent>,
189193
}
190194

191195
private fun updateLatestMarker(shinyProxy: ShinyProxy, shinyProxyInstance: ShinyProxyInstance) {
192-
val latestInstance = shinyProxy.status.instances.firstOrNull { it.hashOfSpec == shinyProxy.hashOfCurrentSpec }
193-
?: return
196+
val latestInstance = shinyProxy.status.getInstanceByHash(shinyProxy.hashOfCurrentSpec) ?: return
194197
if (latestInstance.isLatestInstance) {
195198
// already updated marker
196199
return
@@ -204,7 +207,7 @@ class ShinyProxyController(private val channel: Channel<ShinyProxyEvent>,
204207

205208
updateStatus(shinyProxy) {
206209
it.status.instances.forEach { inst -> inst.isLatestInstance = false }
207-
it.status.instances.first { inst -> inst.hashOfSpec == latestInstance.hashOfSpec }.isLatestInstance = true
210+
it.status.getInstanceByHash(latestInstance.hashOfSpec)?.isLatestInstance = true
208211
}
209212
}
210213

@@ -282,7 +285,8 @@ class ShinyProxyController(private val channel: Channel<ShinyProxyEvent>,
282285
// take a copy of the list to check to prevent concurrent modification
283286
val instancesToCheck = shinyProxy.status.instances.toList()
284287
for (shinyProxyInstance in instancesToCheck) {
285-
if (shinyProxyInstance.isLatestInstance || shinyProxyInstance.hashOfSpec == shinyProxy.hashOfCurrentSpec) {
288+
val latestRevision = shinyProxy.status.getInstanceByHash(shinyProxyInstance.hashOfSpec)?.revision ?: 0
289+
if (shinyProxyInstance.isLatestInstance || (shinyProxyInstance.hashOfSpec == shinyProxy.hashOfCurrentSpec && shinyProxyInstance.revision >= latestRevision)) {
286290
// shinyProxyInstance is either the latest or the soon to be latest instance
287291
continue
288292
}

‎src/main/kotlin/eu/openanalytics/shinyproxyoperator/crd/ShinyProxy.kt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import io.fabric8.kubernetes.model.annotation.Group
3333
import io.fabric8.kubernetes.model.annotation.Version
3434
import javax.json.JsonPatch
3535

36+
3637
@Version("v1")
3738
@Group("openanalytics.eu")
3839
class ShinyProxy : CustomResource<JsonNode, ShinyProxyStatus>(), Namespaced {
@@ -202,7 +203,7 @@ class ShinyProxy : CustomResource<JsonNode, ShinyProxyStatus>(), Namespaced {
202203
}
203204

204205
fun logPrefix(shinyProxyInstance: ShinyProxyInstance): String {
205-
return "[${metadata.namespace}/${metadata.name}/${shinyProxyInstance.hashOfSpec}]"
206+
return "[${metadata.namespace}/${metadata.name}/${shinyProxyInstance.hashOfSpec}/${shinyProxyInstance.revision}]"
206207
}
207208

208209
fun logPrefix(): String {

‎src/main/kotlin/eu/openanalytics/shinyproxyoperator/crd/ShinyProxyInstance.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,4 +20,4 @@
2020
*/
2121
package eu.openanalytics.shinyproxyoperator.crd
2222

23-
data class ShinyProxyInstance(val hashOfSpec: String, var isLatestInstance: Boolean)
23+
data class ShinyProxyInstance(val hashOfSpec: String, var isLatestInstance: Boolean, val revision: Int = 0)

‎src/main/kotlin/eu/openanalytics/shinyproxyoperator/crd/ShinyProxyStatus.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import io.fabric8.kubernetes.api.model.KubernetesResource
2929
data class ShinyProxyStatus(val instances: ArrayList<ShinyProxyInstance> = arrayListOf()) : KubernetesResource {
3030

3131
fun getInstanceByHash(hash: String): ShinyProxyInstance? {
32-
return instances.find { it.hashOfSpec == hash }
32+
return instances.filter { it.hashOfSpec == hash }.maxByOrNull { it.revision }
3333
}
3434

3535
fun latestInstance(): ShinyProxyInstance? {

‎src/test/kotlin/eu/openanalytics/shinyproxyoperator/MainIntegrationTest.kt‎

Lines changed: 35 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -165,19 +165,26 @@ class MainIntegrationTest : IntegrationTestBase() {
165165
spTestInstance.assertInstanceIsCorrect()
166166

167167
// 5. Delete Replicaset -> reconcile -> assert it is still ok
168+
val replicaSetName = "sp-${sp.metadata.name}-rs-0-${spTestInstance.hash}".take(63)
168169
executeAsyncAfter100ms {
169-
stableClient.apps().replicaSets()
170-
.withName("sp-${sp.metadata.name}-rs-${spTestInstance.hash}".take(63)).delete()
170+
getAndDelete(stableClient.apps().replicaSets().withName(replicaSetName))
171171
logger.info { "Deleted ReplicaSet" }
172172
}
173173
spTestInstance.waitForReconcileCycle()
174174
logger.info { "Reconciled after deleting RS" }
175+
// wait for replicaset to be ready
176+
withTimeout(50_000) {
177+
while (stableClient.apps().replicaSets().withName(replicaSetName)?.get()?.status?.readyReplicas != 1){
178+
logger.info { "Replicaset not yet ready" }
179+
delay(1000)
180+
}
181+
}
182+
logger.info { "Replicaset ready" }
175183
spTestInstance.assertInstanceIsCorrect()
176184

177185
// 6. Delete ConfigMap -> reconcile -> assert it is still ok
178186
executeAsyncAfter100ms {
179-
stableClient.configMaps().withName("sp-${sp.metadata.name}-cm-${spTestInstance.hash}".take(63))
180-
.delete()
187+
getAndDelete(stableClient.configMaps().withName("sp-${sp.metadata.name}-cm-${spTestInstance.hash}".take(63)))
181188
logger.info { "Deleted ConfigMap" }
182189
}
183190
spTestInstance.waitForOneReconcile()
@@ -186,8 +193,7 @@ class MainIntegrationTest : IntegrationTestBase() {
186193

187194
// 7. Delete Service -> reconcile -> assert it is still ok
188195
executeAsyncAfter100ms {
189-
stableClient.services().withName("sp-${sp.metadata.name}-svc".take(63))
190-
.delete()
196+
getAndDelete(stableClient.services().withName("sp-${sp.metadata.name}-svc".take(63)))
191197
logger.info { "Deleted Service" }
192198
}
193199
spTestInstance.waitForOneReconcile()
@@ -196,8 +202,7 @@ class MainIntegrationTest : IntegrationTestBase() {
196202

197203
// 8. Delete Ingress -> reconcile -> assert it is still ok
198204
executeAsyncAfter100ms {
199-
stableClient.network().v1().ingresses()
200-
.withName("sp-${sp.metadata.name}-ing".take(63)).delete()
205+
getAndDelete(stableClient.network().v1().ingresses().withName("sp-${sp.metadata.name}-ing".take(63)))
201206
logger.info { "Deleted Ingress" }
202207
}
203208
spTestInstance.waitForReconcileCycle()
@@ -354,12 +359,7 @@ class MainIntegrationTest : IntegrationTestBase() {
354359
recyclableChecker.isRecyclable = true
355360

356361
// 8. wait for delete to happen
357-
while (stableClient.pods().withName("sp-${sp.metadata.name}-pod-${spTestInstanceOriginal.hash}".take(63)).get() != null
358-
|| stableClient.configMaps().withName("sp-${sp.metadata.name}-cm-${spTestInstanceOriginal.hash}".take(63)).get() != null
359-
|| stableClient.services().withName("sp-${sp.metadata.name}-svc-${spTestInstanceOriginal.hash}".take(63)).get() != null) {
360-
delay(1000)
361-
logger.debug { "Pod still exists!" }
362-
}
362+
spTestInstanceOriginal.waitForDeletion(sp)
363363

364364
// 9. assert correctness
365365
spTestInstanceUpdated.assertInstanceIsCorrect()
@@ -427,12 +427,7 @@ class MainIntegrationTest : IntegrationTestBase() {
427427
recyclableChecker.isRecyclable = true
428428

429429
// 10. wait for delete to happen
430-
while (stableClient.pods().withName("sp-${sp.metadata.name}-pod-${spTestInstanceOriginal.hash}".take(63)).get() != null
431-
|| stableClient.configMaps().withName("sp-${sp.metadata.name}-cm-${spTestInstanceOriginal.hash}".take(63)).get() != null
432-
|| stableClient.services().withName("sp-${sp.metadata.name}-svc-${spTestInstanceOriginal.hash}".take(63)).get() != null) {
433-
delay(1000)
434-
logger.debug { "Pod still exists!" }
435-
}
430+
spTestInstanceOriginal.waitForDeletion(sp)
436431

437432
// 11. assert older instance does not exist anymore
438433
assertThrows<IllegalStateException>("Instance not found") {
@@ -636,12 +631,7 @@ class MainIntegrationTest : IntegrationTestBase() {
636631
spTestInstanceUpdated.waitForOneReconcile()
637632

638633
// 7. wait for delete to happen
639-
while (stableClient.pods().withName("sp-${sp.metadata.name}-pod-${spTestInstanceOriginal.hash}".take(63)).get() != null
640-
|| stableClient.configMaps().withName("sp-${sp.metadata.name}-cm-${spTestInstanceOriginal.hash}".take(63)).get() != null
641-
|| stableClient.services().withName("sp-${sp.metadata.name}-svc-${spTestInstanceOriginal.hash}".take(63)).get() != null) {
642-
delay(1000)
643-
logger.debug { "Pod still exists!" }
644-
}
634+
spTestInstanceOriginal.waitForDeletion(sp)
645635

646636
// 8. assert correctness
647637
spTestInstanceUpdated.assertInstanceIsCorrect()
@@ -712,8 +702,8 @@ class MainIntegrationTest : IntegrationTestBase() {
712702

713703
@Test
714704
fun `restore old config version`() =
715-
// idea of test: launch instance A, update config to get instance B, and the update config again
716-
// using the same config as A, resulting in instance A' (which is the same instance as A, as A was never removed!)
705+
// idea of test: launch instance A, update config to get instance B, and the update config again
706+
// the operator will start a new instance, with an increased revision
717707
setup(Mode.NAMESPACED) { namespace, shinyProxyClient, namespacedClient, stableClient, operator, reconcileListener, recyclableChecker ->
718708
// 1. create a SP instance
719709
val instanceA = ShinyProxyTestInstance(
@@ -776,22 +766,25 @@ class MainIntegrationTest : IntegrationTestBase() {
776766
// 10. wait until instance is created
777767
instanceAPrime.waitForOneReconcile()
778768

779-
// 11. wait for delete to happen
769+
// 11. wait for delete of instance A to happen
780770
recyclableChecker.isRecyclable = true
781-
while (stableClient.pods().withName("sp-${spB.metadata.name}-pod-${instanceB.hash}".take(63)).get() != null
782-
|| stableClient.configMaps().withName("sp-${spB.metadata.name}-cm-${instanceB.hash}".take(63)).get() != null
783-
|| stableClient.services().withName("sp-${spB.metadata.name}-svc-${instanceB.hash}".take(63)).get() != null) {
784-
delay(1000)
785-
logger.debug { "Pod still exists!" }
771+
instanceA.waitForDeletion(spA)
772+
773+
// 12. assert instance A does not exists anymore
774+
assertThrows<IllegalStateException>("Instance not found") {
775+
instanceA.retrieveInstance(0)
786776
}
787777

788-
// 12. assert instance B does not exists anymore
778+
// 13. wait for delete of instance B to happen
779+
instanceB.waitForDeletion(spB)
780+
781+
// 14. assert instance B does not exists anymore
789782
assertThrows<IllegalStateException>("Instance not found") {
790783
instanceB.retrieveInstance()
791784
}
792785

793786
// 13. assert instance A' is correct
794-
instanceAPrime.assertInstanceIsCorrect(1, true)
787+
instanceAPrime.assertInstanceIsCorrect(1, true, 1)
795788

796789
job.cancel()
797790

@@ -952,7 +945,8 @@ class MainIntegrationTest : IntegrationTestBase() {
952945
assertEquals(mapOf(
953946
LabelFactory.APP_LABEL to LabelFactory.APP_LABEL_VALUE,
954947
LabelFactory.REALM_ID_LABEL to sp.realmId,
955-
LabelFactory.INSTANCE_LABEL to spTestInstance.hash
948+
LabelFactory.INSTANCE_LABEL to spTestInstance.hash,
949+
LabelFactory.REVISION_LABEL to "0"
956950
), rule.podAffinityTerm.labelSelector.matchLabels)
957951

958952
job.cancel()
@@ -1002,7 +996,8 @@ class MainIntegrationTest : IntegrationTestBase() {
1002996
assertEquals(mapOf(
1003997
LabelFactory.APP_LABEL to LabelFactory.APP_LABEL_VALUE,
1004998
LabelFactory.REALM_ID_LABEL to sp.realmId,
1005-
LabelFactory.INSTANCE_LABEL to spTestInstance.hash
999+
LabelFactory.INSTANCE_LABEL to spTestInstance.hash,
1000+
LabelFactory.REVISION_LABEL to "0"
10061001
), rule.labelSelector.matchLabels)
10071002

10081003
job.cancel()
@@ -1054,7 +1049,8 @@ class MainIntegrationTest : IntegrationTestBase() {
10541049
assertEquals(mapOf(
10551050
LabelFactory.APP_LABEL to LabelFactory.APP_LABEL_VALUE,
10561051
LabelFactory.REALM_ID_LABEL to sp.realmId,
1057-
LabelFactory.INSTANCE_LABEL to spTestInstance.hash
1052+
LabelFactory.INSTANCE_LABEL to spTestInstance.hash,
1053+
LabelFactory.REVISION_LABEL to "0"
10581054
), rule.podAffinityTerm.labelSelector.matchLabels)
10591055

10601056
job.cancel()

0 commit comments

Comments
 (0)