Skip to content

fix(): Optimize NetworkPolicy reconciler to reduce unnecessary API update calls and reduce log noise #446

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.

Sign up for GitHub

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

Open
greninja517 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(): Optimize NetworkPolicy reconciler to reduce unnecessary API update calls and reduce log noise #446

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62a644b
Optimize NetworkPolicy reconciler to reduce unnecessary API update ca…
greninja517 Aug 19, 2025
922147b
Update error handling during status update of slice
greninja517 Aug 20, 2025
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
67 changes: 53 additions & 14 deletions controllers/slice/namespaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -668,33 +669,71 @@ func (r *SliceReconciler) reconcileSliceNetworkPolicy(ctx context.Context, slice
return err
}
}

// Track for any changes made
updatedNamespaces := []string{}
successfulNamespaces := 0
var errors []error

for _, appNsObj := range appNsList.Items {
err = r.installSliceNetworkPolicyInAppNs(ctx, slice, appNsObj.ObjectMeta.Name)
updated, err := r.installSliceNetworkPolicyInAppNs(ctx, slice, appNsObj.ObjectMeta.Name)
if err != nil {
log.Error(err, "Failed to install network policy", "namespace", appNsObj.ObjectMeta.Name)
return err
log.Error(err, "Failed to install/update network policy", "namespace", appNsObj.ObjectMeta.Name)
errors = append(errors, fmt.Errorf("namespace %s: %w", appNsObj.ObjectMeta.Name, err))
continue
}
successfulNamespaces++
if updated {
updatedNamespaces = append(updatedNamespaces, appNsObj.ObjectMeta.Name)
}
}

if len(updatedNamespaces) > 0 {
utils.RecordEvent(ctx, r.EventRecorder, slice, nil, ossEvents.EventNetPolAdded, "slice_reconciler")
log.Info("Installed netpol for namespace successfully", "namespace", appNsObj.ObjectMeta.Name)
log.Info("Network Policies Installed/Updated", "namespaces", updatedNamespaces, "count", len(updatedNamespaces))
}
slice.Status.NetworkPoliciesInstalled = true
return r.Status().Update(ctx, slice)

policiesInstalled := successfulNamespaces > 0
if slice.Status.NetworkPoliciesInstalled != policiesInstalled {
slice.Status.NetworkPoliciesInstalled = policiesInstalled
if err := r.Status().Update(ctx, slice); err != nil {
return err
}
}

if len(errors) > 0 {
Copy link
Contributor

@gourishkb gourishkb Aug 26, 2025

Choose a reason for hiding this comment

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

with this change we return an error after we have set the NetworkPoliciesInstalled status to true above even when only a part of the list of namespaces have the policies installed.

I would suggest to keep the current idea of keeping the NetworkPoliciesInstalled status to be false even when there is partial failure. Otherwise this gives the wrong impression to a regular user that all namespaces are network isolated according to the plan.

Copy link
Author

@greninja517 greninja517 Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@gourishkb , I was about to implement the same but here https://github.com/kubeslice/worker-operator/blob/master/api/v1beta1/slice_types.go#L131-L133, it is mentioned that slice.Status.NetworkPoliciesInstalled signifies at least one namespace has successfully NetworkPolicies installed. So, I have implemented the above approach.

Should I keep this slice.status.NetworkPoliciesInstalled value false even if there is partial failure ?

return fmt.Errorf("Failed to process some namespaces.Errors: %v", errors)
}
return nil
}

func (r *SliceReconciler) installSliceNetworkPolicyInAppNs(ctx context.Context, slice *kubeslicev1beta1.Slice, appNs string) error {
func (r *SliceReconciler) installSliceNetworkPolicyInAppNs(ctx context.Context, slice *kubeslicev1beta1.Slice, appNs string) (bool, error) {
log := r.Log.WithValues("type", "networkPolicy")

netPolicy := controllers.ContructNetworkPolicyObject(ctx, slice, appNs)
err := r.Update(ctx, netPolicy)
desiredNetPolicy := controllers.ContructNetworkPolicyObject(ctx, slice, appNs)

// Try to Get the Existing NetworkPolicy if exists
existingNetPolicy := &networkingv1.NetworkPolicy{}
netPolicyName := slice.Name + "-" + appNs
err := r.Get(ctx, types.NamespacedName{Name: netPolicyName, Namespace: appNs}, existingNetPolicy)
if err != nil {
if errors.IsNotFound(err) {
return r.Create(ctx, netPolicy)
} else {
return err
err := r.Create(ctx, desiredNetPolicy)
return err == nil, err
}
return false, err
}
log.Info("Updated network policy", "namespace", appNs)
return nil

// NetPolicy Exists, check if update is needed
if !equality.Semantic.DeepEqual(desiredNetPolicy.Spec, existingNetPolicy.Spec) {
existingNetPolicy.Spec = desiredNetPolicy.Spec
if err := r.Update(ctx, existingNetPolicy); err != nil {
return false, err
}
log.Info("Updated network policy", "namespace", appNs)
return true, nil
}
return false, nil
}

func (r *SliceReconciler) cleanupSliceNamespaces(ctx context.Context, slice *kubeslicev1beta1.Slice) error {
Expand Down