James colo sync by JPadovano1483 · Pull Request #103 · skupperproject/vms

JPadovano1483 · 2026-05-11T20:02:49Z

This module is responsible for managing the deployment and deletion of colocated namespaces/sites, tying their lifecycle to the lifecycle of the parent backbone network. It currently runs on MC startup, during backbone network creation and deletion, and is set to run every minute after startup to ensure up to date state reconciliation. Added a column on backboneaccesspoint to store routeraccess accesstype ('local' for colocated site's 'manage' accesspoint).

Fixes #91

ted-ross

Many of my comments don't need to be addressed to merge this PR. Some of them could be deferred to later issues.

ted-ross · 2026-05-11T20:57:22Z

+      - skupper.io
+    resources:
+      - sites
+      - links


Shouldn't need access to Links, I think.

Is the mc creating networkaccesses for colocated sites, or this will be done through the sync process?

It is done through the sync process. However, in order to give the site controller the permissions it needs to do that, the management controller itself needs those permissions. Though if there is a better way of handling that issue, please let me know.

You're right. Since it is creating the deployment for the system-controller, it must have the same permissions granted to it! Thank you!

ted-ross · 2026-05-11T20:58:16Z

+  name: {{ include "management-server.serviceAccountName" . }}
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole


We will want to squeeze the permissions in this ClusterRole down to the minimum possible set.

... not a blocker for merging, but something we will need to eventually do.

ted-ross · 2026-05-11T21:02:47Z

+import * as sync from "./sync-management.js"
+import * as common from "@skupperx/modules/common"
+
+let client


Having a module-global variable for client is different from what we do elsewhere. Is this significantly better than allocating and freeing the client on an as-needed basis?

No, I think my original thought was that since the main function polls database regularly that using the same connection pool made sense, but I will switch to the way we do it elsewhere.

ted-ross · 2026-05-11T21:04:27Z

+ */
+export async function processColoBackbones() {
+    // get all backbones with colo namespaces
+    const coloBackbones = await client.query(`SELECT Id, CoLocatedNamespace FROM Backbones WHERE NULLIF(CoLocatedNamespace, '') IS NOT NULL`).then(res => res.rows)


It is my understanding that we are not permitting CoLocatedNamespace to be the empty string.

ted-ross · 2026-05-11T21:09:42Z

+ */
+async function deploySite(backboneId, ns) {
+    const siteId = await client.query(`SELECT Id FROM InteriorSites WHERE Backbone = $1 AND CoLocated = true`, [backboneId]).then(res => res.rows[0]?.id)
+    // Poll the db against the InteriorSites table for this siteId, waiting for lifecycle='ready'


Rather than polling here and hanging up the whole process, wouldn't it be better to only sync colo sites that are in the Ready state? i.e. a colo site that is not Ready is treated as though it doesn't yet exist.

Yes that makes sense. Originally I wasn't running the main process backbones function with the settimeout, so I was concerned the deployment wouldn't get reconciled until the MC restarted or another backbone was created. This should now just get picked up the next time the function is set to run.

ted-ross · 2026-05-11T21:12:41Z

+        }
+
+        const site = result.rows[0];
+        if (site.deploymentstate == 'deployed') {


I'm not sure these checks are appropriate in this case. The "already deployed" error might get thrown if we are re-establishing an already established colo site.

ted-ross · 2026-05-11T21:13:41Z

+            resourceTemplates.BackboneSite(site.name, siteId),
+            resourceTemplates.NetworkCR('mbone'),
+        ];
+        const links = await sync.GetBackboneLinks_TX(client, siteId);


Don't apply the links here. That will be handled later by the sync protocol.

ted-ross · 2026-05-11T21:14:18Z

+        for (const [linkId, linkData] of Object.entries(links)) {
+            output.push(resourceTemplates.LinkCR(linkId, linkData, `skx-site-${siteId}`));
+        }
+        const accessPoints = await sync.GetBackboneAccessPoints_TX(client, siteId, true);


Only apply the manage access point here. Any others will be added by the sync protocol

JPadovano1483 · 2026-05-12T16:52:51Z

Changes:

Removed a couple of permissions on the namespace resource. Most of the permissions in the ClusterRole are currently needed in order to deploy the site with the permissions it needs.
moved client from module scope to function scope
moved from polling for site readiness to only pulling sites that are in the ready state
moved the creation of the colocated namespace to the same block as the creation of the site to avoid situations where the first pass creates the namespace before the site is ready and the second pass skips the creation of the site due to the namespace already existing

fgiorgetti · 2026-05-14T12:12:26Z

    Owner UUID REFERENCES Users,
-    OwnerGroup text
+    OwnerGroup text,
+    AccessType text


I am wondering if AccessType column is actually needed here?
Considering that a CoLocated Backbone Site can only have a single manage endpoint,
if the referenced InteriorSite record has Colocated field set as true, and the Kind field here is set to manage, then the access type must be local.
Unless we are anticipating future needs or scenarios.

I would leave it there in case we need to influence the accessType attribute of the resulting CR. However, it should be considered optional (or have a default value) that is the equivalent to omitting it from the CR.

fgiorgetti · 2026-05-14T12:23:52Z

+      - skupper.io
+    resources:
+      - sites
+      - links


Is the mc creating networkaccesses for colocated sites, or this will be done through the sync process?

fgiorgetti · 2026-05-14T12:27:49Z

+        if (coloBackbones.length > 0) {
+            await reconcileNamespaces(coloBackbones)
+        }
+        setTimeout(processColoBackbones, 60000)


Minor suggestion, but setTimeout could be moved to the finally block only.

NetworkAccesses will be created only via the sync protocol.

JPadovano1483 added 2 commits May 11, 2026 14:50

colocated site sync protocol

8a7750e

cleanup

f28b690

JPadovano1483 requested review from fgiorgetti and ted-ross May 11, 2026 20:02

ted-ross requested changes May 11, 2026

View reviewed changes

addressed PR comments

2498dde

fgiorgetti reviewed May 14, 2026

View reviewed changes

moved setTimeout

30239b7

Conversation

JPadovano1483 commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ted-ross left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JPadovano1483 commented May 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JPadovano1483 commented May 11, 2026 •

edited

Loading