feat(installer): stamp provenance labels on installer-managed resources#25
Open
dmathur0 wants to merge 2 commits into
Open
feat(installer): stamp provenance labels on installer-managed resources#25dmathur0 wants to merge 2 commits into
dmathur0 wants to merge 2 commits into
Conversation
Stamp a consistent set of app.kubernetes.io/* labels plus a nv-config-manager.nvidia.com/installer-version annotation onto every resource the installer creates directly via the Kubernetes API (namespaces, secrets, PVCs, loader pods), and push the same installer marker onto operator Helm releases via each chart's commonLabels value. Operator namespaces are pre-created so they carry the labels instead of being lazily created unlabelled by --create-namespace. This makes installer-managed objects discoverable for cleanup with simple label selectors. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
dmathur0
requested review from
jojung1,
polarweasel,
ryanheffernan,
spidercensus,
susanhooks and
zsblevins
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds installer provenance metadata (labels and annotations) to Kubernetes resources created by the installer. It extends the K8sClient to stamp all directly-created resources with managed-by, part-of, and instance labels plus an installer-version annotation, and wires namespace pre-creation and Helm label injection into the operator CRD installation flow for Envoy Gateway, cert-manager, CNPG, and prometheus-operator. ChangesInstaller Provenance Metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
installer/src/nv_config_manager_installer/deployer.py (1)
1621-1651:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStamp the connected-install cert-manager CRDs too.
global.commonLabelsonly covers Helm-rendered resources. In the connected-install path, thekubectl applyofcert-manager.crds.yamlstill creates cluster-scoped CRDs without installer provenance, so those resources won't be discoverable for cleanup/audit the way the rest of this PR intends.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@installer/src/nv_config_manager_installer/deployer.py` around lines 1621 - 1651, After applying the upstream cert-manager.crds.yaml with _run_logged, stamp those created cluster-scoped CRDs with the same installer labels so they’re discoverable; specifically, after the existing _run_logged call that applies the URL, run a second _run_logged invocation that selects the cert-manager CRDs (e.g. via kubectl get crd -o name with a pattern for cert-manager CRDs) and adds/overwrites the labels from cert_manager_label_args using kubectl label --overwrite, using the same step and self.callback variables so the CRDs receive the global/common installer labels used elsewhere (reference cert_manager_chart, cert_manager_args, cert_manager_label_args, _run_logged, step, self.callback).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@installer/src/nv_config_manager_installer/deployer.py`:
- Around line 1621-1651: After applying the upstream cert-manager.crds.yaml with
_run_logged, stamp those created cluster-scoped CRDs with the same installer
labels so they’re discoverable; specifically, after the existing _run_logged
call that applies the URL, run a second _run_logged invocation that selects the
cert-manager CRDs (e.g. via kubectl get crd -o name with a pattern for
cert-manager CRDs) and adds/overwrites the labels from cert_manager_label_args
using kubectl label --overwrite, using the same step and self.callback variables
so the CRDs receive the global/common installer labels used elsewhere (reference
cert_manager_chart, cert_manager_args, cert_manager_label_args, _run_logged,
step, self.callback).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3d8f7c3b-7f6c-49ee-a603-acecf14f7615
📒 Files selected for processing (4)
installer/src/nv_config_manager_installer/deployer.pyinstaller/src/nv_config_manager_installer/k8s.pyinstaller/tests/test_deployer.pyinstaller/tests/test_k8s_metadata.py
gateway-helm has no chart-wide commonLabels value, so the --set-string commonLabels.* args were silently ignored and never landed on any envoy resource. Envoy provenance already comes from the pre-created envoy-gateway-system namespace label, which is what cleanup selectors key off of, so drop the misleading arg and document why. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stamp a consistent set of app.kubernetes.io/* labels plus a nv-config-manager.nvidia.com/installer-version annotation onto every resource the installer creates directly via the Kubernetes API (namespaces, secrets, PVCs, loader pods), and push the same installer marker onto operator Helm releases via each chart's commonLabels value. Operator namespaces are pre-created so they carry the labels instead of being lazily created unlabelled by --create-namespace. This makes installer-managed objects discoverable for cleanup with simple label selectors.
Description
Validation
The kind integration test is manual due to taking ~30 min to complete. When the PR is ready for review,
run Actions -> Kind Integration -> Run workflow against the copy-pr-bot generated
pull-request/<PR_NUMBER>branch. Use the defaulttest_pathfor the full suite,or narrow it only while debugging.
Passing Kind Integration run:
Checklist
CONTRIBUTING.md.docs screenshots, or Helm/rendered outputs.
Summary by CodeRabbit