MGMT-4475: validations to consider effective role (#2587)

- role based validations now takes into account the
suggested role in case of auto-assign. This results
in timely failures in case the role to be assigned
is incompatible with the validation's logic

- role evaluation is now being done in its own function
within the monitoring instead of in RefreshStatus

- feature flag has been added to allow for shutting down
  the calculation

Author: slaviered

internal/cluster/common.go

@@ -144,7 +144,7 @@ func getKnownMastersNodesIds(c *common.Cluster, db *gorm.DB) ([]*strfmt.UUID, er
 
 	allowedStatuses := []string{models.HostStatusKnown, models.HostStatusPreparingForInstallation}
 	for _, host := range cluster.Hosts {
-		if host.Role == models.HostRoleMaster && funk.ContainsString(allowedStatuses, swag.StringValue(host.Status)) {
+		if common.GetEffectiveRole(host) == models.HostRoleMaster && funk.ContainsString(allowedStatuses, swag.StringValue(host.Status)) {
 			masterNodesIds = append(masterNodesIds, host.ID)
 		}
 	}
@@ -154,7 +154,7 @@ func getKnownMastersNodesIds(c *common.Cluster, db *gorm.DB) ([]*strfmt.UUID, er
 func NumberOfWorkers(c *common.Cluster) int {
 	num := 0
 	for _, host := range c.Hosts {
-		if host.Role != models.HostRoleWorker || *host.Status == models.HostStatusDisabled {
+		if common.GetEffectiveRole(host) != models.HostRoleWorker || *host.Status == models.HostStatusDisabled {
 			continue
 		}
 		num += 1
@@ -173,7 +173,7 @@ func MapWorkersHostsByStatus(c *common.Cluster) map[string][]*models.Host {
 func mapHostsByStatus(c *common.Cluster, role models.HostRole) map[string][]*models.Host {
 	hostMap := make(map[string][]*models.Host)
 	for _, host := range c.Hosts {
-		if role != "" && host.Role != role {
+		if role != "" && common.GetEffectiveRole(host) != role {
 			continue
 		}
 		if _, ok := hostMap[swag.StringValue(host.Status)]; ok {

internal/cluster/validator.go

@@ -380,12 +380,12 @@ func (v *clusterValidator) sufficientMastersCount(c *clusterPreprocessContext) V
 	candidates := make([]*models.Host, 0)
 
 	for _, host := range hosts {
-		switch role := host.Role; role {
+		switch role := common.GetEffectiveRole(host); role {
 		case models.HostRoleMaster:
-			//add pre-assigned master hosts to the masters list
+			//add pre-assigned/suggested master hosts to the masters list
 			masters = append(masters, host)
 		case models.HostRoleWorker:
-			//add pre-assigned worker hosts to the worker list
+			//add pre-assigned/suggested worker hosts to the worker list
 			workers = append(workers, host)
 		default:
 			//auto-assign hosts and other types go to the candidate list
@@ -397,7 +397,8 @@ func (v *clusterValidator) sufficientMastersCount(c *clusterPreprocessContext) V
 		//if allocated masters count is less than the desired count, find eligable hosts
 		//from the candidate pool to match the master count criteria, up to 3
 		if len(masters) < minMastersNeededForInstallation {
-			if isValid, err := v.hostAPI.IsValidMasterCandidate(h, c.cluster, c.db, v.log); isValid && err == nil {
+			candidate := *h
+			if isValid, err := v.hostAPI.IsValidMasterCandidate(&candidate, c.cluster, c.db, v.log); isValid && err == nil {
 				masters = append(masters, h)
 				continue
 			}

internal/common/common.go

@@ -99,6 +99,13 @@ func AreMastersSchedulable(cluster *Cluster) bool {
 	return swag.BoolValue(cluster.SchedulableMasters)
 }
 
+func GetEffectiveRole(host *models.Host) models.HostRole {
+	if host.Role == models.HostRoleAutoAssign && host.SuggestedRole != "" {
+		return host.SuggestedRole
+	}
+	return host.Role
+}
+
 func GetConsoleUrl(clusterName, baseDomain string) string {
 	return fmt.Sprintf("%s.%s.%s", consoleUrlPrefix, clusterName, baseDomain)
 }

internal/hardware/validator.go

@@ -168,7 +168,7 @@ func (v *validator) GetClusterHostRequirements(ctx context.Context, cluster *com
 		return nil, err
 	}
 
-	ocpRequirements, err := v.getOCPClusterHostRoleRequirementsForVersion(cluster, host.Role)
+	ocpRequirements, err := v.getOCPClusterHostRoleRequirementsForVersion(cluster, common.GetEffectiveRole(host))
 	if err != nil {
 		return nil, err
 	}

internal/host/common.go

@@ -100,7 +100,7 @@ func updateRole(log logrus.FieldLogger, h *models.Host, role models.HostRole, su
 		*h.ID, h.InfraEnvID, srcRole).Updates(fields).Error
 }
 
-func GetHostnameAndRoleByIP(ip string, hosts []*models.Host) (string, models.HostRole, error) {
+func GetHostnameAndEffectiveRoleByIP(ip string, hosts []*models.Host) (string, models.HostRole, error) {
 	for _, h := range hosts {
 		if h.Inventory == "" {
 			continue
@@ -117,7 +117,7 @@ func GetHostnameAndRoleByIP(ip string, hosts []*models.Host) (string, models.Hos
 					return "", "", err
 				}
 				if ip == parsedIP.String() {
-					return getRealHostname(h, inv), h.Role, nil
+					return getRealHostname(h, inv), common.GetEffectiveRole(h), nil
 				}
 			}
 		}

internal/host/common_test.go

@@ -18,7 +18,7 @@ type hostNetProfile struct {
 	ip       string
 }
 
-var _ = Describe("GetHostnameAndRoleByIP", func() {
+var _ = Describe("GetHostnameAndEffectiveRoleByIP", func() {
 
 	hostRolesIpv4 := []hostNetProfile{{role: models.HostRoleMaster, hostname: "master-0", ip: "1.2.3.1"}, {role: models.HostRoleWorker, hostname: "worker-0", ip: "1.2.3.2"}}
 	hostrolesIpv6 := []hostNetProfile{{role: models.HostRoleMaster, hostname: "master-1", ip: "1001:db8::11"}, {role: models.HostRoleWorker, hostname: "worker-1", ip: "1001:db8::12"}}
@@ -80,7 +80,7 @@ var _ = Describe("GetHostnameAndRoleByIP", func() {
 					h := hostutil.GenerateTestHostWithNetworkAddress(strfmt.UUID(uuid.New().String()), infraEnvID, clusterID, v.role, models.HostStatusKnown, netAddr)
 					hosts = append(hosts, h)
 				}
-				hostname, role, err := GetHostnameAndRoleByIP(test.targetIP, hosts)
+				hostname, role, err := GetHostnameAndEffectiveRoleByIP(test.targetIP, hosts)
 				if test.expectedError != nil {
 					Expect(err).To(Equal(test.expectedError))
 				} else {

internal/host/host.go

@@ -100,6 +100,7 @@ type LogTimeoutConfig struct {
 type Config struct {
 	LogTimeoutConfig
 	EnableAutoReset         bool                    `envconfig:"ENABLE_AUTO_RESET" default:"false"`
+	EnableAutoAssign        bool                    `envconfig:"ENABLE_AUTO_ASSIGN" default:"true"`
 	ResetTimeout            time.Duration           `envconfig:"RESET_CLUSTER_TIMEOUT" default:"3m"`
 	MonitorBatchSize        int                     `envconfig:"HOST_MONITOR_BATCH_SIZE" default:"100"`
 	DisabledHostvalidations DisabledHostValidations `envconfig:"DISABLED_HOST_VALIDATIONS" default:""` // Which host validations to disable (should not run in preprocess)
@@ -134,6 +135,7 @@ type API interface {
 	IsInstallable(h *models.Host) bool
 	// auto assign host role
 	AutoAssignRole(ctx context.Context, h *models.Host, db *gorm.DB) (bool, error)
+	RefreshRole(ctx context.Context, h *models.Host, db *gorm.DB) error
 	IsValidMasterCandidate(h *models.Host, c *common.Cluster, db *gorm.DB, log logrus.FieldLogger) (bool, error)
 	SetUploadLogsAt(ctx context.Context, h *models.Host, db *gorm.DB) error
 	UpdateLogsProgress(ctx context.Context, h *models.Host, progress string) error
@@ -375,6 +377,28 @@ func (m *Manager) updateInventory(ctx context.Context, cluster *common.Cluster,
 	}).Error
 }
 
+func (m *Manager) refreshRoleInternal(ctx context.Context, h *models.Host, db *gorm.DB, forceRefresh bool) error {
+	//update suggested role, if not yet set
+	var suggestedRole models.HostRole
+	var err error
+	if m.Config.EnableAutoAssign || forceRefresh {
+		//because of possible hw changes, suggested role should be calculated
+		//periodically even if the suggested role is already set
+		if h.Role == models.HostRoleAutoAssign &&
+			funk.ContainsString(hostStatusesBeforeInstallation[:], *h.Status) {
+			if suggestedRole, err = m.autoRoleSelection(ctx, h, db); err == nil {
+				if h.SuggestedRole != suggestedRole {
+					if err = updateRole(m.log, h, h.Role, suggestedRole, db, string(h.Role)); err == nil {
+						h.SuggestedRole = suggestedRole
+						m.log.Infof("suggested role for host %s is %s", *h.ID, suggestedRole)
+					}
+				}
+			}
+		}
+	}
+	return err
+}
+
 func (m *Manager) refreshStatusInternal(ctx context.Context, h *models.Host, c *common.Cluster, i *common.InfraEnv, db *gorm.DB) error {
 	if db == nil {
 		db = m.db
@@ -409,15 +433,6 @@ func (m *Manager) refreshStatusInternal(ctx context.Context, h *models.Host, c *
 		}
 	}
 
-	//update suggested role, if not yet set
-	var suggestedRole models.HostRole
-	if h.Role == models.HostRoleAutoAssign && h.SuggestedRole == models.HostRoleAutoAssign &&
-		funk.ContainsString(hostStatusesBeforeInstallation[:], *h.Status) {
-		if suggestedRole, err = m.autoRoleSelection(ctx, h, db); err == nil {
-			_ = updateRole(m.log, h, h.Role, suggestedRole, db, string(h.Role))
-		}
-	}
-
 	err = m.sm.Run(TransitionTypeRefresh, newStateHost(h), &TransitionArgsRefreshHost{
 		ctx:               ctx,
 		db:                db,
@@ -431,6 +446,13 @@ func (m *Manager) refreshStatusInternal(ctx context.Context, h *models.Host, c *
 	return nil
 }
 
+func (m *Manager) RefreshRole(ctx context.Context, h *models.Host, db *gorm.DB) error {
+	if db == nil {
+		db = m.db
+	}
+	return m.refreshRoleInternal(ctx, h, db, true)
+}
+
 func (m *Manager) RefreshStatus(ctx context.Context, h *models.Host, db *gorm.DB) error {
 	if db == nil {
 		db = m.db
@@ -1009,17 +1031,16 @@ func (m *Manager) AutoAssignRole(ctx context.Context, h *models.Host, db *gorm.D
 	if h.Role == models.HostRoleAutoAssign {
 		log := logutil.FromContext(ctx, m.log)
 		// If role is auto-assigned calculate the suggested roles
-		// This logic will moved to the monitor soon
-		suggestedRole, err := m.autoRoleSelection(ctx, h, db)
-		if err != nil {
+		// to make sure the suggestion is fresh
+		if err := m.RefreshRole(ctx, h, db); err != nil { //force refresh
 			return false, err
 		}
 
 		//copy the suggested role into the role and update the host record
-		log.Infof("suggested role %s for host %s cluster %s", suggestedRole, h.ID.String(), h.ClusterID.String())
-		if err := updateRole(m.log, h, suggestedRole, suggestedRole, db, string(models.HostRoleAutoAssign)); err != nil {
+		log.Infof("suggested role %s for host %s cluster %s", h.SuggestedRole, h.ID.String(), h.ClusterID.String())
+		if err := updateRole(m.log, h, h.SuggestedRole, h.SuggestedRole, db, string(models.HostRoleAutoAssign)); err != nil {
 			log.WithError(err).Errorf("failed to update role %s for host %s cluster %s",
-				suggestedRole, h.ID.String(), h.ClusterID.String())
+				h.SuggestedRole, h.ID.String(), h.ClusterID.String())
 			return true, err
 		}
 
@@ -1061,15 +1082,15 @@ func (m *Manager) selectRole(ctx context.Context, h *models.Host, db *gorm.DB) (
 			h.ID.String(), h.ClusterID.String())
 	}
 
-	// count already existing masters
-	mastersCount := 0
-	if err = db.Model(&models.Host{}).Where("cluster_id = ? and status != ? and role = ?",
-		h.ClusterID, models.HostStatusDisabled, models.HostRoleMaster).Count(&mastersCount).Error; err != nil {
+	// count already existing masters or hosts with suggested role of master
+	otherMastersCount := 0
+	if err = db.Model(&models.Host{}).Where("cluster_id = ? and id != ? and status != ? and (role = ? or suggested_role = ?)",
+		h.ClusterID, h.ID, models.HostStatusDisabled, models.HostRoleMaster, models.HostRoleMaster).Count(&otherMastersCount).Error; err != nil {
 		log.WithError(err).Errorf("failed to count masters in cluster %s", h.ClusterID.String())
 		return autoSelectedRole, err
 	}
 
-	if mastersCount < common.MinMasterHostsNeededForInstallation {
+	if otherMastersCount < common.MinMasterHostsNeededForInstallation {
 		h.Role = models.HostRoleMaster
 		vc, err = newValidationContext(h, nil, nil, db, m.hwValidator)
 		if err != nil {
@@ -1269,13 +1290,14 @@ func (m *Manager) captureConnectivityReportMetrics(ctx context.Context, openshif
 	}
 	for _, r := range connectivityReport.RemoteHosts {
 		for _, l3 := range r.L3Connectivity {
-			_, targetRole, err := GetHostnameAndRoleByIP(l3.RemoteIPAddress, hosts)
+			_, targetRole, err := GetHostnameAndEffectiveRoleByIP(l3.RemoteIPAddress, hosts)
 			if err != nil {
 				log.Warn(err)
 				continue
 			}
-			m.metricApi.NetworkLatencyBetweenHosts(openshiftVersion, h.Role, targetRole, l3.AverageRTTMs)
-			m.metricApi.PacketLossBetweenHosts(openshiftVersion, h.Role, targetRole, l3.PacketLossPercentage)
+			effectiveRole := common.GetEffectiveRole(h)
+			m.metricApi.NetworkLatencyBetweenHosts(openshiftVersion, effectiveRole, targetRole, l3.AverageRTTMs)
+			m.metricApi.PacketLossBetweenHosts(openshiftVersion, effectiveRole, targetRole, l3.PacketLossPercentage)
 		}
 	}
 }

internal/host/host_test.go

@@ -41,6 +41,7 @@ var (
 	defaultConfig                  = &Config{
 		ResetTimeout:            3 * time.Minute,
 		EnableAutoReset:         true,
+		EnableAutoAssign:        true,
 		MonitorBatchSize:        100,
 		DisabledHostvalidations: defaultDisabledHostValidations,
 	}

internal/host/hostcommands/install_cmd.go

@@ -102,7 +102,7 @@ func (i *installCmd) GetSteps(ctx context.Context, host *models.Host) ([]*models
 }
 
 func (i *installCmd) getFullInstallerCommand(cluster *common.Cluster, host *models.Host, infraEnv *common.InfraEnv, bootdevice string) (string, error) {
-	role := host.Role
+	role := common.GetEffectiveRole(host)
 	if host.Bootstrap {
 		role = models.HostRoleBootstrap
 	}

internal/host/hostcommands/logs_cmd.go

@@ -125,7 +125,7 @@ func (i *logsCmd) getNonBootstrapMastersIPsInHostCluster(ctx context.Context, ho
 func (i *logsCmd) getHostsIpsfromMachineCIDR(cluster common.Cluster) ([]string, error) {
 	var ips []string
 	for _, h := range cluster.Hosts {
-		if h.Bootstrap || h.Role == models.HostRoleWorker {
+		if h.Bootstrap || common.GetEffectiveRole(h) == models.HostRoleWorker {
 			continue
 		}
 		ip, err := network.GetPrimaryMachineCIDRIP(h, &cluster)

internal/host/hostrole_test.go

@@ -10,9 +10,7 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"github.com/openshift/assisted-service/internal/common"
-	eventgen "github.com/openshift/assisted-service/internal/common/events"
 	"github.com/openshift/assisted-service/internal/events"
-	"github.com/openshift/assisted-service/internal/events/eventstest"
 	"github.com/openshift/assisted-service/internal/hardware"
 	"github.com/openshift/assisted-service/internal/host/hostutil"
 	"github.com/openshift/assisted-service/internal/operators"
@@ -77,14 +75,12 @@ var _ = Describe("Suggested-Role on Refresh", func() {
 		inventory      string
 		srcState       string
 		suggested_role models.HostRole
-		eventType      *string
 	}{
 		{
 			name:           "insufficient worker memory --> suggested as worker",
 			inventory:      hostutil.GenerateInventoryWithResourcesWithBytes(4, conversions.MibToBytes(150), conversions.MibToBytes(150), "worker"),
 			srcState:       models.HostStatusDiscovering,
 			suggested_role: models.HostRoleWorker,
-			eventType:      &eventgen.HostStatusUpdatedEventName,
 		},
 		{
 			name:           "sufficient master memory --> suggested as master when masters < 3",
@@ -97,7 +93,6 @@ var _ = Describe("Suggested-Role on Refresh", func() {
 			inventory:      workerInventory(),
 			srcState:       models.HostStatusKnown,
 			suggested_role: models.HostRoleWorker,
-			eventType:      &eventgen.HostStatusUpdatedEventName,
 		},
 	}
 
@@ -114,15 +109,8 @@ var _ = Describe("Suggested-Role on Refresh", func() {
 			host.SuggestedRole = models.HostRoleAutoAssign
 			Expect(db.Create(&host).Error).ShouldNot(HaveOccurred())
 			mockDefaultClusterHostRequirements(mockHwValidator)
-			if t.eventType != nil {
-				mockEvents.EXPECT().SendHostEvent(gomock.Any(), eventstest.NewEventMatcher(
-					eventstest.WithNameMatcher(*t.eventType),
-					eventstest.WithHostIdMatcher(host.ID.String()),
-					eventstest.WithInfraEnvIdMatcher(host.InfraEnvID.String()),
-				))
-			}
 
-			err := hapi.RefreshStatus(ctx, &host, db)
+			err := hapi.RefreshRole(ctx, &host, db)
 			Expect(err).ToNot(HaveOccurred())
 
 			var resultHost models.Host

internal/host/hostutil/host_utils.go

@@ -144,7 +144,7 @@ func GetDiskByInstallationPath(disks []*models.Disk, installationPath string) *m
 }
 
 func IgnitionFileName(host *models.Host) string {
-	return fmt.Sprintf("%s-%s.ign", host.Role, host.ID)
+	return fmt.Sprintf("%s-%s.ign", common.GetEffectiveRole(host), host.ID)
 }
 
 var allowedFlags = []string{"--append-karg", "--delete-karg", "-n", "--copy-network", "--network-dir", "--save-partlabel", "--save-partindex", "--image-url"}

internal/host/mock_host_api.go

@@ -80,6 +80,13 @@ func (m *Manager) clusterHostMonitoring() int64 {
 					if err != nil {
 						log.WithError(err).Errorf("failed to refresh host %s state", *host.ID)
 					}
+					//the refreshed role will be taken into account
+					//on the next monitor cycle. The force flag is a workaround
+					//until the feature flag is removed
+					err = m.refreshRoleInternal(ctx, host, m.db, false)
+					if err != nil {
+						log.WithError(err).Errorf("failed to refresh host %s role", *host.ID)
+					}
 				}
 			}
 		}