Skip to content

Implement k8s service labels#227

Merged
gilesknap merged 1 commit into
mainfrom
Implement-k8s-service-labels
Mar 27, 2026
Merged

Implement k8s service labels#227
gilesknap merged 1 commit into
mainfrom
Implement-k8s-service-labels

Conversation

@OCopping
Copy link
Copy Markdown
Contributor

@OCopping OCopping commented Jan 28, 2026

This will display a pod device label in ec monitor if one exists, otherwise it will show default "service" label.
It also bumps textual to be >= v7.4.0

image

@OCopping OCopping requested a review from gilesknap January 28, 2026 14:50
@OCopping OCopping changed the base branch from main to update-copier-template January 28, 2026 15:03
@OCopping OCopping force-pushed the Implement-k8s-service-labels branch 2 times, most recently from 70c57f3 to 7e89ab7 Compare January 28, 2026 15:37
Copy link
Copy Markdown
Member

@gilesknap gilesknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions, I've not looked at the code yet.

Does this affect ec ps as well as ec monitor?

What is the intention of having device, the IOC name should (sort of) tell you what the device is.

@OCopping
Copy link
Copy Markdown
Contributor Author

OCopping commented Jan 29, 2026

Does this affect ec ps as well as ec monitor?

Yes it does, so there will be a new label column between name and version.

✗ ec -b DEMO ps     
***RUNNING IN DEMO MODE***
| name       | label          | version | ready | deployed             |
|------------|----------------|---------|-------|----------------------|
| demo-ea-00 | demo device 00 | 1.0.25  | true  | 2024-10-22T11:23:08Z |
| demo-ea-01 | demo device 01 | 1.0.24  | true  | 2024-10-22T11:23:03Z |
| demo-ea-02 | demo device 02 | 1.0.23  | true  | 2024-10-22T11:23:04Z |
| demo-ea-03 | demo device 03 | 1.0.22  | true  | 2024-10-22T11:23:07Z |
| demo-ea-04 | demo device 04 | 1.0.21  | true  | 2024-10-22T11:23:01Z |
| demo-ea-05 | demo device 05 | 1.0.20  | true  | 2024-10-22T11:23:03Z |
| demo-ea-06 | demo device 06 | 1.0.19  | true  | 2024-10-22T11:23:07Z |
| demo-ea-07 | demo device 07 | 1.0.18  | true  | 2024-10-22T11:23:01Z |

What is the intention of having device, the IOC name should (sort of) tell you what the device is.

The scientists on i19 like having a more descriptive label they can just glance at to see what the IOC is for, similar to what was in EDM (although that was more required due to generic IOC names).
I do admit it seems a bit redundant, but the scientists requested something like this 🙃

EDIT: I should add, it would be quite easy to remove from ec ps if you are not a fan of it there.

@OCopping OCopping changed the base branch from update-copier-template to main January 30, 2026 09:15
@gilesknap
Copy link
Copy Markdown
Member

Lets get this implemented as per our corridor discussion in the up-coming epics-containers sprint.

@gilesknap
Copy link
Copy Markdown
Member

Do you think this is ready to merge?

I'm a little concerned that this is a breaking change for any beamline not yet on the new version of schema for deployment apps values. (and that version is not even released yet at the time I'm writing this)

We have to work out how to roll this out.

@gilesknap
Copy link
Copy Markdown
Member

I just tried this but it should now take a 'description' for ec deploy so we can write the label from the CLI.

@OCopping OCopping force-pushed the Implement-k8s-service-labels branch from 6a283d7 to 65cf66c Compare March 4, 2026 08:58
@OCopping OCopping requested a review from gilesknap March 11, 2026 09:52
Copy link
Copy Markdown
Member

@gilesknap gilesknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to test this - but I guess I need to deploy the chart changes to a beamline - is p47 in the correct state for me to try the new ec?

@OCopping
Copy link
Copy Markdown
Contributor Author

I'd like to test this - but I guess I need to deploy the chart changes to a beamline - is p47 in the correct state for me to try the new ec?

Yes, I have been testing by deploying bl47p-ea-dcam-01 already

@OCopping OCopping requested a review from gilesknap March 18, 2026 16:04
@gilesknap
Copy link
Copy Markdown
Member

This has the appearance of not working - see below.

I updated both p47-services and deployment to the ec-helm-charts 5.3.1-beta.10. The new description label has been added to the deployment values.yaml. But it has not propogated to the IOC statefulset annotations.

hgv27681@pc0116: /scratch/hgv27681/work/edge-containers-cli Implement-k8s-service-labels
$ ec deploy bl47p-c7-sim-01 main a-silly-simulator
Deploy bl47p-c7-sim-01 version main to target p47-beamline/p47 with description a-silly-simulator
Are you sure? [y/N]: y
(edge-containers-cli)  
hgv27681@pc0116: /scratch/hgv27681/work/edge-containers-cli Implement-k8s-service-labels
$ ec ps                                          
╭────────────────────┬─────────┬─────────────────┬───────┬──────────────────────╮
│ name                label    version          ready  deployed             │
├────────────────────┼─────────┼─────────────────┼───────┼──────────────────────┤
│ bl47p-c7-sim-01    │ service │ main            │ True  │ 2026-03-05T11:19:57Z │
│ bl47p-ea-dcam-01   │ service │ main            │ True  │ 2026-03-10T12:06:10Z │
│ bl47p-ea-dcam-02   │ service │ main            │ True  │ 2026-03-05T11:19:58Z │
│ bl47p-ea-fastcs-01 │ service │ main            │ False │ 2026-03-05T11:19:58Z │
│ bl47p-ea-panda-01  │ service │ main            │ True  │ 2026-03-05T11:19:57Z │
│ bl47p-mo-ioc-01    │ service │ pingwait        │ True  │ 2026-03-10T12:07:34Z │
│ bl47p-synoptic     │ service │ fix-commit-hash │ True  │ 2026-03-05T11:19:58Z │
│ p47-blueapi        │ service │ main            │ True  │ 2026-03-11T14:03:47Z │
│ p47-blueapi        │ service │ main            │ True  │ 2026-03-11T14:03:47Z │
│ p47-epics-gateways │ service │ main            │ True  │ 2026-03-05T11:19:58Z │
│ p47-epics-opis     │ service │ main            │ True  │ 2026-03-05T11:19:58Z │
│ p47-epics-pvcs     │ service │ main            │ True  │ 2026-03-05T11:19:58Z │
│ p47-ioc-monitor    │ service │ main            │ True  │ 2026-03-05T11:19:57Z │
│ p47-proxy          │ service │ main            │ True  │ 2026-03-05T11:19:58Z │
│ p47-rabbitmq       │ service │ main            │ True  │ 2026-03-05T11:19:59Z │
│ p47-tiled          │ service │ main            │ True  │ 2026-03-11T13:56:58Z │
│ p47-tiled          │ service │ main            │ True  │ 2026-03-11T13:56:58Z │
│ p47-tiled          │ service │ main            │ True  │ 2026-03-11T13:56:58Z │
│ p47-tiled          │ service │ main            │ True  │ 2026-03-11T13:56:58Z │
╰────────────────────┴─────────┴─────────────────┴───────┴──────────────────────╯
(edge-containers-cli)

@gilesknap
Copy link
Copy Markdown
Member

@OCopping I would prefer the description to be an option instead of optional argument. e.g.

ec deploy my-ioc main --desc "my new label"

Also - I tried a label with spaces and that was allowed. I understand that this is illegal in the annotations so we should do a regex check at the CLI level too.

@OCopping
Copy link
Copy Markdown
Contributor Author

OCopping commented Mar 23, 2026

@gilesknap I'm not sure what's wrong, as I just pushed a new --desc and it worked... See the "whats-wrong" label:

❯ uv run ec deploy -y bl47p-ea-dcam-01 main --desc whats-wrong                                                                                                                                                                                                  08:45:31
Deploy bl47p-ea-dcam-01 version main to target p47-beamline/p47 with description whats-wrong
❯ uv run ec ps                                                                                                                                                                                                                                             7s  08:45:51
╭────────────────────┬─────────────┬─────────────────┬───────┬──────────────────────╮
│ name               │ label       │ version         │ ready │ deployed             │
├────────────────────┼─────────────┼─────────────────┼───────┼──────────────────────┤
│ bl47p-c7-sim-01    │ service     │ main            │ True  │ 2026-03-20T12:55:06Z │
│ bl47p-ea-dcam-01   │ whats-wrong │ main            │ False │ 2026-03-10T12:06:10Z │
│ bl47p-ea-dcam-02   │ service     │ main            │ True  │ 2026-03-05T11:19:58Z │
│ bl47p-ea-fastcs-01 │ service     │ main            │ False │ 2026-03-05T11:19:58Z │
│ bl47p-ea-panda-01  │ service     │ main            │ True  │ 2026-03-05T11:19:57Z │
│ bl47p-mo-ioc-01    │ service     │ pingwait        │ True  │ 2026-03-10T12:07:34Z │
│ bl47p-synoptic     │ service     │ fix-commit-hash │ True  │ 2026-03-05T11:19:58Z │
│ p47-blueapi        │ service     │ main            │ True  │ 2026-03-11T14:03:47Z │
│ p47-blueapi        │ service     │ main            │ True  │ 2026-03-11T14:03:47Z │
│ p47-epics-gateways │ service     │ main            │ True  │ 2026-03-05T11:19:58Z │
│ p47-epics-opis     │ service     │ main            │ True  │ 2026-03-05T11:19:58Z │
│ p47-epics-pvcs     │ service     │ main            │ True  │ 2026-03-05T11:19:58Z │
│ p47-ioc-monitor    │ service     │ main            │ True  │ 2026-03-05T11:19:57Z │
│ p47-proxy          │ service     │ main            │ True  │ 2026-03-05T11:19:58Z │
│ p47-rabbitmq       │ service     │ main            │ True  │ 2026-03-05T11:19:59Z │
│ p47-tiled          │ service     │ main            │ True  │ 2026-03-11T13:56:58Z │
│ p47-tiled          │ service     │ main            │ True  │ 2026-03-11T13:56:58Z │
│ p47-tiled          │ service     │ main            │ True  │ 2026-03-11T13:56:58Z │
│ p47-tiled          │ service     │ main            │ True  │ 2026-03-11T13:56:58Z │
╰────────────────────┴─────────────┴─────────────────┴───────┴──────────────────────╯

Copy link
Copy Markdown
Member

@gilesknap gilesknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review (Claude-assisted)

I've tested this and it works well. I did notice that it does not work for dev-c7 IOCs which is to be expected and I think we don't fix that as it incentive to ibekify your IOCs!

I noticed a couple of issues and Claude picked up on a couple more - see below. (probably don't bother with K8S paths unless its easy to fix)

The following review comments were generated with Claude Code assistance.

Issues

1. Description validation regex/message mismatch (cli.py:29)
The error message says "Only the symbols '-', '_' and '.' are allowed" but the regex ^[a-zA-Z0-9](?:(?!--)[a-zA-Z0-9-]){0,62}$ only permits hyphens — not underscores or dots. Either update the regex or fix the message.

2. _check_description redundant name check (argo_commands.py:218-228)
_check_description gets the manifest via _get_service_manifest (which already validates the service exists), then checks resource_name == service_name again. If the name doesn't match, it silently returns None. This check is either redundant or should raise an error.

3. _get_service_manifest doesn't verify metadata.name (argo_commands.py:197-202)
The refactored method returns the first StatefulSet/Deployment manifest it finds without verifying metadata.name == service_name. The old _check_stoppable had this check. If there are multiple StatefulSets in an app's manifests, this could return the wrong one.

4. Double manifest fetch in deploy flow (argo_commands.py:159-161)
When description is None, _check_description fetches manifests, and _check_stoppable (if called later) does the same. The test data yaml even has a comment: "Not sure why this is called twice". Consider caching or passing the manifest through.

5. Demo data inconsistent with validation (demo_commands.py:34)
Demo labels are "demo device 00" etc. (with spaces), but _check_description in cli.py only allows kebab-case characters. The demo data wouldn't pass the CLI's own validation.

6. K8S deploy has no description fallback (k8s_commands.py)
The K8S deploy method passes description through to Helm without any _check_description equivalent — unlike the ArgoCD path, there's no fallback to fetch the current description from the cluster when --desc is omitted.

Minor nits

  • argo_commands.py:342.to_list() addition is a good fix (was comparing against a polars Series).
  • The confirm_callback type signature change from Callable[[str], None] to Callable[[str, str | None], None] is a breaking change for any external callers of deploy().
  • monitor.py:271self.columns.remove("label") mutates the DataFrame's column list in-place. Safer to use a copy: self.columns = [c for c in iocs_df.columns if c != "label"].
  • Test assertions on exact Rich table box-drawing output (test_argocd.py, test_demo.py, test_k8s.py) are fragile — they'll break if Rich changes rendering or terminal width detection differs.

gilesknap added a commit that referenced this pull request Mar 27, 2026
- Fix description validation error message to match regex (hyphens only,
  not underscores/dots)
- Add metadata.name check in _get_service_manifest to ensure correct
  manifest is returned
- Remove redundant name checks in _check_stoppable and _check_description
  (now guaranteed by _get_service_manifest)
- Fix demo labels to use kebab-case ("demo-device-00" not "demo device 00")
  to be consistent with CLI validation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix description validation error message to match regex (hyphens only,
  not underscores/dots)
- Add metadata.name check in _get_service_manifest to ensure correct
  manifest is returned
- Remove redundant name checks in _check_stoppable and _check_description
  (now guaranteed by _get_service_manifest)
- Fix demo labels to use kebab-case ("demo-device-00" not "demo device 00")
  to be consistent with CLI validation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@gilesknap gilesknap force-pushed the Implement-k8s-service-labels branch from 708fcb8 to a13b2b2 Compare March 27, 2026 14:15
@gilesknap gilesknap merged commit c834fed into main Mar 27, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants