Merge pull request #277 from omersch381/fix_pool_update_job

Fix pool update job

Author: openshift-merge-bot[bot]

controllers/designate_controller.go

@@ -833,7 +833,6 @@ func (r *DesignateReconciler) reconcileNormal(ctx context.Context, instance *des
 	}
 
 	Log.Info("Bind configmap was created successfully")
-
 	if len(nsRecordsConfigMap.Data) > 0 && instance.Status.DesignateCentralReadyCount > 0 {
 		Log.Info("len(nsRecordsConfigMap.Data) > 0")
 		poolsYamlConfigMap := &corev1.ConfigMap{
@@ -844,11 +843,12 @@ func (r *DesignateReconciler) reconcileNormal(ctx context.Context, instance *des
 			},
 			Data: make(map[string]string),
 		}
-		Log.Info("before designate.GeneratePoolsYamlData")
-		poolsYaml, err := designate.GeneratePoolsYamlData(bindConfigMap.Data, mdnsConfigMap.Data, nsRecordsConfigMap.Data)
+
+		poolsYaml, poolsYamlHash, err := designate.GeneratePoolsYamlDataAndHash(bindConfigMap.Data, mdnsConfigMap.Data, nsRecordsConfigMap.Data)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
+
 		Log.Info(fmt.Sprintf("pools.yaml content is\n%v", poolsYaml))
 		updatedPoolsYaml := make(map[string]string)
 		updatedPoolsYaml[designate.PoolsYamlContent] = poolsYaml
@@ -862,40 +862,32 @@ func (r *DesignateReconciler) reconcileNormal(ctx context.Context, instance *des
 			Log.Info("Unable to create config map for pools.yaml file")
 			return ctrl.Result{}, err
 		}
-		configMaps := []interface{}{
-			poolsYamlConfigMap.Data,
-		}
 
-		poolsYamlsEnvVars := make(map[string]env.Setter)
-		_, changed, err := r.createHashOfInputHashes(ctx, instance, designate.PoolsYamlHash, poolsYamlsEnvVars, configMaps)
-		if err != nil {
-			return ctrl.Result{}, err
+		if instance.Status.Hash == nil {
+			instance.Status.Hash = make(map[string]string)
 		}
-		if changed {
-			Log.Info("PoolsYamlHash has changed, creating a pool update job")
-
-			var poolUpdateHash string
-			var ok bool
-			if poolUpdateHash, ok = instance.Status.Hash[designatev1beta1.PoolUpdateHash]; !ok {
-				instance.Status.Hash[designatev1beta1.PoolUpdateHash] = ""
-				poolUpdateHash = ""
+
+		oldHash := instance.Status.Hash[designatev1beta1.PoolUpdateHash]
+		if oldHash != poolsYamlHash {
+			Log.Info(fmt.Sprintf("Old poolsYamlHash %s is different than new poolsYamlHash %s.\nLaunching pool update job", oldHash, poolsYamlHash))
+
+			instance.Status.Hash[designatev1beta1.PoolUpdateHash] = poolsYamlHash
+			if err := r.Client.Status().Update(ctx, instance); err != nil {
+				return ctrl.Result{}, fmt.Errorf("failed to update status hash: %w", err)
 			}
 			jobDef := designate.PoolUpdateJob(instance, serviceLabels, serviceAnnotations)
 
 			Log.Info("Initializing pool update job")
+
 			poolUpdatejob := job.NewJob(
 				jobDef,
 				designatev1beta1.PoolUpdateHash,
 				instance.Spec.PreserveJobs,
 				time.Duration(15)*time.Second,
-				poolUpdateHash,
+				oldHash,
 			)
-			_, err = poolUpdatejob.DoJob(ctx, helper)
-			if err != nil {
-				return ctrl.Result{}, err
-			}
-			instance.Status.Hash[designatev1beta1.PoolUpdateHash] = poolUpdatejob.GetHash()
-			err = r.Client.Status().Update(ctx, instance)
+
+			_, err := poolUpdatejob.DoJob(ctx, helper)
 			if err != nil {
 				return ctrl.Result{}, err
 			}

pkg/designate/generate_bind9_pools_yaml.go

@@ -15,9 +15,12 @@ package designate
 
 import (
 	"bytes"
+	"crypto/sha256"
+	"encoding/hex"
 	"fmt"
 	"os"
 	"path"
+	"sort"
 	"text/template"
 
 	"gopkg.in/yaml.v2"
@@ -70,49 +73,73 @@ type CatalogZone struct {
 	Refresh int    `yaml:"refresh"`
 }
 
-func GeneratePoolsYamlData(BindMap, MdnsMap, NsRecordsMap map[string]string) (string, error) {
-	// Create ns_records
+// We sort all pool resources to get the correct hash every time
+func GeneratePoolsYamlDataAndHash(BindMap, MdnsMap, NsRecordsMap map[string]string) (string, string, error) {
 	nsRecords := []NSRecord{}
 	for _, data := range NsRecordsMap {
 		err := yaml.Unmarshal([]byte(data), &nsRecords)
 		if err != nil {
-			return "", fmt.Errorf("error unmarshalling yaml: %w", err)
+			return "", "", fmt.Errorf("error unmarshalling yaml: %w", err)
 		}
 	}
+	sort.Slice(nsRecords, func(i, j int) bool {
+		if nsRecords[i].Hostname != nsRecords[j].Hostname {
+			return nsRecords[i].Hostname < nsRecords[j].Hostname
+		}
+		return nsRecords[i].Priority < nsRecords[j].Priority
+	})
+
+	bindIPs := make([]string, 0, len(BindMap))
+	for _, ip := range BindMap {
+		bindIPs = append(bindIPs, ip)
+	}
+	sort.Strings(bindIPs)
 
-	// Create targets and nameservers
-	nameservers := []Nameserver{}
-	targets := []Target{}
-	rndcKeyNum := 0
+	masterHosts := make([]string, 0, len(MdnsMap))
+	for _, host := range MdnsMap {
+		masterHosts = append(masterHosts, host)
+	}
+	sort.Strings(masterHosts)
 
-	for _, bindIP := range BindMap {
-		nameservers = append(nameservers, Nameserver{
+	nameservers := make([]Nameserver, len(bindIPs))
+	for i, bindIP := range bindIPs {
+		nameservers[i] = Nameserver{
 			Host: bindIP,
 			Port: 53,
-		})
+		}
+	}
 
-		masters := []Master{}
-		for _, masterHost := range MdnsMap {
-			masters = append(masters, Master{
+	targets := make([]Target, len(bindIPs))
+	for i, bindIP := range bindIPs {
+		masters := make([]Master, len(masterHosts))
+		for j, masterHost := range masterHosts {
+			masters[j] = Master{
 				Host: masterHost,
 				Port: 5354,
-			})
+			}
 		}
 
-		target := Target{
+		targets[i] = Target{
 			Type:        "bind9",
-			Description: fmt.Sprintf("BIND9 Server %d (%s)", rndcKeyNum, bindIP),
+			Description: fmt.Sprintf("BIND9 Server %d (%s)", i, bindIP),
 			Masters:     masters,
 			Options: Options{
 				Host:        bindIP,
 				Port:        53,
 				RNDCHost:    bindIP,
 				RNDCPort:    953,
-				RNDCKeyFile: fmt.Sprintf("%s/%s-%d", RndcConfDir, DesignateRndcKey, rndcKeyNum),
+				RNDCKeyFile: fmt.Sprintf("%s/%s-%d", RndcConfDir, DesignateRndcKey, i),
 			},
 		}
-		targets = append(targets, target)
-		rndcKeyNum++
+	}
+
+	sort.Slice(targets, func(i, j int) bool {
+		return targets[i].Options.Host < targets[j].Options.Host
+	})
+
+	for i := range targets {
+		targets[i].Description = fmt.Sprintf("BIND9 Server %d (%s)", i, targets[i].Options.Host)
+		targets[i].Options.RNDCKeyFile = fmt.Sprintf("%s/%s-%d", RndcConfDir, DesignateRndcKey, i)
 	}
 
 	// Catalog zone is an optional section
@@ -132,25 +159,32 @@ func GeneratePoolsYamlData(BindMap, MdnsMap, NsRecordsMap map[string]string) (st
 		CatalogZone: nil, // set to catalogZone if this section should be presented
 	}
 
+	poolBytes, err := yaml.Marshal(pool)
+	if err != nil {
+		return "", "", fmt.Errorf("error marshalling pool for hash: %w", err)
+	}
+	hasher := sha256.New()
+	hasher.Write(poolBytes)
+	poolHash := hex.EncodeToString(hasher.Sum(nil))
+
 	opTemplates, err := util.GetTemplatesPath()
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 	poolsYamlPath := path.Join(opTemplates, PoolsYamlPath)
 	poolsYaml, err := os.ReadFile(poolsYamlPath)
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 	tmpl, err := template.New("pool").Parse(string(poolsYaml))
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
-
 	var buf bytes.Buffer
 	err = tmpl.Execute(&buf, pool)
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 
-	return buf.String(), nil
+	return buf.String(), poolHash, nil
 }

pkg/designate/pool_update.go

@@ -7,6 +7,7 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"time"
 )
 
 const (
@@ -85,9 +86,10 @@ func PoolUpdateJob(
 		},
 	)
 
+	jobName := fmt.Sprintf("%s-pool-update-%d", ServiceName, time.Now().Unix())
 	job := &batchv1.Job{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      ServiceName + "-pool-update",
+			Name:      jobName,
 			Namespace: instance.Namespace,
 			Labels:    labels,
 		},
@@ -101,7 +103,7 @@ func PoolUpdateJob(
 					ServiceAccountName: instance.RbacResourceName(),
 					Containers: []corev1.Container{
 						{
-							Name:  ServiceName + "-pool-update",
+							Name:  jobName,
 							Image: instance.Spec.DesignateCentral.ContainerImage,
 							Env: []corev1.EnvVar{
 								{

tests/functional/designate_controller_test.go

@@ -644,7 +644,6 @@ var _ = Describe("Designate controller", func() {
 			Expect(err).ToNot(HaveOccurred())
 
 			validate := validator.New()
-
 			for _, pool := range pools {
 				Expect(pool.Name).ToNot(BeEmpty(), "Pool name should not be an empty string")
 				Expect(pool.Description).ToNot(BeEmpty(), "Pool description should not be an empty string")
@@ -726,5 +725,24 @@ var _ = Describe("Designate controller", func() {
 				}
 			}
 		})
+
+		It("should create the same pools.yaml hash when provided the same designate configmaps", func() {
+			bindConfigMap := th.GetConfigMap(types.NamespacedName{
+				Name:      designate.BindPredIPConfigMap,
+				Namespace: namespace})
+			mdnsConfigMap := th.GetConfigMap(types.NamespacedName{
+				Name:      designate.MdnsPredIPConfigMap,
+				Namespace: namespace})
+			nsRecordsConfigMap := th.GetConfigMap(types.NamespacedName{
+				Name:      designate.NsRecordsConfigMap,
+				Namespace: namespace})
+			_, poolsYamlHash, err := designate.GeneratePoolsYamlDataAndHash(bindConfigMap.Data, mdnsConfigMap.Data, nsRecordsConfigMap.Data)
+			Expect(err).ToNot(HaveOccurred())
+			for i := 0; i < 10; i++ {
+				_, newPoolsYamlHash, err := designate.GeneratePoolsYamlDataAndHash(bindConfigMap.Data, mdnsConfigMap.Data, nsRecordsConfigMap.Data)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(poolsYamlHash).Should(Equal(newPoolsYamlHash))
+			}
+		})
 	})
 })