Add features to make iPython console work in cluster by gilesknap · Pull Request #210 · DiamondLightSource/FastCS

gilesknap · 2025-09-12T11:07:44Z

mount in the standard ioc-instance PVCS including runtime
use runtime as a writeable home folder
add ability to set environment variables as per ioc-instance
set a default env vars HOME and TERM

Tested and fully working against this: https://github.com/epics-containers/p45-services/blob/legacy-p45-beamline/services/bl45p-ea-fastcs-01. Includes use of stdio-socket to expose to allow iPython console access.

Copilot

Pull Request Overview

This PR adds iPython console support for cluster environments by enhancing FastCS Helm charts with environment variable configuration and additional persistent volume mounts. The changes enable containers to have a writable home directory and proper environment setup for interactive console access.

Adds globalEnv and iocEnv configuration options for setting environment variables across all IOCs or individual IOCs
Mounts standard ioc-instance persistent volume claims (runtime, autosave) and configures a writable home directory
Sets default HOME and TERM environment variables for proper console functionality

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
Charts/fastcs/values.yaml	Adds globalEnv and iocEnv array configurations for environment variables
Charts/fastcs/values.schema.json	Defines JSON schema validation for the new environment variable arrays
Charts/fastcs/templates/statefulset.yaml	Implements environment variables, working directory, and additional volume mounts in container specs
Charts/fastcs/extra.values.yaml	Provides example configurations for the new environment variable arrays

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Charts/fastcs/templates/statefulset.yaml

codecov · 2025-09-12T11:09:08Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.34%. Comparing base (9c2042a) to head (a1991c4).
⚠️ Report is 60 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #210   +/-   ##
=======================================
  Coverage   92.34%   92.34%           
=======================================
  Files          40       40           
  Lines        2077     2077           
=======================================
  Hits         1918     1918           
  Misses        159      159

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gilesknap

accepting CoPilot spelling fixes

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

GDYendell · 2025-09-12T12:11:24Z

Charts/fastcs/templates/statefulset.yaml

+              value: /epics/runtime
+            - name: TERM
+              value: xterm-256color
+            {{- with  $.Values.globalEnv }}


Is the idea here that you can have a global env and add a service env without overwriting the global one? Would this be an appropriate case to use global - i.e. put $.Values.global.env here? Then we can add global.env in the root services yaml to insert things here and don't need an extra value in this schema.

Yes that is the idea as you suggest.

We could move globalEnv into global but I don't have much appetite for it as its a breaking change for any services repos that have used it already and its not compelling enough to justify those breaks.

So ibek already has globalEnv?

No ioc-instance helm chart does. It is intended to go in the shared values file so that all services (whose helm charts reference gloablEnv) will get these environment variables.

OK. I think this is fine if it would be a big breaking change. It does seem ".Values.global.env" is a common pattern in the helm community, so it would be nice to align with that. But at the same time it might not be worth the upheaval.

Charts/fastcs/templates/statefulset.yaml

GDYendell · 2025-09-12T12:24:59Z

Charts/fastcs/templates/statefulset.yaml

            {{- toYaml . | nindent 12 }}
          {{- end }}
+            - name: runtime-volume
+              mountPath: /epics/runtime


I don't think it makes sense to have /epics/runtime in fastcs. It has a very specific use in ibek IOCs that doesn't apply to fastcs IOCs (generating what are normally static files for conventional IOCs to use at boot time).

Can we just use /tmp as HOME?

Using runtime means that your CLI history is saved between reboots. tmp would be lost.

We could call it something different, but its just where all IOCs put the things they make that are not autosave files or data.

If you don't like the name of the PVC then we have to go create a new PVC - which is not horrible. But right now we have the epics-pvcs pod to go inspect all the folders and make adjustments to them if needed.

None of my reasons are super compelling so if you prefer /tmp then lets go with that. I don't feel like it is a nicer name for a home folder than /runtime though!

Persistent history would be kind of nice, so don't mind too much making a directory other than /tmp. It is just the epics bit that I don't like.

So we can mount it into the container anywhere and call it what we want.

BUT. The PVC is called ixx-runtime and is mounted into all ibek IOCS (using the ioc-instance helm chart) at /epics/runtime. So options are:

make a completely new PVC just for fastCS IOCs and manage it separately so we can call it a better name and mount it where you want.

mount the existing epics-pvcs/my-ioc-name into /home/ipython_user

or just be consistent with ioc-instance and mount it into /epics/runtime so that developers jumping between ioc-instance and fastcs helm charts don't have to think about it.

I went for the last option. In my head the idea that /epics/runtime is a scratch area for EPICS IOCs is not awful, but I get that fastCS things might not be EPICS always.

I think there is perhaps some friction over the fact that I made this helm chart to work with epics-containers and service repositories. Whereas fastcs itself is more generic than that.

Pragmatically, the chart will be used in service repositories and exist along side ioc-instance helm chart for ibek IOCs.
There are some PVCs that are global to each services domain and I've re-used them here. If we don't want to do that then we create a bit of extra work maintaining additional PVCs. I like the consistency but I'm coming from a epics-containers centric viewpoint.

Yeah that is fair. To be honest I am leaning towards /tmp just to fix the warning and allow up arrow once to tweak and re-run something, which realistically I think is all anyone will do 90% of the time anyway. I don't really like that it would pollute /epics/runtime, as a useful place to inspect what ibek IOCs generated, with something completely unrelated and its not worth making a new PVC for.

GDYendell · 2025-09-12T12:27:52Z

Charts/fastcs/templates/statefulset.yaml

+            {{- with  $.Values.globalEnv }}
+            {{-  toYaml . | nindent 12}}
+            {{- end }}
+            {{-  with $.Values.iocEnv }}


I would prefer just env here.

again, I'm trying to be consistent with all IOCs here for familiarity.

The bad choices made early in epics-containers are only worth fixing if the benefit outweighs the breaking changes.
Is it worth it? maybe, I'm reluctant to make changes which will break existing services repos now that we have so many of them. I could grep them all to see if they are using iocEnv and maybe none are.

Do we care about fastCS being consistent with ibek IOCs? dunno.

It is just that it isn't necessarily an IOC. Does ibek already have iocEnv?

Yes ioc-instance (ibek IOCs) has iocEnv. We could change this one because it is never intended to be used in the shared values file, only in the individual values files.

I think it makes sense to call this env but also need to concede that use of the path /epics/ should also change in that case.

Charts/fastcs/templates/statefulset.yaml

gilesknap

@GDYendell I responded to your comments, let me know what you think.

gilesknap

Having read your comments and thought this through I think we should:

drop the autosave mount
for the home dir mount epics-pvcs/runtime/my-ioc-name into /home/ipython
change the iocEnv value to env

I think that is accepting most of your points with the caveat that the PVC name still has epics in it. Changing the pvc names would be a big breaking change, adding a new PVC would be extra maintenance - @GDYendell let me know how you feel about this!

GDYendell · 2025-09-19T07:54:09Z

Charts/fastcs/templates/statefulset.yaml

+            {{- with  $.Values.env }}
+            {{-  toYaml . | nindent 12}}
+            {{- end }}
+            {{-  with $.Values.global.env }}
+            {{-  toYaml . | nindent 12}}
+            {{- end }}


What is the precedence here if a variable appears in both? env should override global.env

This is a good point, I think in fact the schema will fail for having the same key twice in a dictionary.

This means some exciting dictionary mergining in GoLang templates! blurg!

GDYendell · 2025-09-19T08:01:10Z

Charts/fastcs/templates/pvcs.yaml

-{{- $location := default .Values.global.location .Values.location | required "ERROR - You must supply location or global.location" -}}
-{{- $ioc_group := default .Values.global.ioc_group .Values.ioc_group | required "ERROR - You must supply ioc_group or global.ioc_group" -}}
+{{- $location := default .Values.global.location .Values.location | default "" -}}
+{{- $ioc_group := default .Values.global.ioc_group .Values.ioc_group | default "" -}}


Maybe we could set is_ioc to true here if ioc_group is not empty, so that it doesn't have to be a value? And so that we can't have ioc_group=something with is_ioc=false, which would probably do bad things.

This sounds sensible to me.

gilesknap · 2025-09-19T08:27:21Z

Hi @GDYendell I have made a bunch of changes to genericize the helm chart but I'm not happy with them.
AND I have realized that there is lots of work to do with services - as a fastcs deployment should choose the set of ports it exposes bases upon its transports.

The main reason I'm not happy is that the chart now looks a bit like ioc-instance but with loads of odd caveats in it.

I've been chatting to Tom this morning and I think we need to make some fundamental breaking changes to the epics-containers framework to make it easier to add in a chart like this.

I'm going to write up what I think is needed and discuss it with you and Tom on Monday.

In the meantime I've switched this PR to draft as I think the chart will have lots of changes once the epics-containers changes are done.

gilesknap added 4 commits September 12, 2025 10:35

add control of environment variables to the helm chart

e0c4801

add in runtime folder and HOME

6dded26

add in the runtime and autosave mounts

cf733c9

fix env and workingDir for main container

5a6fd94

gilesknap requested review from GDYendell and Copilot September 12, 2025 11:08

Copilot AI reviewed Sep 12, 2025

View reviewed changes

Charts/fastcs/templates/statefulset.yaml Outdated Show resolved Hide resolved

Charts/fastcs/templates/statefulset.yaml Outdated Show resolved Hide resolved

Charts/fastcs/templates/statefulset.yaml Show resolved Hide resolved

gilesknap commented Sep 12, 2025

View reviewed changes

Apply spelling suggestions from coPilot code review

f4b0a48

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

GDYendell reviewed Sep 12, 2025

View reviewed changes

gilesknap commented Sep 17, 2025

View reviewed changes

gilesknap commented Sep 18, 2025

View reviewed changes

gilesknap added 3 commits September 19, 2025 07:49

genericize chart, removing references to EPICS, IOC and epics-containers

1752a8c

fix opi folder mounts

6464a7c

remove ioc specifics from pvc.yaml

a1991c4

gilesknap temporarily deployed to release September 19, 2025 07:50 — with GitHub Actions Inactive

GDYendell reviewed Sep 19, 2025

View reviewed changes

gilesknap marked this pull request as draft September 19, 2025 08:21

GDYendell closed this Nov 11, 2025

GDYendell deleted the fix-chart branch December 9, 2025 12:11

Conversation

gilesknap commented Sep 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gilesknap left a comment

Choose a reason for hiding this comment

Uh oh!

GDYendell Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gilesknap Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

gilesknap left a comment

Choose a reason for hiding this comment

Uh oh!

gilesknap 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!

gilesknap commented Sep 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Sep 12, 2025 •

edited

Loading

GDYendell Sep 12, 2025 •

edited

Loading

gilesknap Sep 18, 2025 •

edited

Loading