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.

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

Jump to bottom

Reconcile ClusterCIDRs with Finalizers #37

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Reconcile ClusterCIDRs with Finalizers #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dbbf605
Add check that finalizer is added to a newly created ClusterCIDR.
mneverov Sep 26, 2024
3b62a51
Fix log message: different messages for update and create.
mneverov Sep 26, 2024
20f9fad
go mod tidy
mneverov Sep 26, 2024
708c9c4
Always reconcile ClusterCIDRs even if a finalizer is present. Do not …
mneverov Sep 26, 2024
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
2 changes: 0 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -824,8 +824,6 @@ golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.2.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.4.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.14.0 h1:Vz7Qs629MkJkGyHxUlRHizWJRG2j8fbQKjELVSNhy7Q=
golang.org/x/sys v0.14.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws=
golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
Expand Down
23 changes: 14 additions & 9 deletions pkg/controller/ipam/multi_cidr_range_allocator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"fmt"
"math"
"net"
"slices"
"sync"
"time"

Expand Down Expand Up @@ -1090,12 +1091,10 @@ func (r *multiCIDRRangeAllocator) reconcileCreate(ctx context.Context, clusterCI
defer r.lock.Unlock()

logger := klog.FromContext(ctx)
if needToAddFinalizer(clusterCIDR, clusterCIDRFinalizer) {
Copy link
Contributor

@aojea aojea Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the problem here is that we were doing the assumption that "finalizer" was the indicator to create or update.

Independently, before Create or Update, we need to check the new object we are building against the old object (this is missing), and then decide if we want to create or update. Regarding PATCH vs PUT, since these objects are only managed by this controller entirely, we can use PUT to move the object to the state we just built

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding is since ClusterCIDR has all fields marked as immutable the only update we do is we set the finalizer.
We already check if the finalizer exists. Previously, we skipped the reconciliation and adding a ClusterCIDR to the cidrMap in this case that caused the bug.
With the current fix the finalizer is always added (if needed). We create a ClusterCIDR only when it does not have the ResourceVersion which seems to me a better indicator if the resource has already been processed and stored in etcd.
I understand that it is not the best approach and would maybe add a tuple to the queue: namespaced name and operation, such that we would know exact operation and would process the CIDR accordingly.
Can provide a PoC with the latter approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

apologize because I only skimmed through the code and my comment can be inaccurate.

I think this is ok, just wanted to say that reconcilation seems to depend on more things than the finalizer, hence, we need to process the entire object.

I understand that it is not the best approach and would maybe add a tuple to the queue: namespaced name and operation, such that we would know exact operation and would process the CIDR accordingly.
Can provide a PoC with the latter approach.

that will make this an event based instead of level based controller, we don't want to do that.

What I try to mean, is that when you need to Update, you have the old and the new object, so you can compare them and decide if you want to update or not.

logger.V(3).Info("Creating ClusterCIDR", "clusterCIDR", clusterCIDR.Name)
if err := r.createClusterCIDR(ctx, clusterCIDR, false); err != nil {
logger.Error(err, "Unable to create ClusterCIDR", "clusterCIDR", clusterCIDR.Name)
return err
}
logger.V(3).Info("Reconciling ClusterCIDR", "clusterCIDR", clusterCIDR.Name)
if err := r.createClusterCIDR(ctx, clusterCIDR, false); err != nil {
logger.Error(err, "failed to reconcile ClusterCIDR", "clusterCIDR", clusterCIDR.Name)
return err
}
return nil
}
Expand Down Expand Up @@ -1154,13 +1153,13 @@ func (r *multiCIDRRangeAllocator) createClusterCIDR(ctx context.Context, cluster
if updatedClusterCIDR.ResourceVersion == "" {
// Create is only used for creating default ClusterCIDR.
if _, err := r.networkClient.Create(ctx, updatedClusterCIDR, metav1.CreateOptions{}); err != nil {
logger.V(2).Info("Error creating ClusterCIDR", "clusterCIDR", klog.KObj(clusterCIDR), "err", err)
logger.V(2).Info("failed to create ClusterCIDR", "clusterCIDR", klog.KObj(clusterCIDR), "err", err)
return err
}
} else {
// Update the ClusterCIDR object when called from reconcileCreate.
if _, err := r.networkClient.Update(ctx, updatedClusterCIDR, metav1.UpdateOptions{}); err != nil {
logger.V(2).Info("Error creating ClusterCIDR", "clusterCIDR", clusterCIDR.Name, "err", err)
logger.V(2).Info("failed to update ClusterCIDR", "clusterCIDR", clusterCIDR.Name, "err", err)
return err
}
}
Expand Down Expand Up @@ -1208,7 +1207,13 @@ func (r *multiCIDRRangeAllocator) mapClusterCIDRSet(cidrMap map[string][]*cidrse
}

if clusterCIDRSetList, ok := cidrMap[nodeSelector]; ok {
cidrMap[nodeSelector] = append(clusterCIDRSetList, clusterCIDRSet)
containsClusterCIDRSet := slices.ContainsFunc(clusterCIDRSetList, func(c *cidrset.ClusterCIDR) bool {
return clusterCIDRSet.Name == c.Name
})

if !containsClusterCIDRSet {
cidrMap[nodeSelector] = append(clusterCIDRSetList, clusterCIDRSet)
}
} else {
cidrMap[nodeSelector] = []*cidrset.ClusterCIDR{clusterCIDRSet}
}
Expand Down
29 changes: 17 additions & 12 deletions pkg/controller/ipam/multi_cidr_range_allocator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1693,6 +1693,7 @@ func TestClusterCIDRDefault(t *testing.T) {
createdCCC, err := client.NetworkingV1().ClusterCIDRs().Get(context.TODO(), defaultClusterCIDRName, metav1.GetOptions{})
assert.Nil(t, err, "Expected no error getting clustercidr objects")
assert.Equal(t, defaultCCC.Spec, createdCCC.Spec)
assert.Contains(t, createdCCC.GetFinalizers(), clusterCIDRFinalizer)
}

// Ensure SyncClusterCIDR creates a new valid ClusterCIDR.
Expand Down Expand Up @@ -1772,19 +1773,23 @@ func TestSyncClusterCIDRCreate(t *testing.T) {
_, ctx := ktesting.NewTestContext(t)
client, cccController := newController(ctx)
for _, tc := range tests {
cccController.clusterCIDRStore.Add(tc.ccc)
err := cccController.syncClusterCIDR(ctx, tc.ccc.Name)
if tc.wantErr {
assert.Error(t, err)
continue
}
assert.NoError(t, err)
expectActions(t, client.Actions(), 1, "create", "clustercidrs")
t.Run(tc.name, func(t *testing.T) {
cccController.clusterCIDRStore.Add(tc.ccc)
err := cccController.syncClusterCIDR(ctx, tc.ccc.Name)

if tc.wantErr {
assert.Error(t, err)
return
}

createdCCC, err := client.NetworkingV1().ClusterCIDRs().Get(context.TODO(), tc.ccc.Name, metav1.GetOptions{})
require.NoError(t, err, "Expected no error getting clustercidr object")
assert.Equal(t, tc.ccc.Spec, createdCCC.Spec)
assert.Equal(t, []string{clusterCIDRFinalizer}, createdCCC.Finalizers)
assert.NoError(t, err)
expectActions(t, client.Actions(), 1, "create", "clustercidrs")

createdCCC, err := client.NetworkingV1().ClusterCIDRs().Get(context.TODO(), tc.ccc.Name, metav1.GetOptions{})
require.NoError(t, err, "Expected no error getting clustercidr object")
assert.Equal(t, tc.ccc.Spec, createdCCC.Spec)
assert.Equal(t, []string{clusterCIDRFinalizer}, createdCCC.Finalizers)
})
}
}

Expand Down
Loading