MGMT-4066 Add enabled operator CPU and memory requirements to host validation #1167

jordigilh · 2021-02-24T16:55:17Z

This PR updates the CPU and memory validations for the host to add the resources request for the enabled operators to check that the cluster's CPU and usable memory are sufficient to deploy the selected operators (LSO, CNV or OCS) in that node.

@machacekondra @masayag @pkliczewski @jakub-dzon @gamli75 can you take a look in the meanwhile?

/Jordi

app-sre-bot · 2021-02-24T16:55:19Z

Can one of the admins verify this patch?

jakub-dzon

I think that the spirit of the https://issues.redhat.com/browse/MGMT-4066 story was to take operator requirements into account while checking general OCP memory and cpu requirements in the host validation (like:

assisted-service/internal/host/refresh_status_preprocessor.go

Line 85 in 8fc0918

id: HasCPUCoresForRole,

or

assisted-service/internal/host/refresh_status_preprocessor.go

Line 90 in 8fc0918

id: HasMemoryForRole,

. using GetCPURequirementFor* and GetMemoryRequirementFor* Operator methods.

@gamli75 - is the above description correct?

jakub-dzon · 2021-02-25T07:52:20Z

internal/cluster/validator.go

+//     sum(Node usable memory) > sum (minimum memory required for each enabled operator)
+func (v *clusterValidator) sufficientMemoryAvailableForOperators(c *clusterPreprocessContext) validationStatus {
+
+	opMgr := operators.NewManager(v.log)


Please use the manager that's created in main.go- the OCS validator is stateful and that may in the future have impact on this code.

Thanks. Will do.

pkliczewski · 2021-02-25T12:18:01Z

internal/cluster/validator.go

+		return boolValue(true)
+	}
+
+	// gather the total memory usable available in the cluster


what do you think about extracting total cluster memory collection to a method?

I'll take a look at the code to see if this logic is being used elsewhere. Thanks for the suggestion 😃

Done: moved to its own struct method

pkliczewski · 2021-02-25T12:18:40Z

internal/cluster/validator.go

+
+	// gather the total CPU available in the cluster
+	var tca int64
+	for _, h := range c.cluster.Hosts {


moved as well.

machacekondra · 2021-02-25T12:48:30Z

internal/cluster/validation_id.go

 )

 func (v validationID) category() (string, error) {
 	switch v {
 	case IsMachineCidrDefined, isMachineCidrEqualsToCalculatedCidr, isApiVipDefined, isApiVipValid, isIngressVipDefined, isIngressVipValid,
 		isClusterCidrDefined, isServiceCidrDefined, noCidrOverlapping, networkPrefixValid, IsDNSDomainDefined, IsNtpServerConfigured:
 		return "network", nil
-	case AllHostsAreReadyToInstall, SufficientMastersCount:
+	case AllHostsAreReadyToInstall, SufficientMastersCount, SufficientMemoryAvailable, SufficientCPUAvailable:


Just wonder if this shouldn't be as part of "operators" instead of "hosts-data". Because it's actually operators related. But I would leave this decision to someone from assisted team.

Agreed. I'd also like to hear @gamli75 's feedback on this.

jordigilh · 2021-02-26T00:44:54Z

I think that the spirit of the https://issues.redhat.com/browse/MGMT-4066 story was to take operator requirements into account while checking general OCP memory and cpu requirements in the host validation (like:

assisted-service/internal/host/refresh_status_preprocessor.go

Line 85 in 8fc0918

id: HasCPUCoresForRole,

or

assisted-service/internal/host/refresh_status_preprocessor.go

Line 90 in 8fc0918

id: HasMemoryForRole,

. using GetCPURequirementFor* and GetMemoryRequirementFor* Operator methods.
@gamli75 - is the above description correct?

Agreed. I will refactor the code to align with this idea.

gamli75 · 2021-03-01T07:35:07Z

I think that the spirit of the https://issues.redhat.com/browse/MGMT-4066 story was to take operator requirements into account while checking general OCP memory and cpu requirements in the host validation (like:

assisted-service/internal/host/refresh_status_preprocessor.go

Line 85 in 8fc0918

id: HasCPUCoresForRole,

or

assisted-service/internal/host/refresh_status_preprocessor.go

Line 90 in 8fc0918

id: HasMemoryForRole,

. using GetCPURequirementFor* and GetMemoryRequirementFor* Operator methods.
@gamli75 - is the above description correct?

Yes, this is correct.

jordigilh · 2021-03-01T13:59:29Z

@avishayt What do you think about adding host disk requirement validation? OCS has such requirements and we can add them to the CPU and memory validations.
Update: after further deliberation, it makes no sense at the host level to validate disk availability for OCS. I will not be implementing such validation.

jordigilh · 2021-03-01T14:00:31Z

I think that the spirit of the https://issues.redhat.com/browse/MGMT-4066 story was to take operator requirements into account while checking general OCP memory and cpu requirements in the host validation (like:

assisted-service/internal/host/refresh_status_preprocessor.go

Line 85 in 8fc0918

id: HasCPUCoresForRole,

or

assisted-service/internal/host/refresh_status_preprocessor.go

Line 90 in 8fc0918

id: HasMemoryForRole,

. using GetCPURequirementFor* and GetMemoryRequirementFor* Operator methods.
@gamli75 - is the above description correct?

Yes, this is correct.

Understood, I will refactor the code to do the validation per host instead.

jordigilh · 2021-03-02T15:45:59Z

@priyanka19-98 can you please take a look at this PR?

One thing that I found puzzling is that the minimum memory validation for hosts uses the Physical memory (here and here), but OCS and I see CNV as well now, use the Usable memory instead. I think they should all use the usable memory but I leave it up to the maintainers to decide.

/cc @gamli75

jordigilh · 2021-03-02T15:47:07Z

@pkliczewski @jakub-dzon @machacekondra @masayag can you take another look?

priyanka19-98 · 2021-03-04T06:12:19Z

internal/operators/ocs/ocs_operator.go

+}
+
+// getHostCPURequirement returns the minimum amount of CPU core count the cluster must have to install the OCS operator
+func (o *ocsOperator) getPerHostCPURequirement(cluster *common.Cluster) (int64, error) {


The per host requirements are different and yet to be implemented. As we only perform aggregate resource validations,
for eg, if in compact mode (3 masters), its not compulsary that per host must have atleast 12 cpu, we rely on aggregate 36 cpu and can be present as 7,15,14.

What do you suggest at this point in time? We are implementing per host validation to ensure that all operators can be deployed based on the available hardware.

priyanka19-98 · 2021-03-04T06:13:39Z

internal/operators/ocs/ocs_operator.go

+	var cpu, count int64
+	switch depType {
+	case compact:
+		cpu = o.ocsValidatorConfig.OCSRequiredCompactModeCPUCount


The OCSRequiredCompactModeCPUCount , are aggregate resources, and not per host.

Since we're looking at per host validation on this PR, what do you suggest we should do for OCS at this point?

internal/operators/ocs/ocs_operator.go

pkliczewski · 2021-03-04T10:21:49Z

internal/host/hostutil/test_utils.go

+	return string(b)
+}
+
+func GenerateInventoryWithResources(cpu, memory int64, hostname string) string {


Where do we use this function?

It's being used to generate a host manifest for testing purposes in the /internal/host/transition_test.go test file

pkliczewski · 2021-03-04T10:26:58Z

internal/host/validator.go

-	default:
-		v.log.Errorf("Unexpected role %s", c.host.Role)
-		return ValidationError
+	if c.host.Role == models.HostRoleMaster || c.host.Role == models.HostRoleWorker || c.host.Role == models.HostRoleAutoAssign {


Why not to check whether c.host.Role != models.HostRoleBootstrap?

I was not successful at creating a test case with this role type, the logic didn't seem to support a state transition with bootstrap. It might be that there is a specific state in which it is supported, but after looking at the OCS validation code, I did not see any mention of the bootstrap as a type of role to check, so I dropped it altogether.

internal/host/validator.go

jordigilh · 2021-03-04T18:24:49Z

/test assisted-service-aws

openshift-ci-robot · 2021-03-04T18:25:01Z

@jordigilh: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

/test assisted-service-aws
/test e2e-metal-assisted
/test e2e-metal-assisted-ipv6
/test e2e-metal-assisted-onprem
/test e2e-metal-assisted-single-node
/test images
/test lint

Use /test all to run the following jobs:

pull-ci-openshift-assisted-service-master-assisted-service-aws
pull-ci-openshift-assisted-service-master-e2e-metal-assisted
pull-ci-openshift-assisted-service-master-images
pull-ci-openshift-assisted-service-master-lint

In response to this:

/retest assisted-service-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jordigilh · 2021-03-04T18:25:40Z

/test assisted-service-aws

jakub-dzon · 2021-03-09T08:04:50Z

internal/operators/api/api.go

 	// GetMemoryRequirementForMaster provides master memory requirements for the operator in MB
-	GetMemoryRequirementForMaster(ctx context.Context, cluster *common.Cluster) (int64, error)
+	GetMemoryRequirementForMaster(cluster *common.Cluster) (int64, error)


For consistency please either restore context parameter in the four methods above or remove it from GetDisksRequirement* methods

It seems that these methods will be used by an API endpoint (https://issues.redhat.com/browse/MGMT-4477), so context.Context parameter will be needed to allow for request-id-bound logging; please revert the removal of it.

Understood.

jordigilh · 2021-03-09T16:35:18Z

/test e2e-metal-assisted

jordigilh · 2021-03-09T17:46:47Z

/test assisted-service-aws

openshift-ci-robot · 2021-03-10T14:30:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gamli75, jordigilh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gamli75]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gamli75 · 2021-03-10T14:32:48Z

@jordigilh please talk with @lalon4 - we need to add e2e test cases for this validation when operators are enabled.

…t validation

gamli75 · 2021-03-11T07:17:28Z

/lgtm

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 24, 2021

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. api-review Categorizes an issue or PR as actively needing an API review. labels Feb 24, 2021

openshift-ci-robot requested review from empovit and omertuc February 24, 2021 16:55

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch from 15f8a5b to fc8fa9c Compare February 24, 2021 16:58

jakub-dzon reviewed Feb 25, 2021

View reviewed changes

pkliczewski reviewed Feb 25, 2021

View reviewed changes

machacekondra reviewed Feb 25, 2021

View reviewed changes

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch from fc8fa9c to 0f365e3 Compare February 27, 2021 23:27

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2021

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch 5 times, most recently from b44b1c1 to d6dae62 Compare February 28, 2021 18:00

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch from d6dae62 to 1a82b33 Compare March 2, 2021 15:28

openshift-ci-robot requested a review from gamli75 March 2, 2021 15:46

jordigilh changed the title ~~[WIP] MGMT-4066 Add enabled operator CPU and memory requirements to cluster validation~~ MGMT-4066 Add enabled operator CPU and memory requirements to cluster validation Mar 2, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 2, 2021

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch 2 times, most recently from 233d5ce to 47a8e90 Compare March 2, 2021 16:31

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2021

priyanka19-98 reviewed Mar 4, 2021

View reviewed changes

pkliczewski reviewed Mar 4, 2021

View reviewed changes

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch 2 times, most recently from 2c844a5 to bd052cd Compare March 4, 2021 16:52

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch from bd052cd to 6233a9f Compare March 5, 2021 14:46

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2021

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch from 6233a9f to 3ac034e Compare March 8, 2021 16:04

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2021

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch from 3ac034e to fd696be Compare March 8, 2021 17:35

jakub-dzon reviewed Mar 9, 2021

View reviewed changes

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch from fd696be to 165c230 Compare March 9, 2021 15:45

gamli75 approved these changes Mar 10, 2021

View reviewed changes

openshift-ci-robot assigned gamli75 Mar 10, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2021

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch from 165c230 to 7a2bd57 Compare March 10, 2021 16:37

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2021

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch from 7a2bd57 to cb38226 Compare March 10, 2021 16:38

MGMT-4066 Add enabled OLM operator CPU and memory requirements to hos…

6078837

…t validation

jordigilh force-pushed the MGMT-4066_add_operator_CPU_Memory_reqs_to_cluster_validation branch from cb38226 to 6078837 Compare March 10, 2021 16:44

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2021

openshift-merge-robot merged commit 709ddcf into openshift:master Mar 11, 2021

MGMT-4066 Add enabled operator CPU and memory requirements to host validation #1167

MGMT-4066 Add enabled operator CPU and memory requirements to host validation #1167

Conversation

jordigilh commented Feb 24, 2021 • edited

app-sre-bot commented Feb 24, 2021

jakub-dzon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordigilh commented Feb 26, 2021

gamli75 commented Mar 1, 2021

jordigilh commented Mar 1, 2021 • edited

jordigilh commented Mar 1, 2021

jordigilh commented Mar 2, 2021

jordigilh commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordigilh commented Mar 4, 2021 • edited

openshift-ci-robot commented Mar 4, 2021

jordigilh commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordigilh commented Mar 9, 2021

jordigilh commented Mar 9, 2021

openshift-ci-robot commented Mar 10, 2021

gamli75 commented Mar 10, 2021

gamli75 commented Mar 11, 2021

jordigilh commented Feb 24, 2021 •

edited

jordigilh commented Mar 1, 2021 •

edited

jordigilh commented Mar 4, 2021 •

edited