Bug 1820300: Extend oVirt's MachinePool #3399

rgolangh · 2020-04-03T00:38:32Z

Those items are now part of MachinePool

instance type
cpu (cores, sockets)
memory
os disk size
vm type

A compute pool snippet from the install-config.yaml may look like that:

compute:
- architecture: amd64
  hyperthreading: Enabled
  name: worker
  platform:
    ovirt:
      cpu:
        cores: 8
        sockets: 1
        instanceTypeID:
      memoryMB: 65536
      osDisk:
        sizeGB: 100
      type: high_performance
  replicas: 3

Terraform now uses those values, and all the defaults in tf files are
now deleted in favour of the machine-config ones.

openshift-ci-robot · 2020-04-03T00:38:34Z

@rgolangh: This pull request references Bugzilla bug 1813741, which is invalid:

expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is ON_QA instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1813741: Extend oVirt's MachinePool

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.

openshift-ci-robot · 2020-04-03T00:39:46Z

@rgolangh: This pull request references Bugzilla bug 1820300, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1820300: Extend oVirt's MachinePool

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.

rgolangh · 2020-04-03T00:39:58Z

/bugzilla refresh

openshift-ci-robot · 2020-04-03T00:40:00Z

@rgolangh: This pull request references Bugzilla bug 1820300, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

rgolangh · 2020-04-03T00:40:29Z

/bugzilla refresh

openshift-ci-robot · 2020-04-03T00:40:31Z

@rgolangh: This pull request references Bugzilla bug 1820300, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

rgolangh · 2020-04-03T00:41:11Z

/bugzilla refresh

openshift-ci-robot · 2020-04-03T00:41:13Z

@rgolangh: This pull request references Bugzilla bug 1820300, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

rgolangh · 2020-04-03T00:44:28Z

/bugzilla refresh

openshift-ci-robot · 2020-04-03T00:44:34Z

@rgolangh: This pull request references Bugzilla bug 1820300, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

openshift-ci-robot · 2020-04-03T07:38:56Z

@rgolangh: This pull request references Bugzilla bug 1820300, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1820300: Extend oVirt's MachinePool

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.

rgolangh · 2020-04-05T14:16:47Z

/retest

patrickdillon

This LGTM. There are just some docs issues that need to be fixed up. oVirt is missing a doc like this: https://github.com/openshift/installer/blob/master/docs/user/aws/customization.md and it should be added to give machinepool examples.

patrickdillon · 2020-05-01T17:54:42Z

data/data/ovirt/masters/variables.tf

  type = string
 }

-variable "ovirt_master_cpu" {
+variable "ovirt_master_os_disk_size_gb" {
  type = string
 }


nit: no newline EOF

patrickdillon · 2020-05-01T17:55:03Z

data/data/ovirt/template/variables.tf

-variable "disk_size_gib" {
-  type        = number
-  description = "The size of the template disk for worker/nodes in GiB."
-}


nit: no new line EOF

i can still see the newline :P

patrickdillon · 2020-05-01T17:56:16Z

data/data/ovirt/variables-ovirt.tf

+
+variable "ovirt_master_instance_type_id" {
+  type = string
+}


nit: no new line EOF

patrickdillon · 2020-05-01T17:56:42Z

go.mod

@@ -69,12 +69,12 @@ require (
 	github.com/openshift/cluster-api v0.0.0-20191129101638-b09907ac6668
 	github.com/openshift/cluster-api-provider-gcp v0.0.1-0.20200120152131-1b09fd9e7156
 	github.com/openshift/cluster-api-provider-libvirt v0.2.1-0.20191219173431-2336783d4603
-	github.com/openshift/cluster-api-provider-ovirt v0.1.1-0.20200128081049-840376ca5c09
+	github.com/openshift/cluster-api-provider-ovirt v0.1.1-0.20200402173228-4981fa21d0d7


I think these changes should be in the vendor commit.

patrickdillon · 2020-05-01T18:13:30Z

pkg/types/ovirt/machinepool.go

+	// InstanceTypeID defines the VM instance type and overrides
+	// the hardware parameters of the created VM, including cpu and memory.
+	// If InstanceTypeID is passed, all memory and cpu variables will be ignored.
+	InstanceTypeID string `json:"instanceTypeID,omitempty"`


Can you mark these with +optional and kubebuilder default? See this for details https://github.com/openshift/installer/pull/3515/files#diff-ef5ad052319246b6e805b03c0221dcdcR34 and check out the whole PR for motivation.

If InstanceTypeID is passed, all memory and cpu variables will be ignored.

I think we need validation that user doesn't set these conflicting values in install-config.yaml

patrickdillon · 2020-05-01T18:14:23Z

pkg/types/ovirt/machinepool.go

-// Set sets the values from `required` to `a`.
-func (l *MachinePool) Set(required *MachinePool) {
-	if required == nil || l == nil {
+// Disk defines a VM disk


nit: missing period.

patrickdillon · 2020-05-01T18:14:40Z

pkg/types/ovirt/machinepool.go

-	if required == nil || l == nil {
+// Disk defines a VM disk
+type Disk struct {
+	// SizeGB size of the bootable disk in GiB.


nit: missing is

patrickdillon · 2020-05-01T18:23:35Z

pkg/types/ovirt/machinepool.go

+	// be used for and this effects the instance parameters.
+	// One of "desktop, server, high_performance"


effects -> affects

One of "desktop, server, high_performance" -> One of: "desktop", "server", or "high_performance".

patrickdillon · 2020-05-01T18:24:09Z

pkg/types/ovirt/machinepool.go

+	VMType string `json:"type,omitempty"`
+}
+
+// CPU defines the VM cpu, made of (Sockets * Cores * Threads)


nit: missing period

Suggested change

// CPU defines the VM cpu, made of (Sockets * Cores * Threads)

// CPU defines the VM's cpu, which is made of (Sockets * Cores * Threads)

The sockets and cores are defined in this struct, while there is no mention of the threads?

I'm hiding threads for now, I don't want to expose this at the moment. I removed it from the comments as well.

patrickdillon · 2020-05-01T18:24:17Z

pkg/types/ovirt/machinepool.go

+// CPU defines the VM cpu, made of (Sockets * Cores * Threads)
+type CPU struct {
+	// Sockets is the number of sockets for a VM.
+	// Total CPUs is (Sockets * Cores * Threads)


nit: missing period

abhinavdahiya · 2020-05-01T19:46:26Z

pkg/asset/machines/master.go

@@ -319,7 +319,11 @@ func (m *Master) Generate(dependencies asset.Parents) error {
 		}
 	case ovirttypes.Name:
 		mpool := defaultOvirtMachinePoolPlatform()
+		mpool.VMType = "high_performance"


nit: move this to defaultOvirtMachinePoolPlatform

or document why this is different from the default.

abhinavdahiya · 2020-05-01T19:50:04Z

pkg/types/ovirt/machinepool.go

+	// VMType defines the workload type the instance will
+	// be used for and this effects the instance parameters.
+	// One of "desktop, server, high_performance"
+	VMType string `json:"type,omitempty"`


if these values are known, desktop, server, high_performance, we should be using string enums like the publish

installer/pkg/types/installconfig.go

Lines 47 to 55 in 1a0bf0a

// PublishingStrategy is a strategy for how various endpoints for the cluster are exposed.

type PublishingStrategy string

const (

// ExternalPublishingStrategy exposes endpoints for the cluster to the Internet.

ExternalPublishingStrategy PublishingStrategy = "External"

// InternalPublishingStrategy exposes the endpoints for the cluster to the private network only.

InternalPublishingStrategy PublishingStrategy = "Internal"

)

also include validations.

data/data/ovirt/variables-ovirt.tf

data/data/ovirt/masters/main.tf

abhinavdahiya · 2020-05-01T19:57:52Z

data/data/ovirt/template/main.tf

+  os {
+    type = "rhcos_x64"
+  }


686b239#r418711291

pkg/types/ovirt/validation/machinepool.go

pkg/types/ovirt/validation/machinepool_test.go

abhinavdahiya · 2020-05-13T05:27:21Z

pkg/types/ovirt/validation/machinepool_test.go

+		{
+			name: "invalid instance type id",
+			pool: &ovirt.MachinePool{
+				InstanceTypeID: "aaaaa-123"},


Suggested change

InstanceTypeID: "aaaaa-123"},

InstanceTypeID: "aaaaa-123",

},

pkg/types/ovirt/validation/machinepool_test.go

abhinavdahiya · 2020-05-13T05:33:47Z

pkg/asset/installconfig/ovirt/validaton.go

+		_, err := con.SystemService().
+			InstanceTypesService().InstanceTypeService(machinePool.InstanceTypeID).Get().Send()


I'm not big fan of mixed indentation here, one multi-line the rest is one-line. single line is better even for this imo.

for me shorter lines (< 120) are preferred for the price of a bit of weirdness of go multi-line. Unfortunately ovirt sdk is more verbose than I'd wanted it to be (a lot of client code with builder probably)

For me shorter lines (< 120) are preferred for the price of a bit of weirdness of go multi-line. Unfortunately ovirt sdk is quite verbose (a lot of client code are the same probably...)

If that's a code-base guideline that's fine by me. I don't want to repeat this over again.

abhinavdahiya · 2020-05-13T05:39:41Z

pkg/types/ovirt/validation/machinepool.go

+			allErrs = append(allErrs, field.Invalid(fldPath.Child("instanceTypeID"), p.InstanceTypeID, "mixing instanceTypeID and CPU is not supported"))
+		}
+		if p.MemoryMB > 0 {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("instanceTypeID"), p.InstanceTypeID, "mixing instanceTypeID and Memory  is not supported"))


Suggested change

allErrs = append(allErrs, field.Invalid(fldPath.Child("instanceTypeID"), p.InstanceTypeID, "mixing instanceTypeID and Memory is not supported"))

allErrs = append(allErrs, field.Invalid(fldPath.Child("instanceTypeID"), p.InstanceTypeID, "mixing instanceTypeID and Memory is not supported"))

abhinavdahiya · 2020-05-13T05:45:39Z

Please update the godoc comments for the new fields in the machinepool as these are very sparse and incomplete feeling. With the addition of openshift-install explain installconfig #3515 the godoc comments will directly relate to better UX for users.

Make sure include the generated code using https://github.com/openshift/installer/blob/master/docs/dev/explain.md#generating-the-documentation

also, there is missing documentation in docs/ovirt/customization.md that documents the fields and also provides some example install-config.yaml 's for various configurations for users. see https://github.com/openshift/installer/blob/master/docs/user/aws/customization.md for AWS as example.

abhinavdahiya · 2020-05-13T15:06:53Z

/approve

openshift-ci-robot · 2020-05-13T15:07:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

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~~ [abhinavdahiya]

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

Those items are now part of MachinePool - instance type - cpu (cores, sockets) - memory - os disk size - vm type With those we have controll on the way create worker/nodes, and we have proper defaults set in the machine pool definitions, one for the pkg/asset/machines/ master.go and worker.go. Default master pool: cpus: 4 mem: 16 GiB os disk: 120 GB VM type: high_performance Default worker pool: cpus: 4 mem: 16 GiB os disk: 120 GB VM type: server A compute pool snippet from the install-config.yaml may look like that: ```yaml compute: - architecture: amd64 hyperthreading: Enabled name: worker platform: ovirt: cpu: cores: 8 sockets: 1 instanceTypeID: memoryMB: 65536 osDisk: sizeGB: 120 vmType: high_performance replicas: 3 ``` Terraform now uses those values, and all the defaults in tf files are now deleted in favour of the machine-config ones. To support `osDisk` size which is differnt than the template master/main.tf has this block: block_device { interface = "virtio_scsi" size = var.ovirt_master_os_disk_size_gb } If needed TF will extend the disk size of the VM. Cluster-api-provider-ovirt also extends the disk of a VM if the definition in the machine spec is differnt than the template. Signed-off-by: Roy Golan <rgolan@redhat.com>

Gal-Zaidman · 2020-05-13T15:51:45Z

/test e2e-ovirt

openshift-ci-robot · 2020-05-13T17:06:04Z

@rgolangh: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-ovirt	`5d0b06a`	link	`/test e2e-ovirt`
ci/prow/e2e-aws-scaleup-rhel7	`5d0b06a`	link	`/test e2e-aws-scaleup-rhel7`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

patrickdillon · 2020-05-14T01:13:25Z

/lgtm

openshift-ci-robot · 2020-05-14T01:18:31Z

@rgolangh: All pull requests linked via external trackers have merged: openshift/installer#3399. Bugzilla bug 1820300 has been moved to the MODIFIED state.

In response to this:

Bug 1820300: Extend oVirt's MachinePool

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.

rgolangh · 2020-05-14T13:52:23Z

/cherry-pick release-4.4

openshift-cherrypick-robot · 2020-05-14T13:52:41Z

@rgolangh: #3399 failed to apply on top of branch "release-4.4":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
A	go.mod
A	go.sum
A	vendor/modules.txt
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): vendor/modules.txt deleted in HEAD and modified in ovirt: bump terraform and cluster-api providers. Version ovirt: bump terraform and cluster-api providers of vendor/modules.txt left in tree.
CONFLICT (modify/delete): go.sum deleted in HEAD and modified in ovirt: bump terraform and cluster-api providers. Version ovirt: bump terraform and cluster-api providers of go.sum left in tree.
CONFLICT (modify/delete): go.mod deleted in HEAD and modified in ovirt: bump terraform and cluster-api providers. Version ovirt: bump terraform and cluster-api providers of go.mod left in tree.
Patch failed at 0001 ovirt: bump terraform and cluster-api providers

In response to this:

/cherry-pick release-4.4

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.

The fix in openshift/installer#3399 adds the machine pool definition into install-config. This updated is needed from now on to make the CI use it. Signed-off-by: Roy Golan <rgolan@redhat.com>

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Apr 3, 2020

openshift-ci-robot requested review from jhixson74 and mtnbikenc April 3, 2020 00:39

rgolangh changed the title ~~Bug 1813741: Extend oVirt's MachinePool~~ Bug 1820300: Extend oVirt's MachinePool Apr 3, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 3, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Apr 3, 2020

rgolangh force-pushed the bz1813741 branch from 884d002 to 9a20535 Compare April 3, 2020 07:38

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

rgolangh force-pushed the bz1813741 branch from 9a20535 to c4b7fbb Compare May 1, 2020 15:31

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

rgolangh force-pushed the bz1813741 branch from c4b7fbb to 686b239 Compare May 1, 2020 16:48

patrickdillon reviewed May 1, 2020

View reviewed changes

patrickdillon mentioned this pull request May 1, 2020

Bug 1816714: ovirt - add vnic profile id to platform #3406

Merged

abhinavdahiya reviewed May 1, 2020

View reviewed changes

data/data/ovirt/variables-ovirt.tf Show resolved Hide resolved

abhinavdahiya reviewed May 1, 2020

View reviewed changes

data/data/ovirt/masters/main.tf Show resolved Hide resolved

abhinavdahiya reviewed May 1, 2020

View reviewed changes

data/data/ovirt/template/main.tf Outdated

Comment on lines 54 to 56

os {

type = "rhcos_x64"

}

Copy link

Contributor

abhinavdahiya May 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

686b239#r418711291

abhinavdahiya reviewed May 13, 2020

View reviewed changes

pkg/types/ovirt/validation/machinepool.go Show resolved Hide resolved

abhinavdahiya reviewed May 13, 2020

View reviewed changes

pkg/types/ovirt/validation/machinepool_test.go Outdated Show resolved Hide resolved