Merge pull request #37 from onmetal/requeue-on-failure

Requeue on failure #27

.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

.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 ]

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 {

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",

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

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

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 {

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)
 				},
 			},
@@ -200,6 +208,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())
 
@@ -211,6 +247,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)

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/[email protected]/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{}).

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())