-
Notifications
You must be signed in to change notification settings - Fork 0
fix: disable keycloak on test3 for benchmark runs #81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 53 commits
eab99f7
f98c9e2
73c40d8
e159b25
e5c87d2
9083a5f
3dde7e6
bfe2a7d
16cc1cb
99e350e
e88558e
a47e4ac
08d8c4c
865a6c7
7ae3a08
6a53fe8
d364437
21f5254
985715d
b747324
2b98737
79858a9
5096fd5
8d6b04e
792f4bd
ca037ce
104825f
7bdbd38
42d1011
84c0d06
ab9102b
47b5766
1a913fb
75e76db
3c24f99
636c3d7
e3d645c
728b359
7a7a92a
c2fc148
3fb95ce
6ef8ba6
d108d4d
d3ebc08
4d5838d
42e84aa
f169694
e532605
1e55177
8b07630
55ac3c1
29bef9a
0492130
41bd05e
87ce2af
4fb5251
3f89c9d
22baabd
195a68f
9849e33
21cee22
5e8368e
77ea9cf
0197378
50976b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -85,6 +85,11 @@ on: | |||||||||||||||
| required: false | ||||||||||||||||
| type: string | ||||||||||||||||
| default: "self-hosted-buildkit" | ||||||||||||||||
| clean_install: | ||||||||||||||||
| description: "If true, perform namespace-scoped cleanup before Helm install" | ||||||||||||||||
| required: false | ||||||||||||||||
| type: boolean | ||||||||||||||||
| default: false | ||||||||||||||||
|
|
||||||||||||||||
| jobs: | ||||||||||||||||
| helm-install: | ||||||||||||||||
|
|
@@ -250,6 +255,7 @@ jobs: | |||||||||||||||
| env: | ||||||||||||||||
| KUBECONFIG: ${{ github.workspace }}/kubeconfig | ||||||||||||||||
| run: | | ||||||||||||||||
| set -euo pipefail | ||||||||||||||||
|
|
||||||||||||||||
| # Install cluster-wide monitoring (PodMonitors + Grafana Dashboards) | ||||||||||||||||
| # This is installed once per cluster, not per environment | ||||||||||||||||
|
|
@@ -276,6 +282,39 @@ jobs: | |||||||||||||||
| --set wildcardTLSSecret.certificate="$(cat shared-gateway-wildcard.crt)" \ | ||||||||||||||||
| --set wildcardTLSSecret.key="$(cat shared-gateway-wildcard.key)" | ||||||||||||||||
|
|
||||||||||||||||
| - name: Clean install - namespace-scoped resource purge | ||||||||||||||||
| if: inputs.clean_install && github.event_name == 'workflow_dispatch' | ||||||||||||||||
| env: | ||||||||||||||||
| KUBECONFIG: ${{ github.workspace }}/kubeconfig | ||||||||||||||||
| run: | | ||||||||||||||||
| set -euo pipefail | ||||||||||||||||
|
|
||||||||||||||||
| NAMESPACE="${{ vars.NAMESPACE }}" | ||||||||||||||||
|
||||||||||||||||
| NAMESPACE="${{ vars.NAMESPACE }}" | |
| NAMESPACE="${{ vars.NAMESPACE }}" | |
| if [ -z "$NAMESPACE" ]; then | |
| echo "ERROR: NAMESPACE is not set or is empty. Aborting clean install purge to avoid deleting resources in an unintended namespace." >&2 | |
| exit 1 | |
| fi | |
| echo "Running clean install purge for namespace: $NAMESPACE" |
Copilot
AI
Mar 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step performs destructive deletes based on NAMESPACE="${{ vars.NAMESPACE }}" but doesn’t validate that the namespace is non-empty / expected. With set -euo pipefail, a misconfigured or empty namespace could lead to deleting resources in the runner’s default namespace. Add an explicit safety check (e.g., fail fast if NAMESPACE is empty or equals default, and echo the namespace before deleting).
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -43,37 +43,32 @@ theia-cloud: | |||||
| interval: 3 | ||||||
|
|
||||||
| operator: | ||||||
| image: ghcr.io/eduide/eduide-cloud/operator:pr-70 | ||||||
| #eagerStart: false | ||||||
| image: ghcr.io/eduide/eduide-cloud/operator:pr-101 | ||||||
| eagerStart: false | ||||||
| replicas: 1 | ||||||
| sessionsPerUser: 10 | ||||||
|
Comment on lines
45
to
49
|
||||||
| # Test3 runs on the parma cluster where Longhorn is the default storage backend. | ||||||
|
||||||
| # Test3 runs on the parma cluster where Longhorn is the default storage backend. | |
| # Test3 runs on the parma cluster using the csi-rbd-sc storage class. |
Copilot
AI
Mar 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description says only Keycloak should be disabled and all other test3 values kept unchanged, but this file also updates operator/service images, landing page image/tagging, preloading images, defaultImageTag, labels, and adds an app definition. Either narrow the changes to match the PR intent or update the PR description to reflect these additional changes (and why they’re required for benchmark runs).
Copilot
AI
Mar 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
landingPage.image is set to :latest, which makes the test3 landing page (and benchmark entrypoint) non-deterministic across runs. If the goal is deterministic benchmark runs, consider pinning this to an immutable tag (or digest) that matches the benchmark baseline.
| image: ghcr.io/eduide/eduidec-landing-page:latest | |
| image: ghcr.io/eduide/eduidec-landing-page:pr-131 |
Copilot
AI
Mar 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description/title says only Keycloak should be disabled for test3 while keeping other test3 values unchanged, but this file also changes multiple unrelated settings (e.g., preloading image tags, landingPage image/tag, appdefinitions defaultImageTag and adds a new app). Either revert the unrelated changes or update the PR description/scope accordingly so reviewers know what is intended to change for benchmark runs.
Copilot
AI
Mar 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description says the only functional change for test3 is disabling Keycloak and otherwise keeping values unchanged, but this file also changes operator/service image tags, preload image list, landing page image, ephemeralStorage, app labels, and appdefinitions scaling/app list. If those are unintended, it would be safer to revert them; if intended, the PR description should be updated to match the actual scope.
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description says only Keycloak should be disabled for test3 and all other test3 values kept unchanged, but this file also changes operator/service image tags, eagerStart, preloading image list, landingPage ephemeralStorage, and appdefinition scaling/default tags. Please either narrow the diff to only the Keycloak toggle, or update the PR description (and ideally split into separate PRs) so reviewers can validate the additional behavioral/operational changes explicitly.
Copilot
AI
Apr 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theia-appdefinitions scaling defaults are changed significantly here (e.g., minInstances: 5 / maxInstances: 200). Even though the chart preserves live scaling after initial creation, these values still affect new/clean installs and can materially increase resource usage in test3. If the intent is only unauthenticated access for benchmarks, consider reverting these scaling/appdefinition changes or documenting why they’re required.
Outdated
Copilot
AI
Mar 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The theia-appdefinitions chart preserves existing AppDefinition minInstances/maxInstances from the live resource (via lookup) on subsequent Helm upgrades, so increasing these values here won’t take effect in an already-deployed namespace unless the AppDefinition resources are deleted first (or you run a clean install that removes them). If the intent is to change scaling for an existing test3 install, you may need to pair this change with an explicit AppDefinition purge/uninstall step.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,17 +42,20 @@ theia-cloud: | |
| interval: 3 | ||
|
|
||
| operator: | ||
| #image: ghcr.io/eduide/eduide-cloud/operator:latest | ||
| #eagerStart: false | ||
| image: ghcr.io/eduide/eduide-cloud/operator:latest-ba732d8 | ||
| # Production keeps Ceph RBD and does not configure sidecars. | ||
| # Disable eager-start so the operator stays on the workspace-backed RWO path | ||
| # instead of creating prewarmed RWX PVCs that do not fit csi-rbd-sc. | ||
| eagerStart: false | ||
|
||
| replicas: 3 | ||
|
Comment on lines
44
to
50
|
||
| sessionsPerUser: 10 | ||
| storageClassName: csi-rbd-sc | ||
|
|
||
| # service: | ||
| # image: ghcr.io/eduide/eduide-cloud/service:latest | ||
| # adminApiTokenSecret: | ||
| # name: service-admin-api-token | ||
| # key: ADMIN_API_TOKEN | ||
| service: | ||
| image: ghcr.io/eduide/eduide-cloud/service:latest-ba732d8 | ||
| adminApiTokenSecret: | ||
| name: service-admin-api-token | ||
| key: ADMIN_API_TOKEN | ||
|
|
||
| preloading: | ||
| images: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.