-
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 58 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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -43,8 +43,8 @@ theia-cloud: | |||||
| interval: 3 | ||||||
|
|
||||||
| operator: | ||||||
| image: ghcr.io/eduide/eduide-cloud/operator:pr-70 | ||||||
| #eagerStart: false | ||||||
| image: ghcr.io/eduide/eduide-cloud/operator:latest | ||||||
| eagerStart: true | ||||||
| replicas: 1 | ||||||
| sessionsPerUser: 10 | ||||||
| # 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 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.
theia-cloud.preloading.images no longer matches the documented fixed index layout (0=landing page, 1–10 IDE images, 11=oauth2-proxy) that deploy-theia.yml relies on via --set theia-cloud.preloading.images[N]=.... With the current shortened list starting at java-17, tag overrides can end up writing sparse arrays / wrong indices and break preloading expectations. Restore the full indexed list (including landing page at index 0 and oauth2-proxy at index 11), or update the workflow and the in-file comment together so they remain consistent.
Copilot
AI
Mar 20, 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 the only intended change is disabling Keycloak for the test3 deployment while keeping other values unchanged, but this hunk also changes multiple preloading image tags from a pinned PR tag to latest. Either update the PR description/title to reflect the broader scope, or split the image/tag changes into a separate PR to keep the benchmark/auth change isolated.
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.
theia-cloud.preloading.images no longer includes the landing page at index 0 (and the list is shorter than the workflow’s expected 0–10/11 layout). This will misalign .github/workflows/deploy-theia.yml --set theia-cloud.preloading.images[0..10]=... overrides when tags are provided, causing the wrong images to be overridden/preloaded. Restore the full indexed list (landing at [0], IDE/no-ls/langserver at [1..10], and either keep oauth2-proxy at [11] or update the surrounding comments + workflow consistently).
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.
Outdated
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.
Bumping minInstances from 1 to 5 (and adding maxInstances: 20) for app definitions will increase baseline resource usage and can change how quickly/consistently workspaces start (especially relevant for benchmark runs). Confirm these scaling defaults are intentional for test3; if not, revert to the previous values or document why the higher prewarm is required.
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 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.
minInstances is increased from 1 to 5 (and maxInstances added at 20) for multiple app definitions. This can significantly increase baseline resource usage in the test3 namespace. If the goal is only benchmark runs with unauthenticated access, consider keeping scaling unchanged here (or justify these new scaling defaults and confirm the cluster has capacity).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,14 +42,17 @@ theia-cloud: | |
| interval: 3 | ||
|
|
||
| operator: | ||
| image: ghcr.io/eduide/eduide-cloud/operator:latest-dfe0d09 | ||
| #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-dfe0d09 | ||
| image: ghcr.io/eduide/eduide-cloud/service:latest-ba732d8 | ||
|
||
| adminApiTokenSecret: | ||
| name: service-admin-api-token | ||
| key: ADMIN_API_TOKEN | ||
|
|
||
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 test3 values should remain unchanged except disabling Keycloak, but this file also updates operator/service image tags, the preloading image set, and appdefinitions (defaultImageTag/maxInstances/new app). Either update the PR description to match the actual scope or split these unrelated changes into separate PRs.