diff --git a/README.md b/README.md index be51e9128..cc203ce8b 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,13 @@ spec: operator: "Exists" effect: "NoSchedule" priorityClassName: "system-cluster-critical" + resources: + requests: + memory: "40Mi" + cpu: "40m" + limits: + memory: "600Mi" + cpu: "300m" config: name: "myconfig" namespace: "openshift-file-integrity" @@ -64,6 +71,7 @@ In the `spec`: * **nodeSelector**: Selector for nodes to schedule the scan instances on. * **tolerations**: Specify tolerations to schedule on nodes with custom taints. When not specified, a default toleration allowing running on master and infra nodes is applied. * **priorityClassName**: (Optional) Specifies the `PriorityClass` for the pods created by the operator. If the PriorityClass is invalid or not found, it will be ignored and cleared from the spec. +* **resources**: (Optional) Specifies the resource requests and limits for the AIDE daemon container. When omitted, the operator applies its built-in defaults (requests: 40Mi memory / 40m CPU, limits: 600Mi memory / 300m CPU). * **config**: Point to a ConfigMap containing an AIDE configuration to use instead of the CoreOS optimized default. See "Applying an AIDE config" below. * **config.gracePeriod**: The number of seconds to pause in between AIDE integrity checks. Frequent AIDE checks on a node may be resource intensive, so it can be useful to specify a longer interval. Defaults to 900 (15 mins). * **config.maxBackups**: The maximum number of AIDE database and log backups (leftover from the re-init process) to keep on a node. Older backups beyond this number are automatically pruned by the daemon. Defaults to 5. diff --git a/bundle/manifests/fileintegrity.openshift.io_fileintegrities.yaml b/bundle/manifests/fileintegrity.openshift.io_fileintegrities.yaml index d89830218..7d95d43c6 100644 --- a/bundle/manifests/fileintegrity.openshift.io_fileintegrities.yaml +++ b/bundle/manifests/fileintegrity.openshift.io_fileintegrities.yaml @@ -84,6 +84,74 @@ spec: This is an optional field. If PriorityClass is invalid or not found, it will be ignored and cleared from the spec. type: string + resources: + default: + limits: + cpu: 300m + memory: 600Mi + requests: + cpu: 40m + memory: 40Mi + description: |- + Resources allows specifying the resource requests and limits for the AIDE + daemon container. When omitted, the built-in defaults below are applied. + properties: + claims: + description: |- + Claims lists the names of resources, defined in spec.resourceClaims, + that are used by this container. + + This field depends on the + DynamicResourceAllocation feature gate. + + This field is immutable. It can only be set for containers. + items: + description: ResourceClaim references one entry in PodSpec.ResourceClaims. + properties: + name: + description: |- + Name must match the name of one entry in pod.spec.resourceClaims of + the Pod where this field is used. It makes that resource available + inside a container. + type: string + request: + description: |- + Request is the name chosen for a request in the referenced claim. + If empty, everything from the claim is made available, otherwise + only the result of this request. + type: string + required: + - name + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map + limits: + additionalProperties: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + description: |- + Limits describes the maximum amount of compute resources allowed. + More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ + type: object + requests: + additionalProperties: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + description: |- + Requests describes the minimum amount of compute resources required. + If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, + otherwise to an implementation-defined value. Requests cannot exceed Limits. + More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ + type: object + type: object tolerations: default: - effect: NoSchedule diff --git a/config/crd/bases/fileintegrity.openshift.io_fileintegrities.yaml b/config/crd/bases/fileintegrity.openshift.io_fileintegrities.yaml index 1fd87caeb..a91822499 100644 --- a/config/crd/bases/fileintegrity.openshift.io_fileintegrities.yaml +++ b/config/crd/bases/fileintegrity.openshift.io_fileintegrities.yaml @@ -84,6 +84,74 @@ spec: This is an optional field. If PriorityClass is invalid or not found, it will be ignored and cleared from the spec. type: string + resources: + default: + limits: + cpu: 300m + memory: 600Mi + requests: + cpu: 40m + memory: 40Mi + description: |- + Resources allows specifying the resource requests and limits for the AIDE + daemon container. When omitted, the built-in defaults below are applied. + properties: + claims: + description: |- + Claims lists the names of resources, defined in spec.resourceClaims, + that are used by this container. + + This field depends on the + DynamicResourceAllocation feature gate. + + This field is immutable. It can only be set for containers. + items: + description: ResourceClaim references one entry in PodSpec.ResourceClaims. + properties: + name: + description: |- + Name must match the name of one entry in pod.spec.resourceClaims of + the Pod where this field is used. It makes that resource available + inside a container. + type: string + request: + description: |- + Request is the name chosen for a request in the referenced claim. + If empty, everything from the claim is made available, otherwise + only the result of this request. + type: string + required: + - name + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map + limits: + additionalProperties: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + description: |- + Limits describes the maximum amount of compute resources allowed. + More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ + type: object + requests: + additionalProperties: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + description: |- + Requests describes the minimum amount of compute resources required. + If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, + otherwise to an implementation-defined value. Requests cannot exceed Limits. + More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ + type: object + type: object tolerations: default: - effect: NoSchedule diff --git a/pkg/apis/fileintegrity/v1alpha1/fileintegrity_types.go b/pkg/apis/fileintegrity/v1alpha1/fileintegrity_types.go index 656692053..c0d6e3fe9 100644 --- a/pkg/apis/fileintegrity/v1alpha1/fileintegrity_types.go +++ b/pkg/apis/fileintegrity/v1alpha1/fileintegrity_types.go @@ -53,6 +53,10 @@ type FileIntegritySpec struct { // This is an optional field. If PriorityClass is invalid or not found, // it will be ignored and cleared from the spec. PriorityClassName string `json:"priorityClassName,omitempty"` + // Resources allows specifying the resource requests and limits for the AIDE + // daemon container. When omitted, the built-in defaults below are applied. + // +kubebuilder:default={requests: {memory: "40Mi", cpu: "40m"}, limits: {memory: "600Mi", cpu: "300m"}} + Resources *corev1.ResourceRequirements `json:"resources,omitempty"` } // FileIntegrityConfig defines the name, namespace, and data key for an AIDE config to use for integrity checking. diff --git a/pkg/apis/fileintegrity/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/fileintegrity/v1alpha1/zz_generated.deepcopy.go index 27b98f8f6..6ad0307a0 100644 --- a/pkg/apis/fileintegrity/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/fileintegrity/v1alpha1/zz_generated.deepcopy.go @@ -1,5 +1,4 @@ //go:build !ignore_autogenerated -// +build !ignore_autogenerated /* Copyright © 2019 - 2022 Red Hat Inc. @@ -199,6 +198,11 @@ func (in *FileIntegritySpec) DeepCopyInto(out *FileIntegritySpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.Resources != nil { + in, out := &in.Resources, &out.Resources + *out = new(v1.ResourceRequirements) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FileIntegritySpec. diff --git a/pkg/controller/fileintegrity/fileintegrity_controller.go b/pkg/controller/fileintegrity/fileintegrity_controller.go index 8ae0aa811..c9cce4daa 100644 --- a/pkg/controller/fileintegrity/fileintegrity_controller.go +++ b/pkg/controller/fileintegrity/fileintegrity_controller.go @@ -592,8 +592,9 @@ func (r *FileIntegrityReconciler) FileIntegrityControllerReconcile(request recon tolsNeedsUpdate := updateDSTolerations(dsCopy, instance, reqLogger) pcNeedsUpdate := updateDSPriorityClassName(dsCopy, instance, reqLogger) volsNeedUpdate := updateDSContainerVolumes(dsCopy, instance, operatorImage, reqLogger) + resNeedsUpdate := updateDSResources(dsCopy, instance, reqLogger) - if argsNeedUpdate || imgNeedsUpdate || nsNeedsUpdate || tolsNeedsUpdate || pcNeedsUpdate || volsNeedUpdate || scriptsUpdated { + if argsNeedUpdate || imgNeedsUpdate || nsNeedsUpdate || tolsNeedsUpdate || pcNeedsUpdate || volsNeedUpdate || resNeedsUpdate || scriptsUpdated { if err := r.Client.Update(context.TODO(), dsCopy); err != nil { return reconcile.Result{}, err } @@ -666,6 +667,20 @@ func updateDSPriorityClassName(currentDS *appsv1.DaemonSet, fi *v1alpha1.FileInt return needsUpdate } +// Returns true when the daemon container resources derived from the FileIntegrity +// object differ from the current DS. Returns false if there was no difference. +// If an update is needed, this will update the resources from the given DaemonSet. +func updateDSResources(currentDS *appsv1.DaemonSet, fi *v1alpha1.FileIntegrity, logger logr.Logger) bool { + resRef := ¤tDS.Spec.Template.Spec.Containers[0].Resources + expectedResources := getDaemonResources(fi) + needsUpdate := !reflect.DeepEqual(*resRef, expectedResources) + if needsUpdate { + logger.Info("FileIntegrity needed daemon resources update") + *resRef = expectedResources + } + return needsUpdate +} + // Returns true when the daemon pod args derived from the FileIntegrity object differ from the current DS. // Returns false if there was no difference. // If an update is needed, this will update the arguments from the given DaemonSet @@ -1000,16 +1015,7 @@ func aideDaemonset(dsName string, fi *v1alpha1.FileIntegrity, operatorImage stri MountPath: "/tmp", }, }, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceMemory: resource.MustParse("40Mi"), - corev1.ResourceCPU: resource.MustParse("40m"), - }, - Limits: corev1.ResourceList{ - corev1.ResourceMemory: resource.MustParse("600Mi"), - corev1.ResourceCPU: resource.MustParse("300m"), - }, - }, + Resources: getDaemonResources(fi), }, }, Volumes: []corev1.Volume{ @@ -1064,6 +1070,27 @@ func getDebug(fi *v1alpha1.FileIntegrity) string { return strconv.FormatBool(fi.Spec.Debug) } +// getDaemonResources returns the resource requirements for the AIDE daemon +// container. The CRD applies a default when the field is omitted, but we keep a +// built-in fallback here so objects constructed without defaulting (e.g. in +// tests) still get sane values. +func getDaemonResources(fi *v1alpha1.FileIntegrity) corev1.ResourceRequirements { + if fi.Spec.Resources != nil { + return *fi.Spec.Resources + } + // If the user did not specify any resources, return the operator's built-in defaults. + return corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("40Mi"), + corev1.ResourceCPU: resource.MustParse("40m"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("600Mi"), + corev1.ResourceCPU: resource.MustParse("300m"), + }, + } +} + func daemonArgs(dsName string, fi *v1alpha1.FileIntegrity) []string { return []string{"daemon", "--lc-file=" + aideLogPath, diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index 2de2b3d3b..0ad9163eb 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -11,6 +11,8 @@ import ( fileintegrity2 "github.com/openshift/file-integrity-operator/pkg/controller/fileintegrity" framework "github.com/openshift/file-integrity-operator/tests/framework" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/types" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -911,6 +913,96 @@ func TestFileIntegrityChangeGracePeriod(t *testing.T) { } } +func TestFileIntegrityChangeResources(t *testing.T) { + f, testctx, namespace := setupTest(t) + testName := testIntegrityNamePrefix + "-changeresources" + setupFileIntegrity(t, f, testctx, testName, namespace, nodeWorkerRoleLabelKey, defaultTestGracePeriod) + defer testctx.Cleanup() + defer func() { + if err := cleanNodes(f, namespace); err != nil { + t.Fatal(err) + } + if err := resetBundleTestMetrics(f, namespace); err != nil { + t.Fatal(err) + } + }() + defer logContainerOutput(t, f, namespace, testName) + + // wait to go active. + err := waitForScanStatus(t, f, namespace, testName, v1alpha1.PhaseActive) + if err != nil { + t.Errorf("Timeout waiting for scan status") + } + + // get daemonSet, make sure it has the default resources + defaultResources := corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("40Mi"), + corev1.ResourceCPU: resource.MustParse("40m"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("600Mi"), + corev1.ResourceCPU: resource.MustParse("300m"), + }, + } + err = assertDSPodHasResources(t, f, testName, namespace, defaultResources, time.Second*5, time.Minute*5) + if err != nil { + t.Errorf("pod spec didn't contain the expected default resources: %v\n", err) + } + t.Log("The pod spec contains the default resources") + + oldPodList, err := getFiDsPods(f, testName, namespace) + if err != nil { + t.Errorf("Error retrieving DS pods") + } + + // change the config + fileIntegrity := &v1alpha1.FileIntegrity{} + err = f.Client.Get(context.TODO(), types.NamespacedName{Name: testName, Namespace: namespace}, fileIntegrity) + if err != nil { + t.Errorf("failed to retrieve FI object: %v\n", err) + } + + modifiedResources := corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("100Mi"), + corev1.ResourceCPU: resource.MustParse("100m"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("800Mi"), + corev1.ResourceCPU: resource.MustParse("500m"), + }, + } + + fileIntegrityCopy := fileIntegrity.DeepCopy() + fileIntegrityCopy.Spec.Resources = &modifiedResources + + err = f.Client.Update(context.TODO(), fileIntegrityCopy) + if err != nil { + t.Errorf("failed to update FI object: %v\n", err) + } + + // make sure the daemonSet pods now have the resources we want + err = assertDSPodHasResources(t, f, testName, namespace, modifiedResources, time.Second*5, time.Minute*5) + if err != nil { + t.Errorf("spec didn't contain the expected resources: %v\n", err) + } + t.Log("The spec contains the modified resources") + + // make sure the DS restarted by first making sure at least one of the original pods + // went away, then waiting until the DS is ready again + err = waitUntilPodsAreGone(t, f.Client.Client, oldPodList, time.Second*5, time.Minute*5) + if err != nil { + t.Errorf("The old pods were not shut down\n") + } + + resDsName := common.DaemonSetName(testName) + err = waitForDaemonSet(daemonSetIsReady(f.KubeClient, resDsName, namespace)) + if err != nil { + t.Errorf("Timed out waiting for DaemonSet %s", resDsName) + } +} + func TestFileIntegrityChangeDebug(t *testing.T) { f, testctx, namespace := setupTest(t) testName := testIntegrityNamePrefix + "-changedebug" diff --git a/tests/e2e/helpers.go b/tests/e2e/helpers.go index 524c2193b..e5914f456 100644 --- a/tests/e2e/helpers.go +++ b/tests/e2e/helpers.go @@ -2190,6 +2190,39 @@ func assertDSPodHasArg(t *testing.T, f *framework.Framework, fiName, namespace, }) } +// assertDSPodHasResources polls the FileIntegrity DaemonSet until the daemon +// container's resource requests and limits match the expected values. +func assertDSPodHasResources(t *testing.T, f *framework.Framework, fiName, namespace string, expected corev1.ResourceRequirements, interval, timeout time.Duration) error { + return wait.PollImmediate(interval, timeout, func() (bool, error) { + ds, getErr := f.KubeClient.AppsV1().DaemonSets(namespace).Get(goctx.TODO(), common.DaemonSetName(fiName), metav1.GetOptions{}) + if getErr != nil { + t.Logf("Retrying. Got error: %v\n", getErr) + return false, nil + } + actual := ds.Spec.Template.Spec.Containers[0].Resources + if resourceListMatches(expected.Requests, actual.Requests) && resourceListMatches(expected.Limits, actual.Limits) { + return true, nil + } + t.Logf("Expected resources not found, retrying. Got: %v", actual) + return false, nil + }) +} + +// resourceListMatches returns true when every quantity in expected is present +// in actual with an equal value. +func resourceListMatches(expected, actual corev1.ResourceList) bool { + if len(expected) != len(actual) { + return false + } + for name, q := range expected { + aq, ok := actual[name] + if !ok || q.Cmp(aq) != 0 { + return false + } + } + return true +} + func getFiDsPods(f *framework.Framework, fileIntegrityName, namespace string) (*corev1.PodList, error) { dsName := common.DaemonSetName(fileIntegrityName) ds, err := f.KubeClient.AppsV1().DaemonSets(namespace).Get(goctx.TODO(), dsName, metav1.GetOptions{})