Skip to content

Commit

Permalink
Merge pull request #1055 from shajmakh/single-mcp-per-ng
Browse files Browse the repository at this point in the history
OCPBUGS-42523: allow single MCP match per node group by default
  • Loading branch information
openshift-merge-bot[bot] authored Oct 29, 2024
2 parents 39902e9 + 8c234e9 commit d4a1af1
Show file tree
Hide file tree
Showing 6 changed files with 423 additions and 1 deletion.
8 changes: 7 additions & 1 deletion controllers/numaresourcesoperator_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,8 @@ func (r *NUMAResourcesOperatorReconciler) Reconcile(ctx context.Context, req ctr
return r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, err)
}

multiMCPsErr := validation.MultipleMCPsPerTree(instance.Annotations, trees)

if err := validation.MachineConfigPoolDuplicates(trees); err != nil {
return r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, err)
}
Expand All @@ -163,7 +165,11 @@ func (r *NUMAResourcesOperatorReconciler) Reconcile(ctx context.Context, req ctr

result, condition, err := r.reconcileResource(ctx, instance, trees)
if condition != "" {
_, _ = r.updateStatus(ctx, instance, condition, reasonFromError(err), err)
if condition == status.ConditionAvailable && multiMCPsErr != nil {
_, _ = r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, multiMCPsErr)
} else {
_, _ = r.updateStatus(ctx, instance, condition, reasonFromError(err), err)
}
}
return result, err
}
Expand Down
194 changes: 194 additions & 0 deletions controllers/numaresourcesoperator_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,200 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {
})
})

Context("with node group with MCP selector that matches more than one MCP", func() {
It("should update the CR condition to degraded when annotation is not enabled but still creat all needed objects", func() {
mcpName1 := "test1"
label1 := map[string]string{
"test1": "test1",
"test": "common",
}

mcpName2 := "test2"
label2 := map[string]string{
"test2": "test2",
"test": "common",
}
ng1 := nropv1.NodeGroup{
MachineConfigPoolSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"test": "common"},
},
}

nro := testobjs.NewNUMAResourcesOperator(objectnames.DefaultNUMAResourcesOperatorCrName, ng1)

mcp1 := testobjs.NewMachineConfigPool(mcpName1, label1, &metav1.LabelSelector{MatchLabels: label1}, &metav1.LabelSelector{MatchLabels: label1})
mcp2 := testobjs.NewMachineConfigPool(mcpName2, label2, &metav1.LabelSelector{MatchLabels: label2}, &metav1.LabelSelector{MatchLabels: label2})

var err error
reconciler, err := NewFakeNUMAResourcesOperatorReconciler(platform.OpenShift, defaultOCPVersion, nro, mcp1, mcp2)
Expect(err).ToNot(HaveOccurred())

key := client.ObjectKeyFromObject(nro)
result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: key})
Expect(err).ToNot(HaveOccurred())
Expect(result).To(Equal(reconcile.Result{}))

// we still expect that all objects be created as usual without blockers (no hard requirement for now) in addition to the Degraded condition
By("Verify all needed objects were created as expected: CRD, MCP, RTE DSs")
crd := &apiextensionsv1.CustomResourceDefinition{}
crdKey := client.ObjectKey{
Name: "noderesourcetopologies.topology.node.k8s.io",
}
Expect(reconciler.Client.Get(context.TODO(), crdKey, crd)).ToNot(HaveOccurred())
Expect(reconciler.Client.Get(context.TODO(), client.ObjectKeyFromObject(mcp1), mcp1)).To(Succeed())
Expect(mcp1.Status.Configuration.Source).To(BeNil()) // default RTE SElinux policy don't expect MCs
Expect(reconciler.Client.Get(context.TODO(), client.ObjectKeyFromObject(mcp2), mcp2)).To(Succeed())
Expect(mcp2.Status.Configuration.Source).To(BeNil()) // default RTE SElinux policy don't expect MCs
mcpDSKey := client.ObjectKey{
Name: objectnames.GetComponentName(nro.Name, mcp1.Name),
Namespace: testNamespace,
}
ds := &appsv1.DaemonSet{}
Expect(reconciler.Client.Get(context.TODO(), mcpDSKey, ds)).ToNot(HaveOccurred())
mcpDSKey.Name = objectnames.GetComponentName(nro.Name, mcp2.Name)
Expect(reconciler.Client.Get(context.TODO(), mcpDSKey, ds)).To(Succeed())

By("Verify operator condition is Degraded")
Expect(reconciler.Client.Get(context.TODO(), key, nro)).ToNot(HaveOccurred())
degradedCondition := getConditionByType(nro.Status.Conditions, status.ConditionDegraded)
Expect(degradedCondition.Status).To(Equal(metav1.ConditionTrue))
Expect(degradedCondition.Reason).To(Equal(validation.NodeGroupsError))
})
It("should create objects and CR will be in Available condition when annotation is enabled - legacy", func() {
mcpName1 := "test1"
label1 := map[string]string{
"test1": "test1",
"test": "common",
}

mcpName2 := "test2"
label2 := map[string]string{
"test2": "test2",
"test": "common",
}
ng1 := nropv1.NodeGroup{
MachineConfigPoolSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"test": "common"},
},
}

nro := testobjs.NewNUMAResourcesOperator(objectnames.DefaultNUMAResourcesOperatorCrName, ng1)
nro.Annotations = map[string]string{
annotations.MultiplePoolsPerTreeAnnotation: annotations.MultiplePoolsPerTreeEnabled,
annotations.SELinuxPolicyConfigAnnotation: annotations.SELinuxPolicyCustom,
}
mcp1 := testobjs.NewMachineConfigPool(mcpName1, label1, &metav1.LabelSelector{MatchLabels: label1}, &metav1.LabelSelector{MatchLabels: label1})
mcp2 := testobjs.NewMachineConfigPool(mcpName2, label2, &metav1.LabelSelector{MatchLabels: label2}, &metav1.LabelSelector{MatchLabels: label2})

var err error
reconciler, err := NewFakeNUMAResourcesOperatorReconciler(platform.OpenShift, defaultOCPVersion, nro, mcp1, mcp2)
Expect(err).ToNot(HaveOccurred())

key := client.ObjectKeyFromObject(nro)
// on the first iteration we expect the CRDs and MCPs to be created, yet, it will wait one minute to update MC, thus RTE daemonsets and complete status update is not going to be achieved at this point
firstLoopResult, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: key})
Expect(err).ToNot(HaveOccurred())
Expect(firstLoopResult).To(Equal(reconcile.Result{RequeueAfter: time.Minute}))

// Ensure mcp1 is ready
Expect(reconciler.Client.Get(context.TODO(), client.ObjectKeyFromObject(mcp1), mcp1)).To(Succeed())
mcp1.Status.Configuration.Source = []corev1.ObjectReference{
{
Name: objectnames.GetMachineConfigName(nro.Name, mcp1.Name),
},
}
mcp1.Status.Conditions = []machineconfigv1.MachineConfigPoolCondition{
{
Type: machineconfigv1.MachineConfigPoolUpdated,
Status: corev1.ConditionTrue,
},
}
Expect(reconciler.Client.Update(context.TODO(), mcp1)).To(Succeed())

// ensure mcp2 is ready
Expect(reconciler.Client.Get(context.TODO(), client.ObjectKeyFromObject(mcp2), mcp2)).To(Succeed())
mcp2.Status.Configuration.Source = []corev1.ObjectReference{
{
Name: objectnames.GetMachineConfigName(nro.Name, mcp2.Name),
},
}
mcp2.Status.Conditions = []machineconfigv1.MachineConfigPoolCondition{
{
Type: machineconfigv1.MachineConfigPoolUpdated,
Status: corev1.ConditionTrue,
},
}
Expect(reconciler.Client.Update(context.TODO(), mcp2)).To(Succeed())

// triggering a second reconcile will create the RTEs and fully update the statuses making the operator in Available condition -> no more reconciliation needed thus the result is clean
secondLoopResult, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: key})
Expect(err).ToNot(HaveOccurred())
Expect(secondLoopResult).To(Equal(reconcile.Result{}))

By("Check DaemonSets are created")
mcp1DSKey := client.ObjectKey{
Name: objectnames.GetComponentName(nro.Name, mcp1.Name),
Namespace: testNamespace,
}
ds := &appsv1.DaemonSet{}
Expect(reconciler.Client.Get(context.TODO(), mcp1DSKey, ds)).ToNot(HaveOccurred())

mcp2DSKey := client.ObjectKey{
Name: objectnames.GetComponentName(nro.Name, mcp2.Name),
Namespace: testNamespace,
}
Expect(reconciler.Client.Get(context.TODO(), mcp2DSKey, ds)).To(Succeed())

Expect(reconciler.Client.Get(context.TODO(), key, nro)).ToNot(HaveOccurred())
availableCondition := getConditionByType(nro.Status.Conditions, status.ConditionAvailable)
Expect(availableCondition.Status).To(Equal(metav1.ConditionTrue))
})
})
Context("with correct NRO and SELinuxPolicyConfigAnnotation not set", func() {
It("should create all objects, RTE daemonsets and MCPs will get updated from the first reconcile iteration", func() {
mcpName := "test1"
label := map[string]string{
"test": "test",
}

ng := nropv1.NodeGroup{
MachineConfigPoolSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"test": "test"},
},
}

nro := testobjs.NewNUMAResourcesOperator(objectnames.DefaultNUMAResourcesOperatorCrName, ng)
mcp := testobjs.NewMachineConfigPool(mcpName, label, &metav1.LabelSelector{MatchLabels: label}, &metav1.LabelSelector{MatchLabels: label})

var err error
reconciler, err := NewFakeNUMAResourcesOperatorReconciler(platform.OpenShift, defaultOCPVersion, nro, mcp)
Expect(err).ToNot(HaveOccurred())

key := client.ObjectKeyFromObject(nro)
// when the SELinux custom annotation is not set, the controller will not wait for
// the selinux update on MC thus no reboot is required hence no need to reconcile again
firstLoopResult, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: key})
Expect(err).ToNot(HaveOccurred())
Expect(firstLoopResult).To(Equal(reconcile.Result{}))

// all objects should be created from the first reconciliation
Expect(reconciler.Client.Get(context.TODO(), client.ObjectKeyFromObject(mcp), mcp)).To(Succeed())
Expect(mcp.Status.Configuration.Source).To(BeEmpty()) // no need to wait for MC update

By("Check DaemonSet is created")
mcp1DSKey := client.ObjectKey{
Name: objectnames.GetComponentName(nro.Name, mcp.Name),
Namespace: testNamespace,
}
ds := &appsv1.DaemonSet{}
Expect(reconciler.Client.Get(context.TODO(), mcp1DSKey, ds)).ToNot(HaveOccurred())

Expect(reconciler.Client.Get(context.TODO(), key, nro)).ToNot(HaveOccurred())
availableCondition := getConditionByType(nro.Status.Conditions, status.ConditionAvailable)
Expect(availableCondition.Status).To(Equal(metav1.ConditionTrue))
})
})

Context("with correct NRO and more than one NodeGroup", func() {
var nro *nropv1.NUMAResourcesOperator
var mcp1 *machineconfigv1.MachineConfigPool
Expand Down
27 changes: 27 additions & 0 deletions internal/api/annotations/annotations.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,28 @@
/*
Copyright 2024.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package annotations

const (
SELinuxPolicyConfigAnnotation = "config.node.openshift-kni.io/selinux-policy"
SELinuxPolicyCustom = "custom"
// MultiplePoolsPerTreeAnnotation an annotation used to re-enable the support of multiple node pools per tree; starting 4.18 it is disabled by default
// the annotation is on when it's set to "enabled", every other value is equivalent to disabled
MultiplePoolsPerTreeAnnotation = "config.node.openshift-kni.io/multiple-pools-per-tree"
MultiplePoolsPerTreeEnabled = "enabled"
)

func IsCustomPolicyEnabled(annot map[string]string) bool {
Expand All @@ -11,3 +31,10 @@ func IsCustomPolicyEnabled(annot map[string]string) bool {
}
return false
}

func IsMultiplePoolsPerTreeEnabled(annot map[string]string) bool {
if v, ok := annot[MultiplePoolsPerTreeAnnotation]; ok && v == MultiplePoolsPerTreeEnabled {
return true
}
return false
}
89 changes: 89 additions & 0 deletions internal/api/annotations/annotations_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
Copyright 2024.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package annotations

import "testing"

func TestIsCustomPolicyEnabled(t *testing.T) {
testcases := []struct {
description string
annotations map[string]string
expected bool
}{
{
description: "empty map",
annotations: map[string]string{},
expected: false,
},
{
description: "annotation set but not to anything except \"custom\" value means the default",
annotations: map[string]string{
SELinuxPolicyConfigAnnotation: "true",
},
expected: false,
},
{
description: "enabled custom policy",
annotations: map[string]string{
SELinuxPolicyConfigAnnotation: "custom",
},
expected: true,
},
}
for _, tc := range testcases {
t.Run(tc.description, func(t *testing.T) {
if got := IsCustomPolicyEnabled(tc.annotations); got != tc.expected {
t.Errorf("expected %v got %v", tc.expected, got)
}
})
}
}

func TestIsMultiplePoolsPerTreeEnabled(t *testing.T) {
testcases := []struct {
description string
annotations map[string]string
expected bool
}{
{
description: "empty map",
annotations: map[string]string{},
expected: false,
},
{
description: "annotation set to anything but not \"enabled\" means it's disabled",
annotations: map[string]string{
MultiplePoolsPerTreeAnnotation: "true",
},
expected: false,
},
{
description: "enabled multiple pools per tree",
annotations: map[string]string{
MultiplePoolsPerTreeAnnotation: MultiplePoolsPerTreeEnabled,
},
expected: true,
},
}
for _, tc := range testcases {
t.Run(tc.description, func(t *testing.T) {
if got := IsMultiplePoolsPerTreeEnabled(tc.annotations); got != tc.expected {
t.Errorf("expected %v got %v", tc.expected, got)
}
})
}
}
16 changes: 16 additions & 0 deletions pkg/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

nropv1 "github.com/openshift-kni/numaresources-operator/api/numaresourcesoperator/v1"
nodegroupv1 "github.com/openshift-kni/numaresources-operator/api/numaresourcesoperator/v1/helper/nodegroup"
"github.com/openshift-kni/numaresources-operator/internal/api/annotations"
)

const (
Expand Down Expand Up @@ -146,3 +147,18 @@ func nodeGroupMachineConfigPoolSelector(nodeGroups []nropv1.NodeGroup) error {

return selectorsErrors
}

func MultipleMCPsPerTree(annot map[string]string, trees []nodegroupv1.Tree) error {
multiMCPsPerTree := annotations.IsMultiplePoolsPerTreeEnabled(annot)
if multiMCPsPerTree {
return nil
}

var err error
for _, tree := range trees {
if len(tree.MachineConfigPools) > 1 {
err = errors.Join(err, fmt.Errorf("found multiple pools matches for node group %v but expected one. Pools found %+v", &tree.NodeGroup, tree.MachineConfigPools))
}
}
return err
}
Loading

0 comments on commit d4a1af1

Please sign in to comment.