Skip to content

Commit

Permalink
Rework status Conditions for Globalnet resources
Browse files Browse the repository at this point in the history 
The status Conditions are appended to preserve the history however
this is contrary to the intended design where each condition type
should be unique and only the last status is preserved. Instead of
appending, use meta.SetStatusCondition. Also trim existing conditions
on upgrade to contain only the last one.

Fixes #2463

Signed-off-by: Automated Release <release@submariner.io>
  • Loading branch information
@tpantelis
Automated Release authored and tpantelis committed Jul 31, 2023
1 parent c72b94d commit 3b162b5
Show file tree
Hide file tree
Showing 9 changed files with 287 additions and 168 deletions.
22 changes: 21 additions & 1 deletion pkg/globalnet/controllers/base_controllers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -139,7 +140,7 @@ func (c *baseIPAllocationController) reserveAllocatedIPs(federator federate.Fede

conditions := util.ConditionsFromUnstructured(obj, "status", "conditions")

conditions = util.TryAppendCondition(conditions, &metav1.Condition{
meta.SetStatusCondition(&conditions, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "ReserveAllocatedIPsFailed",
Expand Down Expand Up @@ -278,3 +279,22 @@ func deleteEndpoints(namespace, name string,

return errors.Wrapf(err, "error deleting Endpoints %s/%s", namespace, name)
}

func trimAllocatedStatusCondition(conditions *[]metav1.Condition) {
last := -1

for i := len(*conditions) - 1; i > 0; i-- {
if (*conditions)[i].Type == string(submarinerv1.GlobalEgressIPAllocated) {
last = i
break
}
}

for i := 0; i < last; i++ {
if (*conditions)[i].Type == string(submarinerv1.GlobalEgressIPAllocated) {
*conditions = append((*conditions)[:i], (*conditions)[i+1:]...)
i--
last--
}
}
}
15 changes: 9 additions & 6 deletions pkg/globalnet/controllers/cluster_egressip_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/submariner-io/submariner/pkg/ipam"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/tools/cache"
Expand Down Expand Up @@ -134,6 +135,8 @@ func (c *clusterGlobalEgressIPController) process(from runtime.Object, numRequeu
case syncer.Create, syncer.Update:
prevStatus := clusterGlobalEgressIP.Status

trimAllocatedStatusCondition(&clusterGlobalEgressIP.Status.Conditions)

if !c.validate(numberOfIPs, clusterGlobalEgressIP) {
return checkStatusChanged(&prevStatus, &clusterGlobalEgressIP.Status, clusterGlobalEgressIP), false
}
Expand All @@ -150,7 +153,7 @@ func (c *clusterGlobalEgressIPController) process(from runtime.Object, numRequeu

func (c *clusterGlobalEgressIPController) validate(numberOfIPs int, egressIP *submarinerv1.ClusterGlobalEgressIP) bool {
if egressIP.Name != constants.ClusterGlobalEgressIPName {
egressIP.Status.Conditions = util.TryAppendCondition(egressIP.Status.Conditions, &metav1.Condition{
meta.SetStatusCondition(&egressIP.Status.Conditions, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "InvalidInstance",
Expand All @@ -162,7 +165,7 @@ func (c *clusterGlobalEgressIPController) validate(numberOfIPs int, egressIP *su
}

if numberOfIPs < 0 {
egressIP.Status.Conditions = util.TryAppendCondition(egressIP.Status.Conditions, &metav1.Condition{
meta.SetStatusCondition(&egressIP.Status.Conditions, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "InvalidInput",
Expand All @@ -173,7 +176,7 @@ func (c *clusterGlobalEgressIPController) validate(numberOfIPs int, egressIP *su
}

if numberOfIPs == 0 {
egressIP.Status.Conditions = util.TryAppendCondition(egressIP.Status.Conditions, &metav1.Condition{
meta.SetStatusCondition(&egressIP.Status.Conditions, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "ZeroInput",
Expand Down Expand Up @@ -250,7 +253,7 @@ func (c *clusterGlobalEgressIPController) allocateGlobalIPs(key string, numberOf
if err != nil {
logger.Errorf(err, "Error allocating IPs for %q", key)

status.Conditions = util.TryAppendCondition(status.Conditions, &metav1.Condition{
meta.SetStatusCondition(&status.Conditions, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "IPPoolAllocationFailed",
Expand All @@ -264,7 +267,7 @@ func (c *clusterGlobalEgressIPController) allocateGlobalIPs(key string, numberOf
if err != nil {
logger.Errorf(err, "Error programming egress IP table rules for %q", key)

status.Conditions = util.TryAppendCondition(status.Conditions, &metav1.Condition{
meta.SetStatusCondition(&status.Conditions, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "ProgramIPTableRulesFailed",
Expand All @@ -278,7 +281,7 @@ func (c *clusterGlobalEgressIPController) allocateGlobalIPs(key string, numberOf

metrics.RecordAllocateClusterGlobalEgressIPs(c.pool.GetCIDR(), numberOfIPs)

status.Conditions = util.TryAppendCondition(status.Conditions, &metav1.Condition{
meta.SetStatusCondition(&status.Conditions, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionTrue,
Reason: "Success",
Expand Down
72 changes: 48 additions & 24 deletions pkg/globalnet/controllers/cluster_egressip_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,14 @@ var _ = Describe("ClusterGlobalEgressIP controller", func() {
})

It("should reallocate the global IPs", func() {
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, *existing.Spec.NumberOfIPs, 1,
metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "ReserveAllocatedIPsFailed",
}, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionTrue,
})
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, *existing.Spec.NumberOfIPs, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "ReserveAllocatedIPsFailed",
}, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionTrue,
})

t.awaitIPTableRules(getGlobalEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName).AllocatedIPs...)
})
Expand All @@ -133,15 +132,14 @@ var _ = Describe("ClusterGlobalEgressIP controller", func() {
})

It("should reallocate the global IPs", func() {
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, *existing.Spec.NumberOfIPs, 0,
metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "ReserveAllocatedIPsFailed",
}, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionTrue,
})
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, *existing.Spec.NumberOfIPs, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "ReserveAllocatedIPsFailed",
}, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionTrue,
})

allocatedIPs := getGlobalEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName).AllocatedIPs
t.awaitIPTableRules(allocatedIPs...)
Expand All @@ -156,7 +154,7 @@ var _ = Describe("ClusterGlobalEgressIP controller", func() {
})

It("should add an appropriate Status condition", func() {
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, 0, 0, metav1.Condition{
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, 0, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "InvalidInput",
Expand All @@ -170,13 +168,39 @@ var _ = Describe("ClusterGlobalEgressIP controller", func() {
})

It("should add an appropriate Status condition", func() {
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, 0, 0, metav1.Condition{
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, 0, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "ZeroInput",
})
})
})

Context("with previously appended Status conditions", func() {
BeforeEach(func() {
existing := newClusterGlobalEgressIP(constants.ClusterGlobalEgressIPName, 1)
existing.Status.Conditions = []metav1.Condition{
{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "AppendedCondition1",
Message: "Should be removed",
},
{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "AppendedCondition2",
Message: "Should be replaced",
},
}

t.createClusterGlobalEgressIP(existing)
})

It("should trim the Status conditions", func() {
t.awaitClusterGlobalEgressIPStatusAllocated(1)
})
})
})

When("the well-known ClusterGlobalEgressIP is updated", func() {
Expand Down Expand Up @@ -232,7 +256,7 @@ var _ = Describe("ClusterGlobalEgressIP controller", func() {
})

It("should update the Status appropriately", func() {
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, 0, 0, metav1.Condition{
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, 0, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "ZeroInput",
Expand Down Expand Up @@ -266,7 +290,7 @@ var _ = Describe("ClusterGlobalEgressIP controller", func() {

It("should eventually reallocate the global IPs", func() {
t.awaitNoIPTableRules(existing.Status.AllocatedIPs...)
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, numberOfIPs, 0, metav1.Condition{
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, numberOfIPs, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "ProgramIPTableRulesFailed",
Expand All @@ -284,7 +308,7 @@ var _ = Describe("ClusterGlobalEgressIP controller", func() {
})

It("should add an appropriate Status condition", func() {
awaitStatusConditions(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, 0, metav1.Condition{
t.awaitStatusConditions(t.clusterGlobalEgressIPs, constants.ClusterGlobalEgressIPName, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "IPPoolAllocationFailed",
Expand Down Expand Up @@ -318,7 +342,7 @@ var _ = Describe("ClusterGlobalEgressIP controller", func() {
})

It("should not allocate the global IP", func() {
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, "other name", 0, 0, metav1.Condition{
t.awaitEgressIPStatus(t.clusterGlobalEgressIPs, "other name", 0, metav1.Condition{
Type: string(submarinerv1.GlobalEgressIPAllocated),
Status: metav1.ConditionFalse,
Reason: "InvalidInstance",
Expand Down
Loading

0 comments on commit 3b162b5

Please sign in to comment.