Fix #32402: create new ShinyProxy server when going back to old config

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

@@ -148,6 +148,8 @@ spec:
                         type: string
                       isLatestInstance:
                         type: boolean
+                      revision:
+                        type: integer
       subresources:
         status: {}
     - name: v1alpha1
@@ -290,4 +292,3 @@ spec:
                         type: boolean
       subresources:
         status: {}
-

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

@@ -148,6 +148,8 @@ spec:
                         type: string
                       isLatestInstance:
                         type: boolean
+                      revision:
+                        type: integer
       subresources:
         status: {}
     - name: v1alpha1
@@ -290,4 +292,3 @@ spec:
                         type: boolean
       subresources:
         status: {}
-

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

@@ -30,7 +30,8 @@ object LabelFactory {
         return mapOf(
             APP_LABEL to APP_LABEL_VALUE,
             REALM_ID_LABEL to shinyProxy.realmId,
-            INSTANCE_LABEL to hashOfSpec
+            INSTANCE_LABEL to hashOfSpec,
+            REVISION_LABEL to shinyProxyInstance.revision.toString()
         )
     }
 
@@ -45,6 +46,7 @@ object LabelFactory {
     const val APP_LABEL_VALUE = "shinyproxy"
     const val REALM_ID_LABEL = "openanalytics.eu/sp-realm-id"
     const val INSTANCE_LABEL = "openanalytics.eu/sp-instance"
+    const val REVISION_LABEL = "openanalytics.eu/sp-instance-revision"
     const val LATEST_INSTANCE_LABEL = "openanalytics.eu/sp-latest-instance"
     const val PROXIED_APP = "openanalytics.eu/sp-proxied-app"
 

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

@@ -32,11 +32,11 @@ object ResourceNameFactory {
     }
 
     fun createNameForPod(shinyProxy: ShinyProxy, shinyProxyInstance: ShinyProxyInstance): String {
-        return "sp-${shinyProxy.metadata.name}-pod-${shinyProxyInstance.hashOfSpec}".take(KUBE_RESOURCE_NAME_MAX_LENGTH)
+        return "sp-${shinyProxy.metadata.name}-pod-${shinyProxyInstance.revision}-${shinyProxyInstance.hashOfSpec}".take(KUBE_RESOURCE_NAME_MAX_LENGTH)
     }
 
     fun createNameForReplicaSet(shinyProxy: ShinyProxy, shinyProxyInstance: ShinyProxyInstance): String {
-        return "sp-${shinyProxy.metadata.name}-rs-${shinyProxyInstance.hashOfSpec}".take(KUBE_RESOURCE_NAME_MAX_LENGTH)
+        return "sp-${shinyProxy.metadata.name}-rs-${shinyProxyInstance.revision}-${shinyProxyInstance.hashOfSpec}".take(KUBE_RESOURCE_NAME_MAX_LENGTH)
     }
 
     fun createNameForService(shinyProxy: ShinyProxy): String {

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

@@ -29,11 +29,7 @@ import io.fabric8.kubernetes.client.NamespacedKubernetesClient
 class PodRetriever(private val client: NamespacedKubernetesClient) {
 
     fun getShinyProxyPods(shinyProxy: ShinyProxy, shinyProxyInstance: ShinyProxyInstance): List<Pod> {
-        val labels = mapOf(
-            LabelFactory.APP_LABEL to LabelFactory.APP_LABEL_VALUE,
-            LabelFactory.INSTANCE_LABEL to shinyProxyInstance.hashOfSpec
-        )
-        return client.pods().inNamespace(shinyProxy.metadata.namespace).withLabels(labels).list().items
+        return client.pods().inNamespace(shinyProxy.metadata.namespace).withLabels(LabelFactory.labelsForShinyProxyInstance(shinyProxy, shinyProxyInstance)).list().items
     }
 
 }

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

@@ -130,19 +130,23 @@ class ShinyProxyController(private val channel: Channel<ShinyProxyEvent>,
         if (existingInstance != null && existingInstance.isLatestInstance) {
             logger.warn { "${shinyProxy.logPrefix(existingInstance)} Trying to create new instance which already exists and is the latest instance" }
             return existingInstance
-        } else if (existingInstance != null && !existingInstance.isLatestInstance) {
+        }
+
+        val revision = if (existingInstance != null) {
             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" }
             // reconcile will take care of making this the latest instance again
-            return existingInstance
+            existingInstance.revision + 1
+        } else {
+            0
         }
 
         // create new instance and add it to the list of instances
         // initial the instance is not the latest. Only when the ReplicaSet is created and fully running
         // the latestInstance marker will change to the new instance.
-        val newInstance = ShinyProxyInstance(shinyProxy.hashOfCurrentSpec, false)
+        val newInstance = ShinyProxyInstance(shinyProxy.hashOfCurrentSpec, false, revision)
         updateStatus(shinyProxy) {
             // Extra check, if this check is positive we have some bug
-            val checkExistingInstance = it.status.instances.firstOrNull { instance -> instance.hashOfSpec == newInstance.hashOfSpec }
+            val checkExistingInstance = it.status.instances.firstOrNull { instance -> instance.hashOfSpec == newInstance.hashOfSpec && instance.revision == newInstance.revision }
             if (checkExistingInstance != null) {
                 // status has already been updated (e.g. after an HTTP 409 Conflict response)
                 // 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>,
     }
 
     private fun updateLatestMarker(shinyProxy: ShinyProxy, shinyProxyInstance: ShinyProxyInstance) {
-        val latestInstance = shinyProxy.status.instances.firstOrNull { it.hashOfSpec == shinyProxy.hashOfCurrentSpec }
-            ?: return
+        val latestInstance = shinyProxy.status.getInstanceByHash(shinyProxy.hashOfCurrentSpec) ?: return
         if (latestInstance.isLatestInstance) {
             // already updated marker
             return
@@ -204,7 +207,7 @@ class ShinyProxyController(private val channel: Channel<ShinyProxyEvent>,
 
         updateStatus(shinyProxy) {
             it.status.instances.forEach { inst -> inst.isLatestInstance = false }
-            it.status.instances.first { inst -> inst.hashOfSpec == latestInstance.hashOfSpec }.isLatestInstance = true
+            it.status.getInstanceByHash(latestInstance.hashOfSpec)?.isLatestInstance = true
         }
     }
 
@@ -282,7 +285,8 @@ class ShinyProxyController(private val channel: Channel<ShinyProxyEvent>,
                 // take a copy of the list to check to prevent concurrent modification
                 val instancesToCheck = shinyProxy.status.instances.toList()
                 for (shinyProxyInstance in instancesToCheck) {
-                    if (shinyProxyInstance.isLatestInstance || shinyProxyInstance.hashOfSpec == shinyProxy.hashOfCurrentSpec) {
+                    val latestRevision = shinyProxy.status.getInstanceByHash(shinyProxyInstance.hashOfSpec)?.revision ?: 0
+                    if (shinyProxyInstance.isLatestInstance || (shinyProxyInstance.hashOfSpec == shinyProxy.hashOfCurrentSpec && shinyProxyInstance.revision >= latestRevision)) {
                         // shinyProxyInstance is either the latest or the soon to be latest instance
                         continue
                     }

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

@@ -33,6 +33,7 @@ import io.fabric8.kubernetes.model.annotation.Group
 import io.fabric8.kubernetes.model.annotation.Version
 import javax.json.JsonPatch
 
+
 @Version("v1")
 @Group("openanalytics.eu")
 class ShinyProxy : CustomResource<JsonNode, ShinyProxyStatus>(), Namespaced {
@@ -202,7 +203,7 @@ class ShinyProxy : CustomResource<JsonNode, ShinyProxyStatus>(), Namespaced {
     }
 
     fun logPrefix(shinyProxyInstance: ShinyProxyInstance): String {
-        return "[${metadata.namespace}/${metadata.name}/${shinyProxyInstance.hashOfSpec}]"
+        return "[${metadata.namespace}/${metadata.name}/${shinyProxyInstance.hashOfSpec}/${shinyProxyInstance.revision}]"
     }
 
     fun logPrefix(): String {

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

@@ -20,4 +20,4 @@
  */
 package eu.openanalytics.shinyproxyoperator.crd
 
-data class ShinyProxyInstance(val hashOfSpec: String, var isLatestInstance: Boolean)
+data class ShinyProxyInstance(val hashOfSpec: String, var isLatestInstance: Boolean, val revision: Int = 0)

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

@@ -29,7 +29,7 @@ import io.fabric8.kubernetes.api.model.KubernetesResource
 data class ShinyProxyStatus(val instances: ArrayList<ShinyProxyInstance> = arrayListOf()) : KubernetesResource {
 
     fun getInstanceByHash(hash: String): ShinyProxyInstance? {
-        return instances.find { it.hashOfSpec == hash }
+        return instances.filter { it.hashOfSpec == hash }.maxByOrNull { it.revision }
     }
 
     fun latestInstance(): ShinyProxyInstance? {

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

@@ -165,19 +165,26 @@ class MainIntegrationTest : IntegrationTestBase() {
             spTestInstance.assertInstanceIsCorrect()
 
             // 5. Delete Replicaset -> reconcile -> assert it is still ok
+            val replicaSetName = "sp-${sp.metadata.name}-rs-0-${spTestInstance.hash}".take(63)
             executeAsyncAfter100ms {
-                stableClient.apps().replicaSets()
-                    .withName("sp-${sp.metadata.name}-rs-${spTestInstance.hash}".take(63)).delete()
+                getAndDelete(stableClient.apps().replicaSets().withName(replicaSetName))
                 logger.info { "Deleted ReplicaSet" }
             }
             spTestInstance.waitForReconcileCycle()
             logger.info { "Reconciled after deleting RS" }
+            // wait for replicaset to be ready
+            withTimeout(50_000) {
+                while (stableClient.apps().replicaSets().withName(replicaSetName)?.get()?.status?.readyReplicas != 1){
+                    logger.info { "Replicaset not yet ready" }
+                    delay(1000)
+                }
+            }
+            logger.info { "Replicaset ready" }
             spTestInstance.assertInstanceIsCorrect()
 
             // 6. Delete ConfigMap -> reconcile -> assert it is still ok
             executeAsyncAfter100ms {
-                stableClient.configMaps().withName("sp-${sp.metadata.name}-cm-${spTestInstance.hash}".take(63))
-                    .delete()
+                getAndDelete(stableClient.configMaps().withName("sp-${sp.metadata.name}-cm-${spTestInstance.hash}".take(63)))
                 logger.info { "Deleted ConfigMap" }
             }
             spTestInstance.waitForOneReconcile()
@@ -186,8 +193,7 @@ class MainIntegrationTest : IntegrationTestBase() {
 
             // 7. Delete Service -> reconcile -> assert it is still ok
             executeAsyncAfter100ms {
-                stableClient.services().withName("sp-${sp.metadata.name}-svc".take(63))
-                    .delete()
+                getAndDelete(stableClient.services().withName("sp-${sp.metadata.name}-svc".take(63)))
                 logger.info { "Deleted Service" }
             }
             spTestInstance.waitForOneReconcile()
@@ -196,8 +202,7 @@ class MainIntegrationTest : IntegrationTestBase() {
 
             // 8. Delete Ingress -> reconcile -> assert it is still ok
             executeAsyncAfter100ms {
-                stableClient.network().v1().ingresses()
-                    .withName("sp-${sp.metadata.name}-ing".take(63)).delete()
+                getAndDelete(stableClient.network().v1().ingresses().withName("sp-${sp.metadata.name}-ing".take(63)))
                 logger.info { "Deleted Ingress" }
             }
             spTestInstance.waitForReconcileCycle()
@@ -354,12 +359,7 @@ class MainIntegrationTest : IntegrationTestBase() {
             recyclableChecker.isRecyclable = true
 
             // 8. wait for delete to happen
-            while (stableClient.pods().withName("sp-${sp.metadata.name}-pod-${spTestInstanceOriginal.hash}".take(63)).get() != null
-                || stableClient.configMaps().withName("sp-${sp.metadata.name}-cm-${spTestInstanceOriginal.hash}".take(63)).get() != null
-                || stableClient.services().withName("sp-${sp.metadata.name}-svc-${spTestInstanceOriginal.hash}".take(63)).get() != null) {
-                delay(1000)
-                logger.debug { "Pod still exists!" }
-            }
+            spTestInstanceOriginal.waitForDeletion(sp)
 
             // 9. assert correctness
             spTestInstanceUpdated.assertInstanceIsCorrect()
@@ -427,12 +427,7 @@ class MainIntegrationTest : IntegrationTestBase() {
             recyclableChecker.isRecyclable = true
 
             // 10. wait for delete to happen
-            while (stableClient.pods().withName("sp-${sp.metadata.name}-pod-${spTestInstanceOriginal.hash}".take(63)).get() != null
-                || stableClient.configMaps().withName("sp-${sp.metadata.name}-cm-${spTestInstanceOriginal.hash}".take(63)).get() != null
-                || stableClient.services().withName("sp-${sp.metadata.name}-svc-${spTestInstanceOriginal.hash}".take(63)).get() != null) {
-                delay(1000)
-                logger.debug { "Pod still exists!" }
-            }
+            spTestInstanceOriginal.waitForDeletion(sp)
 
             // 11. assert older instance does not exist anymore
             assertThrows<IllegalStateException>("Instance not found") {
@@ -636,12 +631,7 @@ class MainIntegrationTest : IntegrationTestBase() {
             spTestInstanceUpdated.waitForOneReconcile()
 
             // 7. wait for delete to happen
-            while (stableClient.pods().withName("sp-${sp.metadata.name}-pod-${spTestInstanceOriginal.hash}".take(63)).get() != null
-                || stableClient.configMaps().withName("sp-${sp.metadata.name}-cm-${spTestInstanceOriginal.hash}".take(63)).get() != null
-                || stableClient.services().withName("sp-${sp.metadata.name}-svc-${spTestInstanceOriginal.hash}".take(63)).get() != null) {
-                delay(1000)
-                logger.debug { "Pod still exists!" }
-            }
+            spTestInstanceOriginal.waitForDeletion(sp)
 
             // 8. assert correctness
             spTestInstanceUpdated.assertInstanceIsCorrect()
@@ -712,8 +702,8 @@ class MainIntegrationTest : IntegrationTestBase() {
 
     @Test
     fun `restore old config version`() =
-    // idea of test: launch instance A, update config to get instance B, and the update config again
-        // using the same config as A, resulting in instance A' (which is the same instance as A, as A was never removed!)
+        // idea of test: launch instance A, update config to get instance B, and the update config again
+        // the operator will start a new instance, with an increased revision
         setup(Mode.NAMESPACED) { namespace, shinyProxyClient, namespacedClient, stableClient, operator, reconcileListener, recyclableChecker ->
             // 1. create a SP instance
             val instanceA = ShinyProxyTestInstance(
@@ -776,22 +766,25 @@ class MainIntegrationTest : IntegrationTestBase() {
             // 10. wait until instance is created
             instanceAPrime.waitForOneReconcile()
 
-            // 11. wait for delete to happen
+            // 11. wait for delete of instance A to happen
             recyclableChecker.isRecyclable = true
-            while (stableClient.pods().withName("sp-${spB.metadata.name}-pod-${instanceB.hash}".take(63)).get() != null
-                || stableClient.configMaps().withName("sp-${spB.metadata.name}-cm-${instanceB.hash}".take(63)).get() != null
-                || stableClient.services().withName("sp-${spB.metadata.name}-svc-${instanceB.hash}".take(63)).get() != null) {
-                delay(1000)
-                logger.debug { "Pod still exists!" }
+            instanceA.waitForDeletion(spA)
+
+            // 12. assert instance A does not exists anymore
+            assertThrows<IllegalStateException>("Instance not found") {
+                instanceA.retrieveInstance(0)
             }
 
-            // 12. assert instance B does not exists anymore
+            // 13. wait for delete of instance B to happen
+            instanceB.waitForDeletion(spB)
+
+            // 14. assert instance B does not exists anymore
             assertThrows<IllegalStateException>("Instance not found") {
                 instanceB.retrieveInstance()
             }
 
             // 13. assert instance A' is correct
-            instanceAPrime.assertInstanceIsCorrect(1, true)
+            instanceAPrime.assertInstanceIsCorrect(1, true, 1)
 
             job.cancel()
 
@@ -952,7 +945,8 @@ class MainIntegrationTest : IntegrationTestBase() {
             assertEquals(mapOf(
                 LabelFactory.APP_LABEL to LabelFactory.APP_LABEL_VALUE,
                 LabelFactory.REALM_ID_LABEL to sp.realmId,
-                LabelFactory.INSTANCE_LABEL to spTestInstance.hash
+                LabelFactory.INSTANCE_LABEL to spTestInstance.hash,
+                LabelFactory.REVISION_LABEL to "0"
             ), rule.podAffinityTerm.labelSelector.matchLabels)
 
             job.cancel()
@@ -1002,7 +996,8 @@ class MainIntegrationTest : IntegrationTestBase() {
             assertEquals(mapOf(
                 LabelFactory.APP_LABEL to LabelFactory.APP_LABEL_VALUE,
                 LabelFactory.REALM_ID_LABEL to sp.realmId,
-                LabelFactory.INSTANCE_LABEL to spTestInstance.hash
+                LabelFactory.INSTANCE_LABEL to spTestInstance.hash,
+                LabelFactory.REVISION_LABEL to "0"
             ), rule.labelSelector.matchLabels)
 
             job.cancel()
@@ -1054,7 +1049,8 @@ class MainIntegrationTest : IntegrationTestBase() {
             assertEquals(mapOf(
                 LabelFactory.APP_LABEL to LabelFactory.APP_LABEL_VALUE,
                 LabelFactory.REALM_ID_LABEL to sp.realmId,
-                LabelFactory.INSTANCE_LABEL to spTestInstance.hash
+                LabelFactory.INSTANCE_LABEL to spTestInstance.hash,
+                LabelFactory.REVISION_LABEL to "0"
             ), rule.podAffinityTerm.labelSelector.matchLabels)
 
             job.cancel()

src/test/kotlin/eu/openanalytics/shinyproxyoperator/helpers/IntegrationTestBase.kt

@@ -29,10 +29,12 @@ import eu.openanalytics.shinyproxyoperator.createKubernetesClient
 import eu.openanalytics.shinyproxyoperator.logger
 import io.fabric8.kubernetes.api.model.NamespaceBuilder
 import io.fabric8.kubernetes.api.model.PodList
+import io.fabric8.kubernetes.api.model.apps.ReplicaSet
 import io.fabric8.kubernetes.client.KubernetesClient
 import io.fabric8.kubernetes.client.KubernetesClientException
 import io.fabric8.kubernetes.client.NamespacedKubernetesClient
-import io.fabric8.kubernetes.client.extended.run.RunConfigBuilder
+import io.fabric8.kubernetes.client.dsl.Resource
+import io.fabric8.kubernetes.client.dsl.RollableScalableResource
 import kotlinx.coroutines.GlobalScope
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.delay
@@ -221,6 +223,13 @@ abstract class IntegrationTestBase {
         )).list()
     }
 
+    protected fun <T> getAndDelete(resource: Resource<T>) {
+        if (resource.get() == null) {
+            throw IllegalStateException("Trying to delete resource but it does not exist!")
+        }
+        resource.delete()
+    }
+
 }
 
 fun KubernetesClient.isStartupProbesSupported(): Boolean {