Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup PR: reflect SELinux policy in RTE DS and report Progressing condition message #1058

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 63 additions & 48 deletions controllers/numaresourcesoperator_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,36 +140,35 @@ func (r *NUMAResourcesOperatorReconciler) Reconcile(ctx context.Context, req ctr

if req.Name != objectnames.DefaultNUMAResourcesOperatorCrName {
err := fmt.Errorf("incorrect NUMAResourcesOperator resource name: %s", instance.Name)
return r.updateStatus(ctx, instance, status.ConditionDegraded, status.ConditionTypeIncorrectNUMAResourcesOperatorResourceName, err)
return r.updateStatus(ctx, instance, status.ConditionDegraded, status.ConditionTypeIncorrectNUMAResourcesOperatorResourceName, "", err)
}

if err := validation.NodeGroups(instance.Spec.NodeGroups); err != nil {
return r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, err)
return r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, "", err)
}

trees, err := getTreesByNodeGroup(ctx, r.Client, instance.Spec.NodeGroups)
if err != nil {
return r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, err)
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)
return r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, "", err)
}

for idx := range trees {
conf := trees[idx].NodeGroup.NormalizeConfig()
trees[idx].NodeGroup.Config = &conf
}

result, condition, err := r.reconcileResource(ctx, instance, trees)
if condition != "" {
if condition == status.ConditionAvailable && multiMCPsErr != nil {
_, _ = r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, multiMCPsErr)
} else {
_, _ = r.updateStatus(ctx, instance, condition, reasonFromError(err), err)
}
result, condition, msg, err := r.reconcileResource(ctx, instance, trees)

if condition == status.ConditionAvailable && multiMCPsErr != nil {
_, _ = r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, "", multiMCPsErr)
} else {
_, _ = r.updateStatus(ctx, instance, condition, reasonFromError(err), msg, err)
}
return result, err
}
Expand All @@ -184,9 +183,13 @@ func updateStatusConditionsIfNeeded(instance *nropv1.NUMAResourcesOperator, cond
return ok
}

func (r *NUMAResourcesOperatorReconciler) updateStatus(ctx context.Context, instance *nropv1.NUMAResourcesOperator, condition string, reason string, stErr error) (ctrl.Result, error) {
func (r *NUMAResourcesOperatorReconciler) updateStatus(ctx context.Context, instance *nropv1.NUMAResourcesOperator, condition string, reason string, msg string, stErr error) (ctrl.Result, error) {
message := messageFromError(stErr)

if msg != "" {
message = msg
}

_ = updateStatusConditionsIfNeeded(instance, condition, reason, message)

err := r.Client.Status().Update(ctx, instance)
Expand All @@ -212,80 +215,81 @@ func (r *NUMAResourcesOperatorReconciler) reconcileResourceAPI(ctx context.Conte
return false, ctrl.Result{}, "", nil
}

func (r *NUMAResourcesOperatorReconciler) reconcileResourceMachineConfig(ctx context.Context, instance *nropv1.NUMAResourcesOperator, trees []nodegroupv1.Tree) (bool, ctrl.Result, string, error) {
func (r *NUMAResourcesOperatorReconciler) reconcileResourceMachineConfig(ctx context.Context, instance *nropv1.NUMAResourcesOperator, trees []nodegroupv1.Tree) (bool, ctrl.Result, string, string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we abuse the error instead of adding a new return value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can, progressing condition never produces a real error anyway, but I don't think it is right to abuse it because it'll create a confusion on the reconciliation result.
my take is that we can define a struct to group the common values returned by the substeps used in the reconciliation procedure. OR if we want to keep it as similar to the current state we can create another type to store additional info (includes and error and a string for msg) and replace the returned error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a new struct seems the best approach, but this needs a larger refactoring. I'll attempt something in my #1032

// we need to sync machine configs first and wait for the MachineConfigPool updates
// before checking additional components for updates
mcpUpdatedFunc, err := r.syncMachineConfigs(ctx, instance, trees)
if err != nil {
r.Recorder.Eventf(instance, corev1.EventTypeWarning, "FailedMCSync", "Failed to set up machine configuration for worker nodes: %v", err)
return true, ctrl.Result{}, status.ConditionDegraded, fmt.Errorf("failed to sync machine configs: %w", err)
return true, ctrl.Result{}, status.ConditionDegraded, "", fmt.Errorf("failed to sync machine configs: %w", err)
}
r.Recorder.Eventf(instance, corev1.EventTypeNormal, "SuccessfulMCSync", "Enabled machine configuration for worker nodes")

// MCO needs to update the SELinux context removal and other stuff, and need to trigger a reboot.
// It can take a while.
mcpStatuses, allMCPsUpdated := syncMachineConfigPoolsStatuses(instance.Name, trees, r.ForwardMCPConds, mcpUpdatedFunc)
mcpStatuses, pendingMCPs := syncMachineConfigPoolsStatuses(instance.Name, trees, r.ForwardMCPConds, mcpUpdatedFunc)
instance.Status.MachineConfigPools = mcpStatuses

if !allMCPsUpdated {
if len(pendingMCPs) != 0 {
// the Machine Config Pool still did not apply the machine config, wait for one minute
return true, ctrl.Result{RequeueAfter: numaResourcesRetryPeriod}, status.ConditionProgressing, nil
return true, ctrl.Result{RequeueAfter: numaResourcesRetryPeriod}, status.ConditionProgressing, fmt.Sprintf("wait for all MCPs to get updated, pending %v", pendingMCPs), nil
}
instance.Status.MachineConfigPools = syncMachineConfigPoolNodeGroupConfigStatuses(instance.Status.MachineConfigPools, trees)

return false, ctrl.Result{}, "", nil
return false, ctrl.Result{}, "", "", nil
}

func (r *NUMAResourcesOperatorReconciler) reconcileResourceDaemonSet(ctx context.Context, instance *nropv1.NUMAResourcesOperator, trees []nodegroupv1.Tree) ([]poolDaemonSet, bool, ctrl.Result, string, error) {
func (r *NUMAResourcesOperatorReconciler) reconcileResourceDaemonSet(ctx context.Context, instance *nropv1.NUMAResourcesOperator, trees []nodegroupv1.Tree) ([]poolDaemonSet, bool, ctrl.Result, string, string, error) {
daemonSetsInfoPerMCP, err := r.syncNUMAResourcesOperatorResources(ctx, instance, trees)
if err != nil {
r.Recorder.Eventf(instance, corev1.EventTypeWarning, "FailedRTECreate", "Failed to create Resource-Topology-Exporter DaemonSets: %v", err)
return nil, true, ctrl.Result{}, status.ConditionDegraded, fmt.Errorf("FailedRTESync: %w", err)
return nil, true, ctrl.Result{}, status.ConditionDegraded, "", fmt.Errorf("FailedRTESync: %w", err)
}
if len(daemonSetsInfoPerMCP) == 0 {
return nil, false, ctrl.Result{}, "", nil
return nil, false, ctrl.Result{}, "", "", nil
}

r.Recorder.Eventf(instance, corev1.EventTypeNormal, "SuccessfulRTECreate", "Created Resource-Topology-Exporter DaemonSets")

dssWithReadyStatus, allDSsUpdated, err := r.syncDaemonSetsStatuses(ctx, r.Client, daemonSetsInfoPerMCP)
instance.Status.DaemonSets = dssWithReadyStatus
instance.Status.RelatedObjects = relatedobjects.ResourceTopologyExporter(r.Namespace, dssWithReadyStatus)
readyDaemonSets, pendingDaemonSets, err := r.syncDaemonSetsStatuses(ctx, r.Client, daemonSetsInfoPerMCP)
instance.Status.DaemonSets = readyDaemonSets
instance.Status.RelatedObjects = relatedobjects.ResourceTopologyExporter(r.Namespace, readyDaemonSets)
if err != nil {
return nil, true, ctrl.Result{}, status.ConditionDegraded, err
return nil, true, ctrl.Result{}, status.ConditionDegraded, "", err
}
if !allDSsUpdated {
return nil, true, ctrl.Result{RequeueAfter: 5 * time.Second}, status.ConditionProgressing, nil
if len(pendingDaemonSets) != 0 {
return nil, true, ctrl.Result{RequeueAfter: 5 * time.Second}, status.ConditionProgressing, fmt.Sprintf("waiting for all RTE daemonsets to be ready, pending: %v", pendingDaemonSets), nil
}

return daemonSetsInfoPerMCP, false, ctrl.Result{}, "", nil
return daemonSetsInfoPerMCP, false, ctrl.Result{}, "", "", nil
}

func (r *NUMAResourcesOperatorReconciler) reconcileResource(ctx context.Context, instance *nropv1.NUMAResourcesOperator, trees []nodegroupv1.Tree) (ctrl.Result, string, error) {
func (r *NUMAResourcesOperatorReconciler) reconcileResource(ctx context.Context, instance *nropv1.NUMAResourcesOperator, trees []nodegroupv1.Tree) (ctrl.Result, string, string, error) {
if done, res, cond, err := r.reconcileResourceAPI(ctx, instance, trees); done {
return res, cond, err
return res, cond, "", err
}

if r.Platform == platform.OpenShift {
if done, res, cond, err := r.reconcileResourceMachineConfig(ctx, instance, trees); done {
return res, cond, err
if done, res, cond, msg, err := r.reconcileResourceMachineConfig(ctx, instance, trees); done {
return res, cond, msg, err
}
}

dsPerMCP, done, res, cond, err := r.reconcileResourceDaemonSet(ctx, instance, trees)
dsPerMCP, done, res, cond, msg, err := r.reconcileResourceDaemonSet(ctx, instance, trees)
if done {
return res, cond, err
return res, cond, msg, err
}

// all fields of NodeGroupStatus are required so publish the status only when all daemonset and MCPs are updated which
// is a certain thing if we got to this point otherwise the function would have returned already
instance.Status.NodeGroups = syncNodeGroupsStatus(instance, dsPerMCP)

return ctrl.Result{}, status.ConditionAvailable, nil
return ctrl.Result{}, status.ConditionAvailable, "", nil
}

func (r *NUMAResourcesOperatorReconciler) syncDaemonSetsStatuses(ctx context.Context, rd client.Reader, daemonSetsInfo []poolDaemonSet) ([]nropv1.NamespacedName, bool, error) {
dssWithReadyStatus := []nropv1.NamespacedName{}
func (r *NUMAResourcesOperatorReconciler) syncDaemonSetsStatuses(ctx context.Context, rd client.Reader, daemonSetsInfo []poolDaemonSet) ([]nropv1.NamespacedName, []nropv1.NamespacedName, error) {
readyDaemonSets := []nropv1.NamespacedName{}
pendingDaemonSets := []nropv1.NamespacedName{}
for _, dsInfo := range daemonSetsInfo {
ds := appsv1.DaemonSet{}
dsKey := client.ObjectKey{
Expand All @@ -294,15 +298,17 @@ func (r *NUMAResourcesOperatorReconciler) syncDaemonSetsStatuses(ctx context.Con
}
err := rd.Get(ctx, dsKey, &ds)
if err != nil {
return dssWithReadyStatus, false, err
return readyDaemonSets, pendingDaemonSets, err
}

if !isDaemonSetReady(&ds) {
return dssWithReadyStatus, false, nil
pendingDaemonSets = append(pendingDaemonSets, dsInfo.DaemonSet)
continue
}
dssWithReadyStatus = append(dssWithReadyStatus, dsInfo.DaemonSet)

readyDaemonSets = append(readyDaemonSets, dsInfo.DaemonSet)
}
return dssWithReadyStatus, true, nil
return readyDaemonSets, pendingDaemonSets, nil
}

func syncNodeGroupsStatus(instance *nropv1.NUMAResourcesOperator, dsPerMCP []poolDaemonSet) []nropv1.NodeGroupStatus {
Expand Down Expand Up @@ -382,24 +388,25 @@ func (r *NUMAResourcesOperatorReconciler) syncMachineConfigs(ctx context.Context
return waitFunc, err
}

func syncMachineConfigPoolsStatuses(instanceName string, trees []nodegroupv1.Tree, forwardMCPConds bool, updatedFunc rtestate.MCPWaitForUpdatedFunc) ([]nropv1.MachineConfigPool, bool) {
func syncMachineConfigPoolsStatuses(instanceName string, trees []nodegroupv1.Tree, forwardMCPConds bool, updatedFunc rtestate.MCPWaitForUpdatedFunc) ([]nropv1.MachineConfigPool, []string) {
klog.V(4).InfoS("Machine Config Status Sync start", "trees", len(trees))
defer klog.V(4).Info("Machine Config Status Sync stop")

mcpStatuses := []nropv1.MachineConfigPool{}
pendingMCPs := []string{}
for _, tree := range trees {
for _, mcp := range tree.MachineConfigPools {
mcpStatuses = append(mcpStatuses, extractMCPStatus(mcp, forwardMCPConds))

isUpdated := updatedFunc(instanceName, mcp)
klog.V(5).InfoS("Machine Config Pool state", "name", mcp.Name, "instance", instanceName, "updated", isUpdated)

if !isUpdated {
return mcpStatuses, false
pendingMCPs = append(pendingMCPs, mcp.Name)
continue
}
mcpStatuses = append(mcpStatuses, extractMCPStatus(mcp, forwardMCPConds))
}
}
return mcpStatuses, true
return mcpStatuses, pendingMCPs
}

func extractMCPStatus(mcp *machineconfigv1.MachineConfigPool, forwardMCPConds bool) nropv1.MachineConfigPool {
Expand Down Expand Up @@ -490,7 +497,15 @@ func (r *NUMAResourcesOperatorReconciler) syncNUMAResourcesOperatorResources(ctx
}
rteupdate.DaemonSetHashAnnotation(r.RTEManifests.DaemonSet, cmHash)
}
rteupdate.SecurityContextConstraint(r.RTEManifests.SecurityContextConstraint, annotations.IsCustomPolicyEnabled(instance.Annotations))

customPolicyEnabled := annotations.IsCustomPolicyEnabled(instance.Annotations)

delete(r.RTEManifests.DaemonSet.Annotations, annotations.SELinuxPolicyConfigAnnotation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan was to reset the annotations to allow discarding it from the DS if required; or in other words to set the annotation in the updated DS only if it is required to set a value to it

if customPolicyEnabled {
rteupdate.DaemonSetAnnotation(r.RTEManifests.DaemonSet, annotations.SELinuxPolicyConfigAnnotation, instance.Annotations[annotations.SELinuxPolicyConfigAnnotation])
}

rteupdate.SecurityContextConstraint(r.RTEManifests.SecurityContextConstraint, customPolicyEnabled)

processor := func(mcpName string, gdm *rtestate.GeneratedDesiredManifest) error {
err := daemonsetUpdater(mcpName, gdm)
Expand All @@ -502,7 +517,7 @@ func (r *NUMAResourcesOperatorReconciler) syncNUMAResourcesOperatorResources(ctx
}

existing := rtestate.FromClient(ctx, r.Client, r.Platform, r.RTEManifests, instance, trees, r.Namespace)
for _, objState := range existing.State(r.RTEManifests, processor, annotations.IsCustomPolicyEnabled(instance.Annotations)) {
for _, objState := range existing.State(r.RTEManifests, processor, customPolicyEnabled) {
if objState.Error != nil {
// We are likely in the bootstrap scenario. In this case, which is expected once, everything is fine.
// If it happens past bootstrap, still carry on. We know what to do, and we do want to enforce the desired state.
Expand Down
60 changes: 53 additions & 7 deletions controllers/numaresourcesoperator_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,7 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {

Context("with correct NRO and more than one NodeGroup", func() {
var nro *nropv1.NUMAResourcesOperator
var nroKey client.ObjectKey
var mcp1 *machineconfigv1.MachineConfigPool
var mcp2 *machineconfigv1.MachineConfigPool

Expand Down Expand Up @@ -418,9 +419,9 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {
reconciler, err = NewFakeNUMAResourcesOperatorReconciler(platform.OpenShift, defaultOCPVersion, nro, mcp1, mcp2)
Expect(err).ToNot(HaveOccurred())

key := client.ObjectKeyFromObject(nro)
nroKey = 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})
firstLoopResult, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: nroKey})
Expect(err).ToNot(HaveOccurred())
Expect(firstLoopResult).To(Equal(reconcile.Result{RequeueAfter: time.Minute}))

Expand Down Expand Up @@ -455,7 +456,7 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {
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})
secondLoopResult, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: nroKey})
Expect(err).ToNot(HaveOccurred())
Expect(secondLoopResult).To(Equal(reconcile.Result{}))

Expand All @@ -466,12 +467,14 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {
}
ds := &appsv1.DaemonSet{}
Expect(reconciler.Client.Get(context.TODO(), mcp1DSKey, ds)).ToNot(HaveOccurred())
Expect(ds.Annotations[annotations.SELinuxPolicyConfigAnnotation]).To(Equal(annotations.SELinuxPolicyCustom))

mcp2DSKey := client.ObjectKey{
Name: objectnames.GetComponentName(nro.Name, mcp2.Name),
Namespace: testNamespace,
}
Expect(reconciler.Client.Get(context.TODO(), mcp2DSKey, ds)).To(Succeed())
Expect(ds.Annotations[annotations.SELinuxPolicyConfigAnnotation]).To(Equal(annotations.SELinuxPolicyCustom))
})
When("NRO updated to remove the custom policy annotation", func() {
BeforeEach(func() {
Expand Down Expand Up @@ -508,6 +511,52 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {
err = reconciler.Client.Get(context.TODO(), mc2Key, mc)
Expect(apierrors.IsNotFound(err)).To(BeTrue(), "MachineConfig %s is expected to not be found", mc2Key.String())
})
It("should update DaemonSets annotations", func() {
// Ensure mcp1 is ready
Expect(reconciler.Client.Get(context.TODO(), client.ObjectKeyFromObject(mcp1), mcp1)).ToNot(HaveOccurred())
mcp1.Status.Configuration.Source = []corev1.ObjectReference{}
mcp1.Status.Conditions = []machineconfigv1.MachineConfigPoolCondition{
{
Type: machineconfigv1.MachineConfigPoolUpdated,
Status: corev1.ConditionTrue,
},
}
Expect(reconciler.Client.Update(context.TODO(), mcp1))

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

//after removing the annotation the MCs are detached from the MCPs and are marked as ready as well as the rest of the objects like DSs, thus no need for another reconciliation loop
postAnnotationDeleteResult, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: nroKey})
Expect(err).ToNot(HaveOccurred())
Expect(postAnnotationDeleteResult).To(Equal(reconcile.Result{}))

Expect(reconciler.Client.Get(context.TODO(), nroKey, nro)).NotTo(HaveOccurred())

By("Check DaemonSets are updated")
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())
val, ok := ds.Annotations[annotations.SELinuxPolicyConfigAnnotation]
Expect(ok).To(BeFalse(), "expected annotation %q to be deleted from RTE ds %s/%s but it was found with values %q", ds.Namespace, ds.Name, annotations.SELinuxPolicyConfigAnnotation, val)

mcpDSKey.Name = objectnames.GetComponentName(nro.Name, mcp2.Name)
Expect(reconciler.Client.Get(context.TODO(), mcpDSKey, ds)).To(Succeed())
val, ok = ds.Annotations[annotations.SELinuxPolicyConfigAnnotation]
Expect(ok).To(BeFalse(), "expected annotation %q to be deleted from RTE ds %s/%s but it was found with values %q", ds.Namespace, ds.Name, annotations.SELinuxPolicyConfigAnnotation, val)

})
})
When("a NodeGroup is deleted", func() {
BeforeEach(func() {
Expand Down Expand Up @@ -811,7 +860,6 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {
})

Context("with machine config pool with SIMPLE machine config selector", func() {

BeforeEach(func() {
var err error

Expand Down Expand Up @@ -851,7 +899,6 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {
}
Expect(reconciler.Client.Get(context.TODO(), mc2Key, mc)).ToNot(HaveOccurred())
})

})
Context("on the second iteration", func() {
var result reconcile.Result
Expand All @@ -870,8 +917,7 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {

key := client.ObjectKeyFromObject(nro)
Expect(reconciler.Client.Get(context.TODO(), key, nro)).ToNot(HaveOccurred())
Expect(len(nro.Status.MachineConfigPools)).To(Equal(1))
Expect(nro.Status.MachineConfigPools[0].Name).To(Equal("test1"))
Expect(nro.Status.MachineConfigPools).To(BeEmpty())
})
})

Expand Down
Loading