Improvement/integrate registry operator mk8s - #5097
Conversation
moved from init.sls to core.sls because it's needed by metalk8s-registry-operator and metalk8s-registry-operator is a core component Ref: MK8S-168
Hello eg-ayoub,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
8e50d61 to
f712e71
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - result: __slot__:salt:metalk8s_kubernetes.check_object_ready( | ||
| apiVersion='apps/v1', kind=Deployment, | ||
| name="metalk8s-registry-operator-controller-manager", | ||
| namespace="metalk8s-registry-system") |
There was a problem hiding this comment.
This checks the deployment in namespace metalk8s-registry-system, but the kustomize namespace override places the deployment in metalk8s-registry. The readiness check will never find the deployment.
— Claude Code
| apiVersion: v1 | ||
| kind: Namespace | ||
| metadata: | ||
| name: metalk8s-registry-system |
There was a problem hiding this comment.
This creates namespace metalk8s-registry-system but all chart resources are deployed to metalk8s-registry (the kustomize namespace override). The namespace created here is unused.
— Claude Code
| - metalk8s.kubernetes.cni.calico.deployed | ||
| - metalk8s.kubernetes.coredns.deployed | ||
| - metalk8s.addons.cert-manager.deployed | ||
| - metalk8s.addons.registry-operator.deployed |
There was a problem hiding this comment.
Wrong salt module path: directory is metalk8s-registry-operator (with metalk8s- prefix), not registry-operator. This will fail at deploy time with a missing SLS error. Should be metalk8s.addons.metalk8s-registry-operator.deployed.
— Claude Code
|
| effect: "NoSchedule" | ||
| - key: "node-role.kubernetes.io/master" | ||
| operator: "Exists" | ||
| effect: "NoSchedule" |
There was a problem hiding this comment.
nodeSelector is at the wrong YAML level — it's under spec.template.nodeSelector (not a valid Deployment field) instead of spec.template.spec.nodeSelector. Kubernetes will silently ignore it, so the operator won't be constrained to master nodes. The rendered chart.sls confirms this: nodeSelector appears as a sibling of spec inside template.
| effect: "NoSchedule" | |
| apiVersion: apps/v1 | |
| kind: Deployment | |
| metadata: | |
| name: not-important | |
| spec: | |
| template: | |
| spec: | |
| nodeSelector: | |
| node-role.kubernetes.io/master: "" | |
| tolerations: | |
| - key: "node-role.kubernetes.io/bootstrap" | |
| operator: "Exists" | |
| effect: "NoSchedule" | |
| - key: "node-role.kubernetes.io/infra" | |
| operator: "Exists" | |
| effect: "NoSchedule" | |
| - key: "node-role.kubernetes.io/master" | |
| operator: "Exists" | |
| effect: "NoSchedule" |
— Claude Code
Review by Claude Code |
f712e71 to
39f5a48
Compare
|
Still open from prior review: |
Also deploy its necessary Certificates and Issuers Ref: MK8S-169
| @@ -0,0 +1,45 @@ | |||
| include: | |||
There was a problem hiding this comment.
Missing explicit cert-manager dependency. The chart.sls deploys cert-manager resources (Certificate, Issuer, webhook cert-manager.io/inject-ca-from annotation), and .registry-certs creates additional Certificates and ClusterIssuers. Without cert-manager deployed first, the metalk8s_kubernetes renderer will fail because the Certificate/Issuer CRDs do not exist yet. Follow the pattern in crl-operator/deployed/init.sls: add - metalk8s.addons.cert-manager.deployed to the include list, then add a gate state that requires cert-manager and require_in the chart.
— Claude Code
|
|
❌ Changelog check failed This PR does not add a
Example: If a changelog entry is not appropriate for this PR (e.g. CI-only or |
| - .registry-certs | ||
| - .registry | ||
|
|
||
| Wait for Registry Operator to be Ready: |
There was a problem hiding this comment.
Missing namespace-before-resources ordering. Two gaps:
1. .namespace must run before .chart — chart.sls deploys ServiceAccount, Deployment, etc. into metalk8s-registry-system, but no requisite guarantees .namespace creates it first.
2. .registry-namespace must run before .registry-certs — mtls.sls creates a Certificate in namespace metalk8s-registry, but nothing orders .registry-namespace before .registry-certs.
Without explicit gates, Salt may apply resources before the target namespace exists, causing API failures. Follow the disk-management-agent/deployed/init.sls pattern — add require/require_in states for each pair.
— Claude Code
|
2 participants
let's plug in the registry operator in metalk8s