From 1db7898806b9125f435e81a82f350dc3b513be97 Mon Sep 17 00:00:00 2001 From: Andrei Ivashchenko Date: Mon, 18 Apr 2022 16:19:40 +0300 Subject: [PATCH 01/10] Requeue subnets and networks on failure --- controllers/network_controller.go | 61 +++++++++--- controllers/networkcounter_controller.go | 120 +++++++++++++++++++++++ controllers/subnet_controller.go | 49 +++++++++ 3 files changed, 219 insertions(+), 11 deletions(-) create mode 100644 controllers/networkcounter_controller.go diff --git a/controllers/network_controller.go b/controllers/network_controller.go index d6bd576..a77370a 100644 --- a/controllers/network_controller.go +++ b/controllers/network_controller.go @@ -36,14 +36,12 @@ import ( const ( CNetworkFinalizer = "network.ipam.onmetal.de/finalizer" - CVXLANCounterName = "k8s-vxlan-network-counter" - CGENEVECounterName = "k8s-geneve-network-counter" - CMPLSCounterName = "k8s-mpls-network-counter" - CNetworkIDProposalFailureReason = "NetworkIDProposalFailure" CNetworkIDReservationFailureReason = "NetworkIDReservationFailure" CNetworkIDReservationSuccessReason = "NetworkIDReservationSuccess" CNetworkIDReleaseSuccessReason = "NetworkIDReleaseSuccess" + + CFailedTopLevelSubnetIndexKey = "failedTopLevelSubnet" ) // NetworkReconciler reconciles a Network object @@ -64,13 +62,6 @@ type NetworkReconciler struct { // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. -// TODO(user): Modify the Reconcile function to compare the state specified by -// the Network object against the actual cluster state, and then -// perform operations to make the cluster state reflect the state specified by -// the user. -// -// For more details, check Reconcile and its Result here: -// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.7.0/pkg/reconcile func (r *NetworkReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := r.Log.WithValues("network", req.NamespacedName) @@ -125,6 +116,10 @@ func (r *NetworkReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct if network.Status.State == machinev1alpha1.CFinishedNetworkState || network.Status.State == machinev1alpha1.CFailedNetworkState { + if err := r.requeueFailedSubnets(ctx, log, network); err != nil { + log.Error(err, "unable to requeue top level subnets", "name", req.NamespacedName) + return ctrl.Result{}, err + } return ctrl.Result{}, nil } @@ -217,6 +212,28 @@ func (r *NetworkReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, nil } +func (r *NetworkReconciler) requeueFailedSubnets(ctx context.Context, log logr.Logger, network *machinev1alpha1.Network) error { + matchingFields := client.MatchingFields{ + CFailedTopLevelSubnetIndexKey: network.Name, + } + + subnets := &machinev1alpha1.SubnetList{} + if err := r.List(context.Background(), subnets, client.InNamespace(network.Namespace), matchingFields); err != nil { + log.Error(err, "unable to get connected top level subnets", "name", types.NamespacedName{Namespace: network.Namespace, Name: network.Name}) + return err + } + + for _, subnet := range subnets.Items { + subnet.Status.State = machinev1alpha1.CProcessingSubnetState + if err := r.Status().Update(ctx, &subnet); err != nil { + log.Error(err, "unable to update top level subnet", "name", types.NamespacedName{Namespace: network.Namespace, Name: network.Name}, "subnet", subnet.Name) + return err + } + } + + return nil +} + func (r *NetworkReconciler) finalizeNetwork(ctx context.Context, log logr.Logger, network *machinev1alpha1.Network) error { if network.Spec.Type == "" { return nil @@ -286,6 +303,28 @@ func (r *NetworkReconciler) typeToCounterName(networkType machinev1alpha1.Networ // SetupWithManager sets up the controller with the Manager. func (r *NetworkReconciler) SetupWithManager(mgr ctrl.Manager) error { + createFailedSubnetIndexValue := func(object client.Object) []string { + subnet, ok := object.(*machinev1alpha1.Subnet) + if !ok { + return nil + } + state := subnet.Status.State + parentNet := subnet.Spec.Network.Name + parentSubnet := subnet.Spec.ParentSubnet.Name + if parentSubnet != "" { + return nil + } + if state != machinev1alpha1.CFailedSubnetState { + return nil + } + return []string{parentNet} + } + + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), &machinev1alpha1.Subnet{}, CFailedTopLevelSubnetIndexKey, createFailedSubnetIndexValue); err != nil { + return err + } + r.EventRecorder = mgr.GetEventRecorderFor("network-controller") return ctrl.NewControllerManagedBy(mgr). For(&machinev1alpha1.Network{}). diff --git a/controllers/networkcounter_controller.go b/controllers/networkcounter_controller.go new file mode 100644 index 0000000..5b092ab --- /dev/null +++ b/controllers/networkcounter_controller.go @@ -0,0 +1,120 @@ +package controllers + +import ( + "context" + + "github.com/go-logr/logr" + machinev1alpha1 "github.com/onmetal/ipam/api/v1alpha1" + "github.com/pkg/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + CVXLANCounterName = "k8s-vxlan-network-counter" + CGENEVECounterName = "k8s-geneve-network-counter" + CMPLSCounterName = "k8s-mpls-network-counter" + + CFailedNetworkOfTypeIndexKey = "failedNetworkOfType" +) + +// NetworkCounterReconciler reconciles a NetworkCounter object +type NetworkCounterReconciler struct { + client.Client + Log logr.Logger + Scheme *runtime.Scheme + EventRecorder record.EventRecorder +} + +func (r *NetworkCounterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + log := r.Log.WithValues("machine", req.NamespacedName) + + nc := &machinev1alpha1.NetworkCounter{} + err := r.Get(ctx, req.NamespacedName, nc) + if apierrors.IsNotFound(err) { + log.Error(err, "requested machine resource not found", "name", req.NamespacedName) + return ctrl.Result{}, client.IgnoreNotFound(err) + } + if err != nil { + log.Error(err, "unable to get machine resource", "name", req.NamespacedName) + return ctrl.Result{}, err + } + + if nc.GetDeletionTimestamp() != nil { + return ctrl.Result{}, nil + } + + netType, err := r.counterNameToType(nc.Name) + if err != nil { + log.Error(err, "unknown network counter", "name", req.NamespacedName) + return ctrl.Result{}, err + } + + matchingFields := client.MatchingFields{ + CFailedNetworkOfTypeIndexKey: string(netType), + } + + networks := &machinev1alpha1.NetworkList{} + if err := r.List(context.Background(), networks, client.InNamespace(req.Namespace), matchingFields); err != nil { + log.Error(err, "unable to get connected networks", "name", req.NamespacedName) + return ctrl.Result{}, err + } + + for _, network := range networks.Items { + network.Status.State = machinev1alpha1.CProcessingNetworkState + if err := r.Status().Update(ctx, &network); err != nil { + log.Error(err, "unable to update network", "name", req.NamespacedName, "network", network.Name) + return ctrl.Result{}, err + } + } + + return ctrl.Result{}, nil +} + +// SetupWithManager sets up the controller with the Manager. +func (r *NetworkCounterReconciler) SetupWithManager(mgr ctrl.Manager) error { + createFailedNetworkOfTypeIndexValue := func(object client.Object) []string { + net, ok := object.(*machinev1alpha1.Network) + if !ok { + return nil + } + state := net.Status.State + netType := net.Spec.Type + if netType == "" { + return nil + } + if state != machinev1alpha1.CFailedNetworkState { + return nil + } + return []string{string(netType)} + } + + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), &machinev1alpha1.Network{}, CFailedNetworkOfTypeIndexKey, createFailedNetworkOfTypeIndexValue); err != nil { + return err + } + + r.EventRecorder = mgr.GetEventRecorderFor("networkcounter-controller") + return ctrl.NewControllerManagedBy(mgr). + For(&machinev1alpha1.NetworkCounter{}). + Complete(r) +} + +func (r *NetworkCounterReconciler) counterNameToType(name string) (machinev1alpha1.NetworkType, error) { + var counterType machinev1alpha1.NetworkType + switch name { + case CVXLANCounterName: + counterType = machinev1alpha1.CVXLANNetworkType + case CGENEVECounterName: + counterType = machinev1alpha1.CGENEVENetworkType + case CMPLSCounterName: + counterType = machinev1alpha1.CMPLSNetworkType + default: + return "", errors.Errorf("unknown network counter %s", name) + } + + return counterType, nil +} diff --git a/controllers/subnet_controller.go b/controllers/subnet_controller.go index 5023764..491a95a 100644 --- a/controllers/subnet_controller.go +++ b/controllers/subnet_controller.go @@ -48,6 +48,8 @@ const ( CChildSubnetReservationFailureReason = "ChildSubnetReservationFailure" CChildSubnetReservationSuccessReason = "ChildSubnetReservationSuccess" CChildSubnetReleaseSuccessReason = "ChildSubnetReleaseSuccess" + + CFailedChildSubnetIndexKey = "failedChildSubnet" ) // SubnetReconciler reconciles a Subnet object @@ -130,6 +132,10 @@ func (r *SubnetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // resource processing has been completed. if subnet.Status.State == v1alpha1.CFailedSubnetState || subnet.Status.State == v1alpha1.CFinishedSubnetState { + if err := r.requeueFailedSubnets(ctx, log, subnet); err != nil { + log.Error(err, "unable to requeue child subnets", "name", req.NamespacedName) + return ctrl.Result{}, err + } return ctrl.Result{}, nil } @@ -270,6 +276,27 @@ func (r *SubnetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // SetupWithManager sets up the controller with the Manager. func (r *SubnetReconciler) SetupWithManager(mgr ctrl.Manager) error { + createFailedSubnetIndexValue := func(object client.Object) []string { + subnet, ok := object.(*v1alpha1.Subnet) + if !ok { + return nil + } + state := subnet.Status.State + parentSubnet := subnet.Spec.ParentSubnet.Name + if parentSubnet == "" { + return nil + } + if state != v1alpha1.CFailedSubnetState { + return nil + } + return []string{parentSubnet} + } + + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), &v1alpha1.Network{}, CFailedChildSubnetIndexKey, createFailedSubnetIndexValue); err != nil { + return err + } + r.EventRecorder = mgr.GetEventRecorderFor("subnet-controller") return ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.Subnet{}). @@ -358,6 +385,28 @@ func (r *SubnetReconciler) finalizeSubnet(ctx context.Context, log logr.Logger, return nil } +func (r *SubnetReconciler) requeueFailedSubnets(ctx context.Context, log logr.Logger, subnet *v1alpha1.Subnet) error { + matchingFields := client.MatchingFields{ + CFailedChildSubnetIndexKey: subnet.Name, + } + + subnets := &v1alpha1.SubnetList{} + if err := r.List(context.Background(), subnets, client.InNamespace(subnet.Namespace), matchingFields); err != nil { + log.Error(err, "unable to get connected child subnets", "name", types.NamespacedName{Namespace: subnet.Namespace, Name: subnet.Name}) + return err + } + + for _, subnet := range subnets.Items { + subnet.Status.State = v1alpha1.CProcessingSubnetState + if err := r.Status().Update(ctx, &subnet); err != nil { + log.Error(err, "unable to update child subnet", "name", types.NamespacedName{Namespace: subnet.Namespace, Name: subnet.Name}, "subnet", subnet.Name) + return err + } + } + + return nil +} + func regionSubset(set []v1alpha1.Region, subset []v1alpha1.Region) error { nameSet := make([]string, len(set)) for i := range set { From d546475efe8359c6c524e8b641f99698ab1050a2 Mon Sep 17 00:00:00 2001 From: Andrei Ivashchenko Date: Mon, 18 Apr 2022 16:20:36 +0300 Subject: [PATCH 02/10] Add dependabot workflow --- .github/dependabot.yml | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 .github/dependabot.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..dce85fc --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,23 @@ +version: 2 +updates: + - package-ecosystem: "gomod" + directory: "/" + schedule: + interval: "daily" + open-pull-requests-limit: 10 + reviewers: + - aiivashchenko + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "daily" + open-pull-requests-limit: 10 + reviewers: + - aiivashchenko + - package-ecosystem: "docker" + directory: "/" + schedule: + interval: "daily" + open-pull-requests-limit: 10 + reviewers: + - aiivashchenko \ No newline at end of file From 66446f209023d46bc9de4ca8803a4c13c933903a Mon Sep 17 00:00:00 2001 From: Andrei Ivashchenko Date: Mon, 18 Apr 2022 16:35:04 +0300 Subject: [PATCH 03/10] Make regions definition for subnets non-mandatory --- api/v1alpha1/subnet_types.go | 18 ++++++++++-------- config/crd/bases/ipam.onmetal.de_subnets.yaml | 2 -- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/api/v1alpha1/subnet_types.go b/api/v1alpha1/subnet_types.go index 846db30..aab3677 100644 --- a/api/v1alpha1/subnet_types.go +++ b/api/v1alpha1/subnet_types.go @@ -46,9 +46,8 @@ type SubnetSpec struct { // +kubebuilder:validation:Required Network v1.LocalObjectReference `json:"network"` // Regions represents the network service location - // +kubebuilder:validation:Required - // +kubebuilder:validation:MinItems=1 - Regions []Region `json:"regions"` + // +kubebuilder:validation:Optional + Regions []Region `json:"regions,omitempty"` // Consumer refers to resource Subnet has been booked for // +kubebuilder:validation:Optional Consumer *ResourceReference `json:"consumer,omitempty"` @@ -151,13 +150,16 @@ func init() { func (in *Subnet) PopulateStatus() { in.Status.State = CProcessingSubnetState - // Validator checks that slice has at least one element, - // so it is safe to assume that there is an element on zero index. - // It is also okay to check AZ count only for first region, + regionCount := len(in.Spec.Regions) + if regionCount == 0 { + return + } + + // It is okay to check AZ count only for first region, // since if there is more than one region, it gets classified as - // multiregion subnet. + // multiregional subnet. azCount := len(in.Spec.Regions[0].AvailabilityZones) - regionCount := len(in.Spec.Regions) + if azCount == 1 && regionCount == 1 { in.Status.Locality = CLocalSubnetLocalityType } else if azCount > 1 && regionCount == 1 { diff --git a/config/crd/bases/ipam.onmetal.de_subnets.yaml b/config/crd/bases/ipam.onmetal.de_subnets.yaml index 592060b..98486eb 100644 --- a/config/crd/bases/ipam.onmetal.de_subnets.yaml +++ b/config/crd/bases/ipam.onmetal.de_subnets.yaml @@ -166,11 +166,9 @@ spec: - availabilityZones - name type: object - minItems: 1 type: array required: - network - - regions type: object status: description: SubnetStatus defines the observed state of Subnet From fd21b412896f8335f5bbef97e495b3c0bff003cd Mon Sep 17 00:00:00 2001 From: Andrei Ivashchenko Date: Mon, 18 Apr 2022 18:55:57 +0300 Subject: [PATCH 04/10] Add tests for NetworkCounter controller --- controllers/ip_controller_test.go | 14 +- controllers/network_controller_test.go | 27 +++ controllers/networkcounter_controller_test.go | 166 ++++++++++++++++++ controllers/subnet_controller_test.go | 16 ++ controllers/suite_test.go | 5 + main.go | 8 + 6 files changed, 233 insertions(+), 3 deletions(-) create mode 100644 controllers/networkcounter_controller_test.go diff --git a/controllers/ip_controller_test.go b/controllers/ip_controller_test.go index a79fbc8..f93e347 100644 --- a/controllers/ip_controller_test.go +++ b/controllers/ip_controller_test.go @@ -42,6 +42,14 @@ var _ = Describe("IP controller", func() { list client.ObjectList count func(client.ObjectList) int }{ + { + res: &v1alpha1.IP{}, + list: &v1alpha1.IPList{}, + count: func(objList client.ObjectList) int { + list := objList.(*v1alpha1.IPList) + return len(list.Items) + }, + }, { res: &v1alpha1.Subnet{}, list: &v1alpha1.SubnetList{}, @@ -59,10 +67,10 @@ var _ = Describe("IP controller", func() { }, }, { - res: &v1alpha1.IP{}, - list: &v1alpha1.IPList{}, + res: &v1alpha1.NetworkCounter{}, + list: &v1alpha1.NetworkCounterList{}, count: func(objList client.ObjectList) int { - list := objList.(*v1alpha1.IPList) + list := objList.(*v1alpha1.NetworkCounterList) return len(list.Items) }, }, diff --git a/controllers/network_controller_test.go b/controllers/network_controller_test.go index 941da6f..8af1259 100644 --- a/controllers/network_controller_test.go +++ b/controllers/network_controller_test.go @@ -37,6 +37,22 @@ var _ = Describe("Network controller", func() { list client.ObjectList count func(client.ObjectList) int }{ + { + res: &v1alpha1.IP{}, + list: &v1alpha1.IPList{}, + count: func(objList client.ObjectList) int { + list := objList.(*v1alpha1.IPList) + return len(list.Items) + }, + }, + { + res: &v1alpha1.Subnet{}, + list: &v1alpha1.SubnetList{}, + count: func(objList client.ObjectList) int { + list := objList.(*v1alpha1.SubnetList) + return len(list.Items) + }, + }, { res: &v1alpha1.Network{}, list: &v1alpha1.NetworkList{}, @@ -45,6 +61,14 @@ var _ = Describe("Network controller", func() { return len(list.Items) }, }, + { + res: &v1alpha1.NetworkCounter{}, + list: &v1alpha1.NetworkCounterList{}, + count: func(objList client.ObjectList) int { + list := objList.(*v1alpha1.NetworkCounterList) + return len(list.Items) + }, + }, } for _, r := range resources { @@ -185,6 +209,9 @@ var _ = Describe("Network controller", func() { return true }, timeout, interval).Should(BeTrue()) + By(fmt.Sprintf("%s network ID with the same ID deleted", testNetworkCase.network.Spec.Type)) + Expect(k8sClient.Delete(ctx, &testNetworkCopy)).Should(Succeed()) + By(fmt.Sprintf("%s network ID CR deleted", testNetworkCase.network.Spec.Type)) oldNetworkID := testNetwork.Status.Reserved.DeepCopy() Expect(k8sClient.Delete(ctx, testNetwork)).Should(Succeed()) diff --git a/controllers/networkcounter_controller_test.go b/controllers/networkcounter_controller_test.go new file mode 100644 index 0000000..400d084 --- /dev/null +++ b/controllers/networkcounter_controller_test.go @@ -0,0 +1,166 @@ +package controllers + +import ( + "context" + "time" + + "github.com/onmetal/ipam/api/v1alpha1" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var _ = Describe("NetworkCounter controller", func() { + const ( + NetworkCounterNamespace = "default" + NetworkCounterName = CVXLANCounterName + + NetworkName = "test-network" + + timeout = time.Second * 30 + interval = time.Millisecond * 250 + ) + + AfterEach(func() { + ctx := context.Background() + resources := []struct { + res client.Object + list client.ObjectList + count func(client.ObjectList) int + }{ + { + res: &v1alpha1.IP{}, + list: &v1alpha1.IPList{}, + count: func(objList client.ObjectList) int { + list := objList.(*v1alpha1.IPList) + return len(list.Items) + }, + }, + { + res: &v1alpha1.Subnet{}, + list: &v1alpha1.SubnetList{}, + count: func(objList client.ObjectList) int { + list := objList.(*v1alpha1.SubnetList) + return len(list.Items) + }, + }, + { + res: &v1alpha1.Network{}, + list: &v1alpha1.NetworkList{}, + count: func(objList client.ObjectList) int { + list := objList.(*v1alpha1.NetworkList) + return len(list.Items) + }, + }, + { + res: &v1alpha1.NetworkCounter{}, + list: &v1alpha1.NetworkCounterList{}, + count: func(objList client.ObjectList) int { + list := objList.(*v1alpha1.NetworkCounterList) + return len(list.Items) + }, + }, + } + + for _, r := range resources { + Expect(k8sClient.DeleteAllOf(ctx, r.res, client.InNamespace(NetworkCounterNamespace))).To(Succeed()) + Eventually(func() bool { + err := k8sClient.List(ctx, r.list) + if err != nil { + return false + } + if r.count(r.list) > 0 { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + } + }) + + Context("When network counter is updated", func() { + It("Should trigger update of failed networks", func() { + By("Counter is created") + ctx := context.Background() + + counterSpec := v1alpha1.NewNetworkCounterSpec(v1alpha1.CVXLANNetworkType) + counterSpec.Vacant[0].Begin = v1alpha1.NetworkIDFromInt64(101) + + networkCounter := v1alpha1.NetworkCounter{ + ObjectMeta: metav1.ObjectMeta{ + Name: NetworkCounterName, + Namespace: NetworkCounterNamespace, + }, + Spec: *counterSpec, + } + + Expect(k8sClient.Create(ctx, &networkCounter)).Should(Succeed()) + + By("Network is created") + network := v1alpha1.Network{ + ObjectMeta: metav1.ObjectMeta{ + Name: NetworkName, + Namespace: NetworkCounterNamespace, + }, + Spec: v1alpha1.NetworkSpec{ + Type: v1alpha1.CVXLANNetworkType, + ID: v1alpha1.NetworkIDFromInt64(100), + }, + } + + Expect(k8sClient.Create(ctx, &network)).Should(Succeed()) + + By("Network has failed state") + Eventually(func() bool { + networkNamespacedName := types.NamespacedName{ + Namespace: NetworkCounterNamespace, + Name: NetworkName, + } + updatedNetwork := &v1alpha1.Network{} + err := k8sClient.Get(ctx, networkNamespacedName, updatedNetwork) + if err != nil { + return false + } + if updatedNetwork.Status.State != v1alpha1.CFailedNetworkState { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + + By("Counter is updated") + networkCounterNamespacedName := types.NamespacedName{ + Namespace: NetworkCounterNamespace, + Name: NetworkCounterName, + } + updatedNetworkCounter := &v1alpha1.NetworkCounter{} + Expect(k8sClient.Get(ctx, networkCounterNamespacedName, updatedNetworkCounter)).Should(Succeed()) + + updatedNetworkCounter.Spec.Vacant[0].Begin = v1alpha1.NetworkIDFromInt64(100) + Expect(k8sClient.Update(ctx, updatedNetworkCounter)).Should(Succeed()) + + By("Network has ID assigned") + Eventually(func() bool { + networkNamespacedName := types.NamespacedName{ + Namespace: NetworkCounterNamespace, + Name: NetworkName, + } + updatedNetwork := &v1alpha1.Network{} + err := k8sClient.Get(ctx, networkNamespacedName, updatedNetwork) + if err != nil { + return false + } + if updatedNetwork.Status.State != v1alpha1.CFinishedNetworkState { + return false + } + if updatedNetwork.Status.Reserved == nil { + return false + } + if updatedNetwork.Status.Reserved.Int64() != 100 { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + }) + }) +}) diff --git a/controllers/subnet_controller_test.go b/controllers/subnet_controller_test.go index f146ba6..657e11c 100644 --- a/controllers/subnet_controller_test.go +++ b/controllers/subnet_controller_test.go @@ -35,6 +35,14 @@ var _ = Describe("Subnet controller", func() { list client.ObjectList count func(client.ObjectList) int }{ + { + res: &v1alpha1.IP{}, + list: &v1alpha1.IPList{}, + count: func(objList client.ObjectList) int { + list := objList.(*v1alpha1.IPList) + return len(list.Items) + }, + }, { res: &v1alpha1.Subnet{}, list: &v1alpha1.SubnetList{}, @@ -51,6 +59,14 @@ var _ = Describe("Subnet controller", func() { return len(list.Items) }, }, + { + res: &v1alpha1.NetworkCounter{}, + list: &v1alpha1.NetworkCounterList{}, + count: func(objList client.ObjectList) int { + list := objList.(*v1alpha1.NetworkCounterList) + return len(list.Items) + }, + }, } for _, r := range resources { diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 7a12edd..298b212 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -74,6 +74,11 @@ var _ = BeforeSuite(func() { }) Expect(err).ToNot(HaveOccurred()) + err = (&NetworkCounterReconciler{ + Client: k8sManager.GetClient(), + Log: ctrl.Log.WithName("controllers").WithName("NetworkCounter"), + }).SetupWithManager(k8sManager) + Expect(err).ToNot(HaveOccurred()) err = (&SubnetReconciler{ Client: k8sManager.GetClient(), Log: ctrl.Log.WithName("controllers").WithName("Subnet"), diff --git a/main.go b/main.go index 3a9eb27..c2bb3b3 100644 --- a/main.go +++ b/main.go @@ -78,6 +78,14 @@ func main() { os.Exit(1) } + if err = (&controllers.NetworkCounterReconciler{ + Client: mgr.GetClient(), + Log: ctrl.Log.WithName("controllers").WithName("NetworkCounter"), + Scheme: mgr.GetScheme(), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "NetworkCounter") + os.Exit(1) + } if err = (&controllers.NetworkReconciler{ Client: mgr.GetClient(), Log: ctrl.Log.WithName("controllers").WithName("Network"), From 4b98bc7ba490683aea91c7fdf6b3b28719c61d4d Mon Sep 17 00:00:00 2001 From: Andrei Ivashchenko Date: Mon, 18 Apr 2022 18:59:36 +0300 Subject: [PATCH 05/10] Add test for Subnet submission without regions --- api/v1alpha1/subnet_webhook_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/api/v1alpha1/subnet_webhook_test.go b/api/v1alpha1/subnet_webhook_test.go index c078c01..af78c0d 100644 --- a/api/v1alpha1/subnet_webhook_test.go +++ b/api/v1alpha1/subnet_webhook_test.go @@ -216,6 +216,21 @@ var _ = Describe("Subnet webhook", func() { Context("When Network is not created", func() { It("Should check that valid CR will be accepted", func() { crs := []Subnet{ + { + ObjectMeta: controllerruntime.ObjectMeta{ + Name: "without-regions", + Namespace: SubnetNamespace, + }, + Spec: SubnetSpec{ + CIDR: cidrMustParse("127.0.0.0/24"), + ParentSubnet: corev1.LocalObjectReference{ + Name: "parent-subnet", + }, + Network: corev1.LocalObjectReference{ + Name: "parent-net", + }, + }, + }, { ObjectMeta: controllerruntime.ObjectMeta{ Name: "with-cidr-rule", From 2a8e82ca31ad07f43df8c8928d0cd794d546be4c Mon Sep 17 00:00:00 2001 From: Andrei Ivashchenko Date: Mon, 18 Apr 2022 19:12:31 +0300 Subject: [PATCH 06/10] Requeue failed IPs --- controllers/subnet_controller.go | 50 +++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/controllers/subnet_controller.go b/controllers/subnet_controller.go index 491a95a..c9b13c5 100644 --- a/controllers/subnet_controller.go +++ b/controllers/subnet_controller.go @@ -50,6 +50,7 @@ const ( CChildSubnetReleaseSuccessReason = "ChildSubnetReleaseSuccess" CFailedChildSubnetIndexKey = "failedChildSubnet" + CFailedIPIndexKey = "failedIP" ) // SubnetReconciler reconciles a Subnet object @@ -136,6 +137,10 @@ func (r *SubnetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr log.Error(err, "unable to requeue child subnets", "name", req.NamespacedName) return ctrl.Result{}, err } + if err := r.requeueFailedIPs(ctx, log, subnet); err != nil { + log.Error(err, "unable to requeue child ips", "name", req.NamespacedName) + return ctrl.Result{}, err + } return ctrl.Result{}, nil } @@ -292,8 +297,29 @@ func (r *SubnetReconciler) SetupWithManager(mgr ctrl.Manager) error { return []string{parentSubnet} } + createFailedIPIndexValue := func(object client.Object) []string { + ip, ok := object.(*v1alpha1.IP) + if !ok { + return nil + } + state := ip.Status.State + parentSubnet := ip.Spec.Subnet.Name + if parentSubnet == "" { + return nil + } + if state != v1alpha1.CFailedIPState { + return nil + } + return []string{parentSubnet} + } + + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), &v1alpha1.Subnet{}, CFailedChildSubnetIndexKey, createFailedSubnetIndexValue); err != nil { + return err + } + if err := mgr.GetFieldIndexer().IndexField( - context.Background(), &v1alpha1.Network{}, CFailedChildSubnetIndexKey, createFailedSubnetIndexValue); err != nil { + context.Background(), &v1alpha1.IP{}, CFailedIPIndexKey, createFailedIPIndexValue); err != nil { return err } @@ -407,6 +433,28 @@ func (r *SubnetReconciler) requeueFailedSubnets(ctx context.Context, log logr.Lo return nil } +func (r *SubnetReconciler) requeueFailedIPs(ctx context.Context, log logr.Logger, subnet *v1alpha1.Subnet) error { + matchingFields := client.MatchingFields{ + CFailedIPIndexKey: subnet.Name, + } + + ips := &v1alpha1.IPList{} + if err := r.List(context.Background(), ips, client.InNamespace(subnet.Namespace), matchingFields); err != nil { + log.Error(err, "unable to get connected ips", "name", types.NamespacedName{Namespace: subnet.Namespace, Name: subnet.Name}) + return err + } + + for _, ip := range ips.Items { + ip.Status.State = v1alpha1.CProcessingIPState + if err := r.Status().Update(ctx, &ip); err != nil { + log.Error(err, "unable to update child ips", "name", types.NamespacedName{Namespace: subnet.Namespace, Name: subnet.Name}, "subnet", subnet.Name) + return err + } + } + + return nil +} + func regionSubset(set []v1alpha1.Region, subset []v1alpha1.Region) error { nameSet := make([]string, len(set)) for i := range set { From b642b1e36eddd198f93c90ae04be78c1a531f06b Mon Sep 17 00:00:00 2001 From: Andrei Ivashchenko Date: Tue, 19 Apr 2022 14:30:16 +0300 Subject: [PATCH 07/10] Add tests for IP and Subnet reconcile after failure --- controllers/ip_controller.go | 1 + controllers/ip_controller_test.go | 54 ++++++++++ controllers/subnet_controller_test.go | 138 +++++++++++++++++++++++++- 3 files changed, 188 insertions(+), 5 deletions(-) diff --git a/controllers/ip_controller.go b/controllers/ip_controller.go index 480144b..11d85ac 100644 --- a/controllers/ip_controller.go +++ b/controllers/ip_controller.go @@ -149,6 +149,7 @@ func (r *IPReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re return ctrl.Result{}, err } r.EventRecorder.Eventf(ip, v1.EventTypeWarning, CIPReservationFailureReason, ip.Status.Message) + return ctrl.Result{}, err } if err := r.Status().Update(ctx, &subnet); err != nil { diff --git a/controllers/ip_controller_test.go b/controllers/ip_controller_test.go index f93e347..0df82b4 100644 --- a/controllers/ip_controller_test.go +++ b/controllers/ip_controller_test.go @@ -2,6 +2,7 @@ package controllers import ( "context" + "fmt" "time" corev1 "k8s.io/api/core/v1" @@ -208,6 +209,34 @@ var _ = Describe("IP controller", func() { return len(createdSubnet.Status.Vacant) == 2 }, timeout, interval).Should(BeTrue()) + By("IP copy is created") + ipCopyName := IPName + "-copy" + ipCopyNamespacedName := types.NamespacedName{ + Namespace: Namespace, + Name: ipCopyName, + } + ipCopy := &v1alpha1.IP{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: Namespace, + Name: ipCopyName, + }, + Spec: *ip.Spec.DeepCopy(), + } + + Expect(k8sClient.Create(ctx, ipCopy)).Should(Succeed()) + + By("IP copy is failed to get reserved") + Eventually(func() bool { + err := k8sClient.Get(ctx, ipCopyNamespacedName, ipCopy) + if err != nil { + return false + } + if ipCopy.Status.State != v1alpha1.CFailedIPState { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + By("IP is deleted") Expect(k8sClient.Delete(ctx, ip)).Should(Succeed()) @@ -219,6 +248,31 @@ var _ = Describe("IP controller", func() { return true }, timeout, interval).Should(BeTrue()) + By("IP copy gets reserved") + Eventually(func() bool { + err := k8sClient.Get(ctx, ipCopyNamespacedName, ipCopy) + if err != nil { + return false + } + if ipCopy.Status.State != v1alpha1.CFinishedIPState { + return false + } + if !ipCopy.Status.Reserved.Equal(testIP) { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + + By("IP copy is deleted") + Expect(k8sClient.Delete(ctx, ipCopy)).To(Succeed()) + Eventually(func() bool { + err := k8sClient.Get(ctx, ipCopyNamespacedName, ipCopy) + if !apierrors.IsNotFound(err) { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + By("IP is released in subnet") Eventually(func() bool { err := k8sClient.Get(ctx, subnetNamespacedName, createdSubnet) diff --git a/controllers/subnet_controller_test.go b/controllers/subnet_controller_test.go index 657e11c..57e8e04 100644 --- a/controllers/subnet_controller_test.go +++ b/controllers/subnet_controller_test.go @@ -189,6 +189,36 @@ var _ = Describe("Subnet controller", func() { return false }()).To(BeTrue()) + By("Subnet copy is created") + subnetCopyName := createdSubnet.Name + "-copy" + subnetCopy := v1alpha1.Subnet{ + ObjectMeta: v1.ObjectMeta{ + Name: subnetCopyName, + Namespace: SubnetNamespace, + }, + Spec: *createdSubnet.Spec.DeepCopy(), + } + subnetCopy.Spec.CIDR = testCidr + + Expect(k8sClient.Create(ctx, &subnetCopy)).To(Succeed()) + + By("Subnet copy is failed to get CIDR reserved") + subnetCopyNamespacedName := types.NamespacedName{ + Namespace: SubnetNamespace, + Name: subnetCopyName, + } + + Eventually(func() bool { + err := k8sClient.Get(ctx, subnetCopyNamespacedName, &subnetCopy) + if err != nil { + return false + } + if subnetCopy.Status.State != v1alpha1.CFailedSubnetState { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + By("Subnet is deleted") Expect(k8sClient.Delete(ctx, &createdSubnet)).To(Succeed()) Eventually(func() bool { @@ -199,9 +229,42 @@ var _ = Describe("Subnet controller", func() { return true }, timeout, interval).Should(BeTrue()) + By("Subnet copy gets CIDR reserved") + Eventually(func() bool { + err := k8sClient.Get(ctx, subnetCopyNamespacedName, &subnetCopy) + if err != nil { + return false + } + if subnetCopy.Status.State != v1alpha1.CFinishedSubnetState { + return false + } + if !subnetCopy.Status.Reserved.Equal(testCidr) { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + + By("Subnet copy is deleted") + Expect(k8sClient.Delete(ctx, &subnetCopy)).To(Succeed()) + Eventually(func() bool { + err := k8sClient.Get(ctx, testSubnetNamespacedName, &subnetCopy) + if !apierrors.IsNotFound(err) { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + By("Subnet CIDR is released in Network") - Expect(k8sClient.Get(ctx, testNetworkNamespacedName, &createdNetwork)).To(Succeed()) - Expect(createdNetwork.Status.IPv4Ranges).To(HaveLen(0)) + Eventually(func() bool { + err := k8sClient.Get(ctx, testNetworkNamespacedName, &createdNetwork) + if err != nil { + return false + } + if len(createdNetwork.Status.IPv4Ranges) != 0 { + return false + } + return true + }, timeout, interval).Should(BeTrue()) }) }) @@ -337,6 +400,36 @@ var _ = Describe("Subnet controller", func() { Expect(createdParentSubnet.CanReserve(testCidr)).To(BeFalse()) Expect(createdParentSubnet.CanRelease(testCidr)).To(BeTrue()) + By("Subnet copy is created") + subnetCopyName := createdSubnet.Name + "-copy" + subnetCopy := v1alpha1.Subnet{ + ObjectMeta: v1.ObjectMeta{ + Name: subnetCopyName, + Namespace: SubnetNamespace, + }, + Spec: *createdSubnet.Spec.DeepCopy(), + } + subnetCopy.Spec.CIDR = testCidr + + Expect(k8sClient.Create(ctx, &subnetCopy)).To(Succeed()) + + By("Subnet copy is failed to get CIDR reserved") + subnetCopyNamespacedName := types.NamespacedName{ + Namespace: SubnetNamespace, + Name: subnetCopyName, + } + + Eventually(func() bool { + err := k8sClient.Get(ctx, subnetCopyNamespacedName, &subnetCopy) + if err != nil { + return false + } + if subnetCopy.Status.State != v1alpha1.CFailedSubnetState { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + By("Subnet is deleted") Expect(k8sClient.Delete(ctx, &createdSubnet)).To(Succeed()) Eventually(func() bool { @@ -347,10 +440,45 @@ var _ = Describe("Subnet controller", func() { return true }, timeout, interval).Should(BeTrue()) + By("Subnet copy gets CIDR reserved") + Eventually(func() bool { + err := k8sClient.Get(ctx, subnetCopyNamespacedName, &subnetCopy) + if err != nil { + return false + } + if subnetCopy.Status.State != v1alpha1.CFinishedSubnetState { + return false + } + if !subnetCopy.Status.Reserved.Equal(testCidr) { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + + By("Subnet copy is deleted") + Expect(k8sClient.Delete(ctx, &subnetCopy)).To(Succeed()) + Eventually(func() bool { + err := k8sClient.Get(ctx, testSubnetNamespacedName, &subnetCopy) + if !apierrors.IsNotFound(err) { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + By("Subnet CIDR is released in parent Subnet") - Expect(k8sClient.Get(ctx, testParentSubnetNamespacedName, &createdParentSubnet)).To(Succeed()) - Expect(createdParentSubnet.CanReserve(testCidr)).To(BeTrue()) - Expect(createdParentSubnet.CanRelease(testCidr)).To(BeFalse()) + Eventually(func() bool { + err := k8sClient.Get(ctx, testParentSubnetNamespacedName, &createdParentSubnet) + if err != nil { + return false + } + if !createdParentSubnet.CanReserve(testCidr) { + return false + } + if createdParentSubnet.CanRelease(testCidr) { + return false + } + return true + }, timeout, interval).Should(BeTrue()) }) }) From f87fe9a032334a6b6ab36c5858da8a6120a44df8 Mon Sep 17 00:00:00 2001 From: Andrei Ivashchenko Date: Tue, 19 Apr 2022 14:32:08 +0300 Subject: [PATCH 08/10] Update subnet CRD in Helm chart --- charts/ipam/templates/subnet-crd.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/charts/ipam/templates/subnet-crd.yaml b/charts/ipam/templates/subnet-crd.yaml index a19ee49..b411446 100644 --- a/charts/ipam/templates/subnet-crd.yaml +++ b/charts/ipam/templates/subnet-crd.yaml @@ -164,11 +164,9 @@ spec: - availabilityZones - name type: object - minItems: 1 type: array required: - network - - regions type: object status: description: SubnetStatus defines the observed state of Subnet From 05db8275eed5c31c69ccc7c0a1f6e5a167b723d6 Mon Sep 17 00:00:00 2001 From: Andrei Ivashchenko Date: Tue, 19 Apr 2022 14:45:24 +0300 Subject: [PATCH 09/10] Remove unused imports --- controllers/ip_controller_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controllers/ip_controller_test.go b/controllers/ip_controller_test.go index 0df82b4..091e163 100644 --- a/controllers/ip_controller_test.go +++ b/controllers/ip_controller_test.go @@ -2,7 +2,6 @@ package controllers import ( "context" - "fmt" "time" corev1 "k8s.io/api/core/v1" From 224637bc4d84a4954c206647a5e965c278e5b800 Mon Sep 17 00:00:00 2001 From: Andrei Ivashchenko Date: Tue, 19 Apr 2022 14:49:47 +0300 Subject: [PATCH 10/10] Increase timeout for golangci-lint --- .github/workflows/pipeline.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pipeline.yaml b/.github/workflows/pipeline.yaml index dfd073c..0459a33 100644 --- a/.github/workflows/pipeline.yaml +++ b/.github/workflows/pipeline.yaml @@ -42,7 +42,7 @@ jobs: uses: golangci/golangci-lint-action@v2 with: version: v1.43 - args: -e S1008 + args: -e S1008 --timeout 3m go-test: runs-on: [ self-hosted, Linux, X64 ]