Fix #33546: always create new revision

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

@@ -39,6 +39,7 @@ class ServiceFactory(private val serviceClient: MixedOperation<Service, ServiceL
     fun create(shinyProxy: ShinyProxy, latestShinyProxyInstance: ShinyProxyInstance) {
         val labels = LabelFactory.labelsForShinyProxy(shinyProxy).toMutableMap()
         labels[LabelFactory.LATEST_INSTANCE_LABEL] = latestShinyProxyInstance.hashOfSpec
+        labels[LabelFactory.REVISION_LABEL] = latestShinyProxyInstance.revision.toString()
 
         //@formatter:off
         val serviceDefinition: Service = ServiceBuilder()

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

@@ -49,6 +49,7 @@ class ServiceController(
         val services = resourceRetriever.getServiceByLabels(LabelFactory.labelsForShinyProxy(shinyProxy), shinyProxy.metadata.namespace)
         val mustBeUpdated = services.isEmpty()
             || services[0].metadata?.labels?.get(LabelFactory.LATEST_INSTANCE_LABEL) != latestInstance.hashOfSpec
+            || services[0].metadata?.labels?.get(LabelFactory.REVISION_LABEL) != latestInstance.revision.toString()
 
         if (mustBeUpdated) {
             val replicaSet = getReplicaSet(resourceRetriever, shinyProxy, latestInstance)

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

@@ -144,11 +144,6 @@ class ShinyProxyController(private val channel: Channel<ShinyProxyEvent>,
         val shinyProxy = refreshShinyProxy(_shinyProxy) // refresh shinyproxy to ensure status is always up to date
         val existingInstance = shinyProxy.status.getInstanceByHash(shinyProxy.hashOfCurrentSpec)
 
-        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
-        }
-
         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

src/main/kotlin/eu/openanalytics/shinyproxyoperator/isInManagedNamespace.kt

@@ -23,7 +23,7 @@ package eu.openanalytics.shinyproxyoperator
 import eu.openanalytics.shinyproxyoperator.crd.ShinyProxy
 import mu.KotlinLogging
 
-val logger = KotlinLogging.logger { }
+private val logger = KotlinLogging.logger { }
 
 fun isInManagedNamespace(shinyProxy: ShinyProxy): Boolean {
     when (Operator.getOperatorInstance().mode) {

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

@@ -680,7 +680,7 @@ 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
+        // idea of test: launch instance A, update config to get instance B, and then update config again to A
         // 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
@@ -723,14 +723,95 @@ class MainIntegrationTest : IntegrationTestBase() {
             // 7. wait until instance is created
             instanceB.waitForOneReconcile()
 
-            // 7. wait for delete to not happen
+            // 8. wait for delete to not happen
             delay(5000)
 
-            // 8. assert that two instances are correctly working
+            // 9. assert that two instances are correctly working
             instanceA.assertInstanceIsCorrect(2, false)
             instanceB.assertInstanceIsCorrect(2, true)
 
-            // 9. update config to again have the config of A
+            // 10. update config to again have the config of A
+            logger.debug { "Updating config to get A'" }
+            val instanceAPrime = ShinyProxyTestInstance(
+                namespace,
+                namespacedClient,
+                shinyProxyClient,
+                "simple_config.yaml",
+                reconcileListener
+            )
+            instanceAPrime.create()
+
+            // 11. wait until instance is created
+            instanceAPrime.waitForOneReconcile()
+
+            // 12. wait for delete of instance A to happen
+            recyclableChecker.isRecyclable = true
+            instanceA.waitForDeletion(spA)
+
+            // 13. assert instance A does not exists anymore
+            assertThrows<IllegalStateException>("Instance not found") {
+                instanceA.retrieveInstance(0)
+            }
+
+            // 14. wait for delete of instance B to happen
+            instanceB.waitForDeletion(spB)
+
+            // 15. assert instance B does not exists anymore
+            assertThrows<IllegalStateException>("Instance not found") {
+                instanceB.retrieveInstance()
+            }
+
+            // 16. assert instance A' is correct
+            instanceAPrime.assertInstanceIsCorrect(1, true, 1)
+        }
+
+    @Test
+    fun `restore old config version when new instance fails`() =
+        // see #33546
+        // idea of test: launch instance A, update config to get instance B, however, instance B fails to start up, and then update config again to A
+        // 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(
+                namespace,
+                namespacedClient,
+                shinyProxyClient,
+                "simple_config.yaml",
+                reconcileListener
+            )
+            val spA = instanceA.create()
+
+            val (resourceRetriever, shinyProxyLister) = operator.prepare()
+            // 2. start the operator and let it do it's work
+            scope.launch {
+                operator.run(resourceRetriever, shinyProxyLister)
+            }
+
+            // 3. wait until instance is created
+            instanceA.waitForOneReconcile()
+
+            // 4. assert correctness
+            instanceA.assertInstanceIsCorrect()
+
+            // 5. launch an app
+            startApp(spA, instanceA)
+
+            // 6. update ShinyProxy instance
+            logger.debug { "Base instance created -> updating it" }
+            val instanceB = ShinyProxyTestInstance(
+                namespace,
+                namespacedClient,
+                shinyProxyClient,
+                "simple_config_updated_broken.yaml",
+                reconcileListener
+            )
+            val spB = instanceB.create()
+            logger.debug { "Base instance created -> updated" }
+
+            // 7. wait for instance to startup (startup will fail)
+            delay(100_000) // TODO replace by event listener
+
+            // 8. update config to again have the config of A
             logger.debug { "Updating config to get A'" }
             val instanceAPrime = ShinyProxyTestInstance(
                 namespace,
@@ -741,27 +822,27 @@ class MainIntegrationTest : IntegrationTestBase() {
             )
             instanceAPrime.create()
 
-            // 10. wait until instance is created
+            // 9. wait until instance is created
             instanceAPrime.waitForOneReconcile()
 
-            // 11. wait for delete of instance A to happen
+            // 10. wait for delete of instance A to happen
             recyclableChecker.isRecyclable = true
             instanceA.waitForDeletion(spA)
 
-            // 12. assert instance A does not exists anymore
+            // 11. assert instance A does not exists anymore
             assertThrows<IllegalStateException>("Instance not found") {
                 instanceA.retrieveInstance(0)
             }
 
-            // 13. wait for delete of instance B to happen
+            // 12. wait for delete of instance B to happen
             instanceB.waitForDeletion(spB)
 
-            // 14. assert instance B does not exists anymore
+            // 13. assert instance B does not exists anymore
             assertThrows<IllegalStateException>("Instance not found") {
                 instanceB.retrieveInstance()
             }
 
-            // 13. assert instance A' is correct
+            // 14. assert instance A' is correct
             instanceAPrime.assertInstanceIsCorrect(1, true, 1)
         }
 
@@ -1025,7 +1106,7 @@ class MainIntegrationTest : IntegrationTestBase() {
         }
 
     @Test
-    fun `test additional fqns`() =
+    fun `test additional fqdns`() =
         setup(Mode.NAMESPACED) { namespace, shinyProxyClient, namespacedClient, _, operator, reconcileListener, _ ->
             // 1. create a SP instance
             val spTestInstance = ShinyProxyTestInstance(

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

@@ -26,15 +26,12 @@ import eu.openanalytics.shinyproxyoperator.ShinyProxyClient
 import eu.openanalytics.shinyproxyoperator.components.LabelFactory
 import eu.openanalytics.shinyproxyoperator.crd.ShinyProxy
 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.dsl.Resource
-import io.fabric8.kubernetes.client.dsl.RollableScalableResource
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.GlobalScope
@@ -44,6 +41,7 @@ import kotlinx.coroutines.delay
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.withTimeout
+import mu.KotlinLogging
 import org.junit.jupiter.api.AfterAll
 import org.junit.jupiter.api.AfterEach
 
@@ -55,6 +53,7 @@ abstract class IntegrationTestBase {
 
     protected val chaosEnabled = System.getenv("SPO_TEST_CHAOS") != null
 
+    private val logger = KotlinLogging.logger { }
     private val stableClient: NamespacedKubernetesClient = createKubernetesClient()
     private val chaosClient: NamespacedKubernetesClient = if (chaosEnabled) {
         ChaosInterceptor.createChaosKubernetesClient()

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

@@ -27,14 +27,14 @@ import eu.openanalytics.shinyproxyoperator.components.LabelFactory
 import eu.openanalytics.shinyproxyoperator.components.ResourceNameFactory
 import eu.openanalytics.shinyproxyoperator.crd.ShinyProxy
 import eu.openanalytics.shinyproxyoperator.crd.ShinyProxyInstance
-import eu.openanalytics.shinyproxyoperator.logger
 import io.fabric8.kubernetes.api.model.HasMetadata
 import io.fabric8.kubernetes.api.model.IntOrString
 import io.fabric8.kubernetes.client.NamespacedKubernetesClient
 import io.fabric8.kubernetes.client.readiness.Readiness
 import kotlinx.coroutines.TimeoutCancellationException
 import kotlinx.coroutines.delay
 import kotlinx.coroutines.withTimeout
+import mu.KotlinLogging
 import kotlin.test.assertEquals
 import kotlin.test.assertNotEquals
 import kotlin.test.assertNotNull
@@ -47,7 +47,7 @@ class ShinyProxyTestInstance(private val namespace: String,
                              private val reconcileListener: ReconcileListener) {
 
     lateinit var hash: String
-    private val objectMapper = ObjectMapper().registerKotlinModule()
+    private val logger = KotlinLogging.logger { }
 
     fun create(): ShinyProxy {
         val sp: ShinyProxy = shinyProxyClient.inNamespace(namespace).load(this.javaClass.getResourceAsStream("/configs/$fileName")).serverSideApply()
@@ -59,7 +59,7 @@ class ShinyProxyTestInstance(private val namespace: String,
     }
 
     fun instance(sp: ShinyProxy, revision: Int = 0): ShinyProxyInstance {
-        return sp.status.instances.firstOrNull { it.hashOfSpec == hash  && it.revision == revision } ?: error("ShinyProxyInstance with hash $hash not found")
+        return sp.status.instances.firstOrNull { it.hashOfSpec == hash && it.revision == revision } ?: error("ShinyProxyInstance with hash $hash not found")
     }
 
     suspend fun waitForOneReconcile(): ShinyProxyInstance {
@@ -87,7 +87,8 @@ class ShinyProxyTestInstance(private val namespace: String,
     suspend fun waitForDeletion(sp: ShinyProxy, revision: Int = 0) {
         val instance = ShinyProxyInstance(hash, false, revision)
         while (client.apps().replicaSets().withName(ResourceNameFactory.createNameForReplicaSet(sp, instance)).get() != null
-            || client.configMaps().withName(ResourceNameFactory.createNameForReplicaSet(sp, instance)).get() != null) {
+            || client.configMaps().withName(ResourceNameFactory.createNameForReplicaSet(sp, instance)).get() != null
+        ) {
             delay(1000)
             logger.debug { "Pod still exists ${hash}!" }
         }
@@ -156,7 +157,8 @@ class ShinyProxyTestInstance(private val namespace: String,
         assertEquals(mapOf(
             LabelFactory.APP_LABEL to LabelFactory.APP_LABEL_VALUE,
             LabelFactory.REALM_ID_LABEL to sp.realmId,
-            LabelFactory.LATEST_INSTANCE_LABEL to sp.status.latestInstance()!!.hashOfSpec
+            LabelFactory.LATEST_INSTANCE_LABEL to sp.status.latestInstance()!!.hashOfSpec,
+            LabelFactory.REVISION_LABEL to revision.toString()
         ), service.metadata.labels)
 
         assertOwnerReferenceIsCorrect(service, sp)

src/test/resources/configs/simple_config_updated_broken.yaml

@@ -0,0 +1,40 @@
+apiVersion: openanalytics.eu/v1
+kind: ShinyProxy
+metadata:
+  name: example-shinyproxy
+spec:
+  management:
+    server:
+      port: 9091
+  fqdn: itest.local
+  image: openanalytics/shinyproxy-snapshot:3.1.0-SNAPSHOT-20240219.093646
+  proxy:
+    title: Open Analytics Shiny Proxy 2
+    logoUrl: http://www.openanalytics.eu/sites/www.openanalytics.eu/themes/oa/logo.png
+    landingPage: /
+    heartbeatRate: 10000
+    heartbeatTimeout: -1
+    port: 8080
+    authentication: simple
+    containerBackend: kubernetes
+    stop-proxies-on-shutdown: false
+    default-stop-proxy-on-logout: false
+    kubernetes:
+      namespace: itest
+      internal-networking: true
+    users:
+      - name: demo
+        password: demo
+        groups: scientists
+      - name: demo2
+        password: demo2
+        groups: mathematicians
+    specs:
+      - id: 01_hello
+        displayName: Hello Application
+        description: Application which demonstrates the basics of a Shiny app
+        containerCmd: [ "R", "-e", "shinyproxy::run_01_hello()" ]
+        containerImage: openanalytics/shinyproxy-integration-test-app
+      - id: 06_tabsets
+        container-cmd: [ "R", "-e", "shinyproxy::run_06_tabsets()" ]
+        container-image: openanalytics/shinyproxy-integration-test-app