Repository: kubernetes-sigs/kueue

Issue:
[HELM] Allow to label and annotate the kueue-system namespace
<!-- Please only use this template for submitting enhancement requests -->

**What would you like to be added**:

I've heard some feedback recently that our helm chart is missing the following settings:
- ~~Kueue controller manager feature gates (there is `--set "controllerManager.featureGates[0].name=TopologyAwareScheduling,controllerManager.featureGates[0].enabled=true"` but it's a bit hacky),~~
Seems like we already support it. Thanks @mbobrovskyi 🙇‍♂️
- Additional properties for the kueue-system namespace. We could start with at least labels and annotations.

**Why is this needed**:

~~Currently Kueue has some hot features that are still in alpha like TAS. It'd be good to provide a reliable and repeatable way to perform an initial installation and upgrades.~~

As for namespaces it's a common practice to use labels and annotations at the namespace level for some additional network setup like with service meshes or to enable certain monitoring features for pods within a namespace.

**Completion requirements**:

This enhancement requires the following artifacts:

- [ ] Design doc
- [ ] API change
- [ ] Docs update

The artifacts should be linked in subsequent comments.


Discussion:
@kannon92: I'd like to discourage the use of CLI arguments on the deployment for feature gates. Most features can add new configuration fields. We should either encourage the use of config map for configuration or CLI arguments. If we continue to support both it can lead to headaches. 

We allow either config map or deployment. 

https://github.com/kubernetes-sigs/kueue/blob/main/apis/config/v1beta1/configuration_types.go#L104@kannon92: The second option I have no problem with.@mwysokin: Yeah, I mentioned the CLI example just as hacky way of achieving this today but I agree that it's awful. That's why it'd be good to have native support for feature gates in the helm chart.@mbobrovskyi: We support featureGates in Helm:

https://github.com/kubernetes-sigs/kueue/blob/750dc52a707b6b7e9e7c6101c49d8bbea611d308/charts/kueue/values.yaml#L13-L16

https://github.com/kubernetes-sigs/kueue/blob/750dc52a707b6b7e9e7c6101c49d8bbea611d308/charts/kueue/templates/manager/manager.yaml#L28@mwysokin: That's great! I must've missed it!

In that case let me edit the issue  to leave only the second request. @mbobrovskyi: /assign@mimowo: Well, it is supported, but:
1. lacks documentation in [here](https://github.com/kubernetes-sigs/kueue/blob/main/charts/kueue/README.md)
2. the API is very inconvenient use set via "--set" parameter, as mentioned, above, and there is no mention of using the overrides file

I don't want to scope creep the issue, but I would still propose:
1. documenting featureGates inside the HELM README.md
2. inside the helm readme add a short note indicating that for more advanced parametrization of Kueue we recommend using a local overrides file used via `--values overrides.yaml`, for example:
```yaml
controllerManager:
  replicas: 2
  manager:
    resources:
      limits:
        cpu: "2" 
        memory: 2Gi 
      requests:
        cpu: "2" 
        memory: 2Gi 
```
because often users wouldn't know that
3. add a note on that in the [Productization](https://kueue.sigs.k8s.io/docs/tasks/manage/productization/) page that if a user is customizing the local installation of Kueue, then we recommend using helm, with local overrides file) which makes it easier to update Kueue on upgrades.

wdyt @mwysokin @mbobrovskyi ?
@mwysokin: SGTM 🖖

I'll update the issue. @mbobrovskyi: > Additional properties for the kueue-system namespace. We could start with at least labels and annotations.

This is a tricky area in Helm. The helm install command uses the `--create-namespace` flag to automatically create the namespace:

```
$ helm install kueue kueue/ --create-namespace --namespace kueue-system
```

However, Helm does not support setting labels or annotations on the namespace when using `--create-namespace`, and it doesn’t provide `--labels` or `--annotations` flags.

One workaround is to add a namespace manifest (template) in the chart, but this is not a common practice in Helm. It also causes conflicts if the namespace already exists, breaking the `--create-namespace` behavior:

```
$ helm install kueue charts/kueue/ --namespace kueue-system --create-namespace

Error: INSTALLATION FAILED: 1 error occurred:
        * namespaces "kueue-system" already exists
```

Additionally, this approach breaks helm upgrade due to missing ownership metadata:

```
Error: UPGRADE FAILED: Unable to continue with update: Namespace "kueue-system" in namespace "" exists and cannot be imported into the current release: invalid ownership metadata; 
label validation error: missing key "app.kubernetes.io/managed-by": must be set to "Helm"; 
annotation validation error: missing key "meta.helm.sh/release-name": must be set to "kueue"; 
annotation validation error: missing key "meta.helm.sh/release-namespace": must be set to "kueue-system"
```

Recommendation:
The best approach is to create the namespace manually before installing the Helm chart, allowing users to set any desired labels or annotations. Alternatively, labels and annotations can be added to the namespace after the Helm install.@mimowo: > Recommendation:
The best approach is to create the namespace manually before installing the Helm chart, allowing users to set any desired labels or annotations. Alternatively, labels and annotations can be added to the namespace after the Helm install.

Sounds reasonable to me. Indeed, the lifetime of the namespace is longer than the lifetime of Kueue version (as it would typically survive upgrades). So, I can see why this is tricky.

I think preparing the kueue-system namespace with custom labels and annotations could also be part of the Productization page. @mimowo: @mbobrovskyi could you follow up extending (2.) with the example for setting an example feature gate, and (3.)?

Solution patch:
diff --git a/charts/kueue/README.md b/charts/kueue/README.md
index 3e3bc54e1..c110cf314 100644
--- a/charts/kueue/README.md
+++ b/charts/kueue/README.md
@@ -35,6 +35,9 @@ For more advanced parametrization of Kueue, we recommend using a local overrides
 
 ```yaml
 controllerManager:
+  featureGates:
+    - name: TopologyAwareScheduling
+      enabled: true
   replicas: 2
   manager:
     resources:
diff --git a/charts/kueue/README.md.gotmpl b/charts/kueue/README.md.gotmpl
index ee3a6f850..8ed6411a8 100644
--- a/charts/kueue/README.md.gotmpl
+++ b/charts/kueue/README.md.gotmpl
@@ -37,6 +37,9 @@ For more advanced parametrization of Kueue, we recommend using a local overrides
 
 ```yaml
 controllerManager:
+  featureGates:
+    - name: TopologyAwareScheduling
+      enabled: true
   replicas: 2
   manager:
     resources:



Test patch:
diff --git a/test/integration/singlecluster/scheduler/scheduler_test.go b/test/integration/singlecluster/scheduler/scheduler_test.go
index d172b5667..9d8905c26 100644
--- a/test/integration/singlecluster/scheduler/scheduler_test.go
+++ b/test/integration/singlecluster/scheduler/scheduler_test.go
@@ -473,7 +473,11 @@ var _ = ginkgo.Describe("Scheduler", func() {
 
 			ginkgo.By("checking the first workload gets created and gets quota reserved", func() {
 				util.MustCreate(ctx, k8sClient, wl1)
-				util.ExpectWorkloadToBeAdmittedAs(ctx, k8sClient, wl1, nil)
+				wl1Admission := testing.MakeAdmission(admissionCheckClusterQ.Name).
+					Assignment(corev1.ResourceCPU, kueue.ResourceFlavorReference(onDemandFlavor.Name), "2").
+					AssignmentPodCount(1).
+					Obj()
+				util.ExpectWorkloadToBeAdmittedAs(ctx, k8sClient, wl1, wl1Admission)
 				util.ExpectPendingWorkloadsMetric(admissionCheckClusterQ, 0, 0)
 				util.ExpectReservingActiveWorkloadsMetric(admissionCheckClusterQ, 1)
 				util.ExpectQuotaReservedWorkloadsTotalMetric(admissionCheckClusterQ, 1)