OSPOOL-158: Add NRP OSPool EP image builds by brianhlin · Pull Request #308 · opensciencegrid/images

brianhlin · 2026-04-17T22:58:11Z

I bet this will fail, at the very least because the Harbor user for pushing images probably doesn't have permissions to push to the osg-htc project.

Copilot

Pull request overview

Adds a new osg-htc/nrp-ospool-ep container image (and supporting init/entrypoint scripts) intended for NRP OSPool EP usage, and updates the GitHub Actions container build workflow to detect images under osg-htc/ in addition to opensciencegrid/.

Changes:

Introduces a new nrp-ospool-ep image (Dockerfile + build-config) with custom entrypoint behavior and HTCondor/pilot configuration scripts.
Adds wrapper scripts for singularity/apptainer to strip --pid/-p and adjusts runtime validation scripts.
Updates .github/workflows/build-containers.yml to include osg-htc/... image directories when building the image list.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`osg-htc/nrp-ospool-ep/Dockerfile`	Defines the new EP-derived image and wires in runtime/init scripts.
`osg-htc/nrp-ospool-ep/build-config.json`	Declares build matrix inputs (OS/series/repos) for the new image.
`osg-htc/nrp-ospool-ep/scripts/entrypoint.sh`	New entrypoint wrapper to run image-config hooks and supervise the pilot process.
`osg-htc/nrp-ospool-ep/scripts/check_master.sh`	Adds condor_master monitoring/diagnostics for termination/restart handling.
`osg-htc/nrp-ospool-ep/scripts/singularity_npid.sh`	Wrapper to drop `--pid/-p` and rewrite related flags.
`osg-htc/nrp-ospool-ep/scripts/apptainer_npid.sh`	Wrapper to drop `--pid/-p` and rewrite related flags.
`osg-htc/nrp-ospool-ep/scripts/01_token.sh`	Exposes the pilot token as an env var for osgvo-pilot.
`osg-htc/nrp-ospool-ep/scripts/01_no_condor_host.sh`	Unsets `CONDOR_HOST` to satisfy osgvo-pilot expectations.
`osg-htc/nrp-ospool-ep/scripts/02_validate_singularity.sh`	Adds a basic singularity execution validation at init time.
`osg-htc/nrp-ospool-ep/scripts/02_validate_apptainer.sh`	Adds a basic apptainer execution validation at init time.
`osg-htc/nrp-ospool-ep/scripts/11_set_OSGInstitutionID.sh`	Attempts to derive `OSG_INSTITUTION_ID` from k8s node labels.
`osg-htc/nrp-ospool-ep/scripts/19_set_resources.sh`	Sets advertised CPU/memory/disk/GPU slot/resource config.
`osg-htc/nrp-ospool-ep/scripts/20_advertise_glidein.sh`	Advertises glidein attribute(s) into the pilot config.
`osg-htc/nrp-ospool-ep/scripts/20_advertise_k8s_domain.sh`	Advertises k8s pod/domain/namespace/physical host into the pilot config.
`osg-htc/nrp-ospool-ep/scripts/21_advertise_k8s_provisioner.sh`	Advertises k8s provisioner name/type into the pilot config.
`osg-htc/nrp-ospool-ep/scripts/22_set_requirements.sh`	Adds matching/provisioning requirements expressions into the pilot config.
`.github/workflows/build-containers.yml`	Expands image discovery to include `osg-htc/...` directories.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T23:02:20Z

+  if [ $npids -gt 1 ]; then
+    echo "condor_master restrated" 1>&2
+    break
+  fi
+
+  if [ $nprocs -ne 1 ]; then
+    echo "condor_master not running" 1>&2
+    break
+  fi


check_master.sh computes nprocs once (line 25) but then uses it inside the monitoring loop (line 49) without recomputing it after each sleep. This means the loop may never detect a restarted/dead condor_master based on process state. Recompute the process check inside the loop (and consider using ps -p "$orgpid" or similar to avoid grep-based matching).

Copilot · 2026-04-17T23:02:21Z

+trap 'echo signal received!; kill $(jobs -p); wait' SIGINT SIGTERM
+
+export HOME=/pilot
+su osg -p -c "/usr/local/sbin/entrypoint.osg.sh $@" &


su ... -c "/usr/local/sbin/entrypoint.osg.sh $@" builds a shell command by interpolating the container's arguments into a string. This is fragile (breaks on spaces/quoting) and can allow shell metacharacters in args to be interpreted by the shell invoked by su -c. Prefer passing arguments without re-parsing (e.g., use su's ability to pass additional args to the shell, or switch to runuser/gosu/setpriv so you can exec the target with a proper "$@" argument vector).

Suggested change

su osg -p -c "/usr/local/sbin/entrypoint.osg.sh $@" &

su osg -p -s /bin/bash -c 'exec /usr/local/sbin/entrypoint.osg.sh "$@"' -- "$@" &

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…fixed condor config

…elease

brianhlin · 2026-08-25T18:45:58Z

I've removed the osg-htc push support since we added that in #317

Copilot

Pull request overview

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

Suppressed comments (11)

osg-htc/nrp-ospool-ep/scripts/check_master.sh:52

nprocs is only computed once (before the loop) but is used inside the monitoring loop, so the "condor_master not running" check never reflects current state. Recompute nprocs inside the loop (prefer ps -p to avoid matching the grep process itself).

  if [ $nprocs -ne 1 ]; then
    echo "condor_master not running" 1>&2
    break
  fi

osg-htc/nrp-ospool-ep/scripts/check_master.sh:19

Spelling: "restrated" -> "restarted".

  echo "condor_master restrated at first step" 1>&2

osg-htc/nrp-ospool-ep/scripts/check_master.sh:45

Spelling: "restrated" -> "restarted".

    echo "condor_master restrated" 1>&2

osg-htc/nrp-ospool-ep/scripts/entrypoint.sh:10

su -c "/usr/local/sbin/entrypoint.osg.sh $@" expands $@ in the current shell, which can break argument quoting and allow shell metacharacters in args to be re-interpreted by the su -c shell. Pass args via bash -c positional parameters instead.

su osg -p -c "/usr/local/sbin/entrypoint.osg.sh $@" &

osg-htc/nrp-ospool-ep/scripts/entrypoint.sh:13

Spelling/grammar in comment: "terminate oby itself" -> "terminate on its own".

# protection in case it does not terminate oby itself when condor dies or restarts

osg-htc/nrp-ospool-ep/scripts/check_master.sh:61

Typo in the StartLog path: /pilotl/log/StartLog will fail and hide useful diagnostics. This should be /pilot/log/StartLog.

tail -100 /pilotl/log/StartLog

osg-htc/nrp-ospool-ep/scripts/02_validate_apptainer.sh:14

Comment says "if singularity is present" but this script is validating apptainer.

  # only test for apptainer functionality if singularity is present
  # may not be in all pods

osg-htc/nrp-ospool-ep/scripts/01_token.sh:7

If the token file is missing/unreadable, this silently exports an empty TOKEN and failures happen later. Fail fast with a clear error and avoid backticks/UUOC.

export TOKEN=`cat /etc/condor/tokens.d/prp-wn.token`

osg-htc/nrp-ospool-ep/scripts/01_no_condor_host.sh:4

Spelling/grammar in comment: "does not like is CONDOR_HOST is set" -> "does not like it if CONDOR_HOST is set".

# osgvo pilot does not like is CONDOR_HOST is set

osg-htc/nrp-ospool-ep/Dockerfile:66

This repo’s Dockerfiles consistently use uppercase instructions (e.g., ENV ...). Using env here is inconsistent and can be confusing; switch to ENV for both variables.

env ACCEPT_JOBS_FOR_HOURS=24

# keep default idle time low, as we may over-provision certain kinds of resources
# but others may be waiting
env ACCEPT_IDLE_MINUTES=20

osg-htc/nrp-ospool-ep/Dockerfile:24

The Docker build downloads whatever kubectl version is current at build time and does not verify its integrity (checksum/signature). This is a supply-chain risk and makes builds non-reproducible; pin a version and verify the download (or install from a trusted package repo).

# Add kubectl, to be able to interact with the k8s cluster
RUN curl -L "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/amd64/kubectl" -o /usr/sbin/kubectl && \
    chmod u+x /usr/sbin/kubectl

+if [ "x${OSG_INSTITUTION_ID}" == "x" ]; then
+   OSG_INSTITUTION_ID=`/usr/sbin/kubectl get node ${PHYSICAL_HOSTNAME} -L nautilus.io/OSGInstitutionID | tail -1 | awk '{print $6}'`
+   if [ "x${OSG_INSTITUTION_ID}" != "x" ]; then
+      export OSG_INSTITUTION_ID
+   fi
+fi


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

matyasselmeci

Here's a couple of typos noticed by copilot that it didn't make suggestions for.

Co-authored-by: Matyas Selmeci <mselmeci@wisc.edu>

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

matyasselmeci · 2026-08-31T17:07:39Z

+
+# else do nothing, let Condor figure it out
+
+if [ -f "/usr/bin/apptainer" ]; then


Not sure why this check: the Dockerfile explicitly creates /usr/bin/apptainer, so this is always true.

matyasselmeci · 2026-08-31T17:07:53Z

+
+# else do nothing, let Condor figure it out
+
+if [ -f "/usr/bin/singularity" ]; then


Not sure why this check: the Dockerfile explicitly creates /usr/bin/singularity, so this is always true.

matyasselmeci · 2026-08-31T17:12:46Z

Note that this image depends on CVMFS because the apptainer/singularity checks (which must pass) use apptainer and singularity from OASIS, and the image from /cvmfs/singularity.opensciencegrid.org. That's fine, but we should probably make that check explicit to save us some debugging time if CVMFS fails.

matyasselmeci · 2026-08-31T17:35:49Z

One notable difference between this and the ospool-ep image is that in this image, if apptainer/singularity fail validation, the entire container aborts (as opposed to continuing without singularity/apptainer support).

brianhlin · 2026-08-31T21:44:38Z

One notable difference between this and the ospool-ep image is that in this image, if apptainer/singularity fail validation, the entire container aborts (as opposed to continuing without singularity/apptainer support).

@ashtongraves @djw8605 @jthiltges is running user payloads in apptainer a requirement on the NRP side?

Note that this image depends on CVMFS because the apptainer/singularity checks (which must pass) use apptainer and singularity from OASIS, and the image from /cvmfs/singularity.opensciencegrid.org. That's fine, but we should probably make that check explicit to save us some debugging time if CVMFS fails.

We should probably just change this since HTCondor explicitly packages apptainer now. We may as well rely on that since there's a bunch of testing that goes into version compat upstream.

ashtongraves · 2026-09-01T15:03:47Z

@ashtongraves @djw8605 @jthiltges is running user payloads in apptainer a requirement on the NRP side?

No not a requirement on NRP side, moreso OSG side. We're currently running a modified version of the EP image that lets it fail on the apptainer check and run without apptainer while we're waiting.

brianhlin · 2026-09-03T16:47:03Z

@ashtongraves

No not a requirement on NRP side, moreso OSG side. We're currently running a modified version of the EP image that lets it fail on the apptainer check and run without apptainer while we're waiting.

Hrm, we don't have that requirement in the base OSPool images so I think we can just rip it out! @matyasselmeci if this looks good enough, then I think we should just merge this and rip out the failure in a follow-on PR

matyasselmeci · 2026-09-03T20:51:58Z

How do you feel about adding a REQUIRE_APPTAINER env knob, setting it to "false" in the standard ospool-ep containers but "true" in this one (but letting NRP folks turn it off until the CVE is fixed)? I'd like to be able to have pilots "fail fast" in case container support breaks (which was the NRP behavior until this CVE).

brianhlin · 2026-09-03T21:21:42Z

@matyasselmeci sounds good to me but I think that means that step 1 is still merging this PR unless there are other blockers?

matyasselmeci · 2026-09-03T21:23:18Z

Right, I think this is OK to merge. We can save some of my other comments for subsequent PRs.

matyasselmeci

Approving this; we'll fix remaining issues in separate PRs.

brianhlin requested a review from a team as a code owner April 17, 2026 22:58

Copilot AI review requested due to automatic review settings April 17, 2026 22:58

Copilot started reviewing on behalf of brianhlin April 17, 2026 22:58 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

brianhlin force-pushed the OSPOOL-158.nrp-ospool-ep branch 2 times, most recently from 34374da to f09cdf3 Compare April 17, 2026 23:16

brianhlin requested a review from Copilot April 17, 2026 23:30

Copilot started reviewing on behalf of brianhlin April 17, 2026 23:30 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

This was referenced Jul 28, 2026

Add osg-htc Harbor project support #316

Closed

Add osg-htc Harbor project support #317

Merged

sfiligoi added 19 commits August 25, 2026 13:40

Copy files from prp-osgvo-pilot/image

8edff67

Integrate wn/image/image-config.d/02_additional_requirements.sh with …

dcced00

…fixed condor config

Add scripts/21_advertise_k8s_provisioner.sh

69be689

Add FORCE_K8SNAMESPACE_MATCHING

74ce469

Add scripts/19_set_resources.sh

79b005d

Fix typo

58e6e0d

Add scripts/01_token.sh

e49e9c6

Fix script location

c2847c1

Fix typo in 22_set_requirements.sh

d3cfaa4

Add scripts/01_no_condor_host.sh

8bfba7f

Add scripts/check_master.sh

d5465d0

Fix typo

9ab0361

Add 02_validate_singularity.sh

1cbab6a

Fix return code

b9bef53

Wrap apptainer, too

f7b25ac

Test also --nv in singularity

85d06f4

Test also --nv in apptainer

62f5713

Change entrypoint location as per upstream

9f289cf

Switch from osgvo-docker-pilot:3.6-release to osgvo-docker-pilot:23-r…

4f9f5ea

…elease

brianhlin force-pushed the OSPOOL-158.nrp-ospool-ep branch from f09cdf3 to 7311aae Compare August 25, 2026 18:40

brianhlin requested a lite review from Copilot August 25, 2026 18:46

Copilot started reviewing on behalf of brianhlin August 25, 2026 18:47 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

brianhlin requested a review from mwestphall August 26, 2026 20:38

matyasselmeci and others added 6 commits August 31, 2026 09:49

Apply suggestion from @Copilot

891b07f

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

Typo fix from @Copilot

e99e315

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

Typo fix from @Copilot

30bdd54

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

Fix syntax error noticed by @Copilot

ddfdfa8

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

Apply suggestion from @Copilot

38ac2b4

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

Pin kubectl download (from @Copilot)

005bbd2

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

matyasselmeci reviewed Aug 31, 2026

View reviewed changes

Comment thread osg-htc/nrp-ospool-ep/scripts/01_no_condor_host.sh Outdated

Comment thread osg-htc/nrp-ospool-ep/scripts/check_master.sh Outdated

Comment thread osg-htc/nrp-ospool-ep/scripts/check_master.sh Outdated

Comment thread osg-htc/nrp-ospool-ep/Dockerfile Outdated

matyasselmeci and others added 2 commits August 31, 2026 10:18

Apply batched suggestions from code review

68e3fe1

Co-authored-by: Matyas Selmeci <mselmeci@wisc.edu>

Apply suggestion from @Copilot

0767c2c

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

matyasselmeci reviewed Aug 31, 2026

View reviewed changes

Comment thread osg-htc/nrp-ospool-ep/scripts/01_token.sh Outdated

Avoid accidentally printing out the token

de8c513

matyasselmeci reviewed Aug 31, 2026

View reviewed changes

matyasselmeci approved these changes Sep 3, 2026

View reviewed changes

matyasselmeci merged commit 1174698 into opensciencegrid:main Sep 3, 2026
5 checks passed

brianhlin deleted the OSPOOL-158.nrp-ospool-ep branch September 3, 2026 21:24

	su osg -p -c "/usr/local/sbin/entrypoint.osg.sh $@" &
	su osg -p -s /bin/bash -c 'exec /usr/local/sbin/entrypoint.osg.sh "$@"' -- "$@" &


		# else do nothing, let Condor figure it out

		if [ -f "/usr/bin/apptainer" ]; then


		# else do nothing, let Condor figure it out

		if [ -f "/usr/bin/singularity" ]; then

Uh oh!

Conversation

brianhlin commented Apr 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brianhlin commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matyasselmeci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matyasselmeci Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

matyasselmeci Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

matyasselmeci commented Aug 31, 2026

Uh oh!

matyasselmeci commented Aug 31, 2026

Uh oh!

brianhlin commented Aug 31, 2026

Uh oh!

ashtongraves commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brianhlin commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matyasselmeci commented Sep 3, 2026

Uh oh!

brianhlin commented Sep 3, 2026

Uh oh!

matyasselmeci commented Sep 3, 2026

Uh oh!

matyasselmeci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

ashtongraves commented Sep 1, 2026 •

edited

Loading

brianhlin commented Sep 3, 2026 •

edited

Loading