Proposed version 0.2.0#2
Conversation
…gmap-pelican.yaml Since both of these files contain Pelican configuration, splitting them up hurts the organization because the information is not in one place. Co-authored-by: Copilot <copilot@github.com>
* Use chart's appVersion as the default Pelican image tag. * Remove `app: pelican-cache` label - Helm's own labels are sufficient. * Remove deprecated `Cache.DataLocation` var. * Remove the config-dir-placeholder; in current images, it is unnecessary. * Avoid having an empty Cache block if none of the cache options are customized. * Rename the "cache-data" volume to just "data" to avoid having a "pelican-cache-cache-data" name when the prefixes are applied. * Add a note explaining why we can't mount the TLS cert/key at Pelican's default location. * Change UIAdminUsers to a YAML list instead of space-separated values. * Remove 'federation' from selector labels because selector labels are immutable and prevent chart upgrades in case the federation changes. Co-authored-by: Copilot <copilot@github.com>
…ificate. certificate.dnsNames will be a list of additional SANs instead. This avoids an admission webhook error with the default values when only serverHostname is specified. Co-authored-by: Copilot <copilot@github.com>
Do not specify IssuerKey in configmap-pelican.yaml. Instead, if the issuer key is of type "existingSecret", mount the key at "/etc/pelican/issuer-keys/issuer.pem". If the type is "pvc", mount it at "/etc/pelican/issuer-keys". Co-authored-by: Copilot <copilot@github.com>
…delete PVCs on uninstall Co-authored-by: Copilot <copilot@github.com>
The two federations we want this check for are osdf and osdf-itb, with https://osg-htc.org and https://osdf-itb.osg-htc.org as their discoveryUrls, respectively. Co-authored-by: Copilot <copilot@github.com>
* Specify the type of storage for a cache by `cache.type`;
if `cache.type==pvc`, then the specifics go under `cache.pvc`;
if `cache.type==hostPath`, then the specifics go under `cache.hostPath`.
* Rename storageClassName to storageClass since that's more common.
* Make sure specifying volumes are consistent between the `cache` section,
`logging` section and `lotman` section.
* Rename "namespaceKey" to "issuerKey" and the default secret key name to
"private-key.pem"; "issuer key" is the terminology Pelican uses and
"namespace key" doesn't make much sense for a cache; private-key.pem is the
name of the file that `pelican key create` creates.
* Add to NOTES.txt a list of all the PVCs we created; also mention that we
do not delete them on uninstall.
* Add validation for PVCs and volumes.
* If `cache.type==pvc`, require `cache.pvc.existingClaim` or
`cache.pvc.storageClass`.
* If `cache.type==hostPath`, require `cache.hostPath.path`.
* If `issuerKey.type==pvc`, require `issuerKey.pvc.storageClass`.
* If `issuerKey.type==existingSecret`, require
`issuerKey.existingSecret`.
* If lotman is enabled, require `lotman.pvc.existingClaim` or
`lotman.pvc.storageClass`.
* If oidc is enabled, require `oidc.existingSecret`.
* Require `webPasswordSecret`.
Co-authored-by: Copilot <copilot@github.com>
* There were multiple top-level blocks regarding logging and log rotation
(for example, resources were in one place, the images were in another);
these have been consolidated.
* Renamed logging.persist to logging.persistence and put the various
options under it.
Instead of logging.persistence.enabled, we have
logging.persistence.separateVolume because we always persist logs,
it's just that on the NRP caches we put the logs on the cache volume.
* If we're not logging to a separate volume, mount the cache data volume at
/var/log (expecting the logs to live in pelican/*.log under the data
volume), following what the Houston I2 cache does.
NOTE: This is different than what some of the OSStore Origins do (they
mount a subPath of the data volume as /var/log/pelican) so we will have
to see if those two patterns can be consolidated.
* Fix a brittle hasKey check in the deployment template.
Co-authored-by: Copilot <copilot@github.com>
that is the default filename that `pelican generate password` creates.
Since it's a required value, bring it up to the top instead of mixing it in with the rest of the XRootD config. Co-authored-by: Copilot <copilot@github.com>
Do not change the imagePullPolicy for the logrotate image; we want that one to be up to date. Co-authored-by: Copilot <copilot@github.com>
…unning, for debugging Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Nest the certManager config under tls so certificate-related knobs are specified together. Co-authored-by: Copilot <copilot@github.com>
Put the resource requests/limits for the cache container itself under the cache block, for consistency with the way we set resources/limits for the logrotate container. Co-authored-by: Copilot <copilot@github.com>
Rename webPasswordSecret and webPasswordSecretKey to webPassword.existingSecret and webPassword.key, for consistency with the issuerKey and tls config. Co-authored-by: Copilot <copilot@github.com>
Use of client X.509 certs breaks on many of our caches since Let's Encrypt certificates cannot be used for client auth anymore. Disable them. Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Also drop information about tiger-osg-config - since that's a private repo, the information is of no use to others. Co-authored-by: Copilot <copilot@github.com>
…esemble the existing cache Co-authored-by: Copilot <copilot@github.com>
brianhlin
left a comment
There was a problem hiding this comment.
I find the organization of values.yaml odd, almost as if a machine wrote it :)
The relevant bug was fixed in 7.23.1 and 7.24: PelicanPlatform/pelican#3159 Co-authored-by: Claude <claude@anthropic.com>
Co-authored-by: Copilot <copilot@github.com>
…eparate label value that must match it This simplifies configuration and processing since we no longer need a separate validation for it. Note that the label needs to be sanitized because `:` and `/` are not allowed in labels. While we're at it, add the `pelicanplatform.org/` prefix to the label, because unprefixed labels should be reserved for the local cluster admin. Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Also move the definition out of `_helpers.tpl` since it's only used in one place. Co-authored-by: Copilot <copilot@github.com>
… logrotate image settings Also use the `pelican_platform/cache` image instead of the `pelican_platform/osdf-cache` image to be more generic. Co-authored-by: Copilot <copilot@github.com>
…adminUsers is nonempty Co-authored-by: Copilot <copilot@github.com>
…tes settings The `cache` block now only controls the Kubernetes resources for the cache, and the `logging` and `logrotate` blocks now only control the Kubernetes resources for the logging PVC and logrotate image. Application settings (cache tuning like `highWaterMark`, logging levels, logrotate size and count) have been moved into new `cacheConfig` and `loggingConfig` blocks. Co-authored-by: Copilot <copilot@github.com>
|
OK, this is ready for another look. Other than your comments, one thing I changed was to split out the Pelican cache configuration (high water mark, low water mark, etc.) from the Kubernetes configuration (cache image, pvc, etc.). Same for logging/logrotate. I think it's cleaner if application and Kubernetes parameters aren't mixed together. |
|
Mat told me to tell the robot to review the robot-assisted work |
There was a problem hiding this comment.
Pull request overview
This PR updates the pelican-cache Helm chart to v0.2.0, aligning it with OSDF ops deployment patterns and common Helm chart conventions, while adding new deployment options and render-time validation.
Changes:
- Refactors
values.yamlinto nested, convention-aligned sections (cache/logging/tls/oidc/etc.), addshostNetwork,sleep, and introduces requiredsitename. - Consolidates Pelican configuration into a single generated ConfigMap and adds a dedicated validation template to enforce required/conditional values at render time.
- Expands PVC behavior to support existing claims, “keep” retention, and safer rendering to avoid adoption conflicts.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
values.yaml |
Major values schema refactor; adds required sitename, new storage/logging/tls structures, and new feature toggles. |
templates/validate.yaml |
Triggers render-time validation on every template render. |
templates/service.yaml |
Skips Service creation when hostNetwork is enabled; ports now come from .Values.server.*. |
templates/networkpolicy.yaml |
Skips NetworkPolicy when hostNetwork is enabled; ports now come from .Values.server.*. |
templates/deployment.yaml |
Updates images/volumes/config mounts; adds hostNetwork + sleep; adjusts PVC/Secret mounting logic. |
templates/configmap-pelican.yaml |
New unified Pelican config ConfigMap. |
templates/configmap-logrotate.yaml |
Uses loggingConfig.rotateSize/rotateCount for logrotate settings. |
templates/certificate.yaml |
Moves cert-manager config under tls.certManager and always includes serverHostname in SANs. |
templates/pvc-cache-data.yaml |
New cache PVC naming + “keep” retention + safe render logic (lookup). |
templates/pvc-logging.yaml |
Logging PVC now conditional; supports existing claim + “keep” retention + safe render logic. |
templates/pvc-lotman.yaml |
Lotman PVC supports existing claim + “keep” retention + safe render logic. |
templates/pvc-namespace-key.yaml |
Removes namespace-key PVC (issuer key now always sourced from an existing Secret). |
templates/NOTES.txt |
Updates TLS notes and reports PVCs created/retained by the chart. |
templates/_helpers.tpl |
Adds federation label sanitization, PVC safe-render helper, and required-values validation helper. |
README.md |
Updates documentation to match new values schema, required secrets, and new features/validation. |
ci/uw-osdf-cache-values.yaml |
Updates CI profile to new values schema. |
ci/itb-osdf-pelican-cache-values.yaml |
Updates CI profile to new values schema. |
ci/houston2-i2-pelican-cache-values.yaml |
Updates CI profile to new values schema and hostNetwork profile. |
Chart.yaml |
Bumps chart version to 0.2.0 and appVersion to v7.24.2. |
AGENTS.md |
Updates agent context to match new chart structure, validation, and design decisions. |
.github/workflows/ci.yaml |
Updates CI matrix “minimal” render args for new required values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Issue: > pelican-cache.sanitizedFederationUrl redeclares $stripped with := twice, which causes a template render error in Helm/Go templates. Use = on the second assignment to update the existing variable instead of redeclaring it. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There is a default value, but error out if the user has explicilty set it to an empty string.
This PR contains many changes in order to reflect both how we (OSDF ops) want to set up our caches and community conventions for how Helm charts should look.
In addition, the PR adds support for host networking and various othe rfeatures, plus safety checks to avoid inconsistent deployments.
Notable changes:
Features
in the latter case, "storageClass" must be specified.
sleep: truecauses the cache to sleep instead of running, for debugging purposes.Validation
federation.labelmust matchfederation.discoveryUrlfor the "osdf" and "osdf-itb" federations:("osdf" must have a discoveryUrl of "https://osg-htc.org"; "osdf-itb" must have a discoveryUrl of "https://osdf-itb.osg-htc.org")
storageClassattributes must be specified for each.sitename(used forXrootd.Sitenamein the Pelican config) must be specified.tls.certManager.enabledandtls.existingSecretcannot both be set.serverHostnamemust be specified; it's automatically added to the DNS names list.Various renames
private-key.pem, which is whatpelican key generatecreates.logrotateImage,resources.logrotate, andlogrotate; these have been merged into thelogrotatemapping.hostPath,storageClassName,pvcSizeare no longer mixed together undercache; now you have acache.hostPathmapping withpath, and acache.pvcmapping withexistingClaim,storageClass, andsizecachemapping.tls.existingSecretandcertificate(for CerManager config) are no longer separate: now there aretls.certManagerandtls.existingSecret.webPasswordSecretandwebPasswordSecretKeyhave been renamed towebPassword.existingSecretandwebPassword.key.xrootd.sitenamehas been pulled out and moved to the top as justsitename; as mentioned above, it's required.logging.storageClassNameandlogging.pvcSizehave been moved tologging.persistence.storageClassandlogging.persistence.size; there are also options to not create a separate volume (logging.persistence.separateVolume: false, or reuse an existing PVC (logging.persistence.existingClaim))Minor changes
IfNotPresentsince the cache uses an image with a version tag.The logrotate image still uses the
Alwayspolicy.As per community convention, the chart's AppVersion is used as the default image tag instead of being specifiedi n the default values file.
XRD_CURLDISABLEX509=1is now always set in the environment.