Bug 1816714: ovirt - add vnic profile id to platform by rgolangh · Pull Request #3406 · openshift/installer

rgolangh · 2020-04-03T11:16:56Z

The vnic profile that all the VM shares can now be configurable through
the platform.
The wizard will also ask to the user the choose one, in case there are multiple profiles per network.

platform:
  ovirt:
    vnicProfileID: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx

rgolangh · 2020-04-03T13:39:27Z

@patrickdillon @wking PTAL

openshift-ci-robot · 2020-04-03T14:41:05Z

@rgolangh: This pull request references Bugzilla bug 1816714, 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)

Details

In response to this:

Bug 1816714: ovirt - add vnic profile id to platform

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-03T14:42:13Z

@rgolangh: This pull request references Bugzilla bug 1816714, 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)

Details

In response to this:

Bug 1816714: ovirt - add vnic profile id to platform

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.

Jmainguy · 2020-04-03T18:30:42Z

code looks solid to my noob eyeballs, nice work

sdodson · 2020-04-07T00:55:42Z

/cc @Gal-Zaidman @sandrobonazzola
/approve

sdodson · 2020-04-07T00:56:06Z

/retest

abonillabeeche · 2020-04-08T03:31:47Z

Is this fix already in a nightly build?

sdodson · 2020-04-08T11:43:08Z

Is this fix already in a nightly build?

no, it wouldn't be in a build if it hasn't merged

sferich888 · 2020-04-14T15:01:32Z

/cherry-pick release-4.4

Cherrypicking this for https://bugzilla.redhat.com/show_bug.cgi?id=1820575

openshift-cherrypick-robot · 2020-04-14T15:01:32Z

@sferich888: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

Details

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.

sferich888 · 2020-04-14T17:42:31Z

/retest

patrickdillon · 2020-04-14T19:41:47Z

/assign

sdodson · 2020-04-14T19:42:14Z

@Gal-Zaidman @sandrobonazzola can you please review the ovirt specific bits here while installer team takes on review of the generic components

patrickdillon · 2020-04-14T20:10:34Z

The CI failure indicates a problem when ovirt_vnic_profile_id is not set in the install-config: https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_installer/3406/pull-ci-openshift-installer-master-e2e-ovirt/1470

I don't think ovirt_vnic_profile_id can be set to required in order to maintain backward compatibility. Previously a default value was being set here: https://github.com/openshift/installer/pull/3406/files#diff-4f2a1c1c86a37e0831ca92d07d3f6949L25

Can we assume that default if the variable is not set in the install config? Ideally that would be done outside of terraform and passed in tfvars. If you need to do it in terraform, you will need to set a default value for the variable--but (again) doing this in go and not terraform is easier/cleaner.

rgolangh · 2020-04-22T11:05:21Z

The CI failure indicates a problem when ovirt_vnic_profile_id is not set in the install-config: https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_installer/3406/pull-ci-openshift-installer-master-e2e-ovirt/1470

I don't think ovirt_vnic_profile_id can be set to required in order to maintain backward compatibility. Previously a default value was being set here: https://github.com/openshift/installer/pull/3406/files#diff-4f2a1c1c86a37e0831ca92d07d3f6949L25

Can we assume that default if the variable is not set in the install config? Ideally that would be done outside of terraform and passed in tfvars. If you need to do it in terraform, you will need to set a default value for the variable--but (again) doing this in go and not terraform is easier/cleaner.

The assumption is what cause the bug - there is no way to 'guess' the vnic profile name and choosing the first one in the list is a poor choice. That heuristic is my terrible mistake.

Like you mentioned, getting the defaults from go is much more desirable and I'll look for a clean way to do it.

rgolangh · 2020-04-25T08:22:05Z

/test e2e-ovirt

rgolangh · 2020-04-28T18:13:24Z

/retest

patrickdillon · 2020-05-01T13:29:45Z

Shouldn't this logic only be run if installConfig.Config.Platform.Ovirt.VNICProfileID is not set?

right, I missed it during the refactoring. adding back

patrickdillon · 2020-05-01T13:33:46Z

If i understand correctly this is different default behavior than what you're replacing, but that behavior would be considered a bug so this is ok, in my opinion.

patrickdillon · 2020-05-01T19:02:52Z

I will try to summarize Slack conversation:

It doesn't look like this will set VNIC profile ID for compute nodes.

ovirt MachinePools are being added in #3399
the VNIC profile ID will also need to be added to the machine provider

Is there a benefit from merging this before these changes? What is the effect when the changes happen on control plane but not compute?

openshift-ci-robot · 2020-05-02T07:46:50Z

@rgolangh: This pull request references Bugzilla bug 1816714, 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)

Details

In response to this:

Bug 1816714: ovirt - add vnic profile id to platform

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-03T16:52:11Z

I will try to summarize Slack conversation:

It doesn't look like this will set VNIC profile ID for compute nodes.

Vnic and network name are currently generic definition that goes into the template creation. Both masters and workers gets them because they use the same template.
As far as what this bug is about, this solves them urgent problem.

ovirt MachinePools are being added in #3399
the VNIC profile ID will also need to be added to the machine provider

The cluster api provider still doesn't support customizing the nics of a machine.
I need to know now if it is acceptable to have a vnic id in the platform level and then override them in the machine config. If not then I need to add vnic into the machine pool ASAP

Is there a benefit from merging this before these changes? What is the effect when the changes happen on control plane but not compute?

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

This validation is providing a default value, but I think you are already doing that elsewhere. Validation should be checking that if a user provides a value then it is valid.

this is replying nil if valid (line 63) or error in some cases. Maybe your are talking about the 2nd use of it.?

The way I am reading this code:
if user provided a vnic profile ID: do nothing (it is valid)
if user did not provide a vnic profile ID: see if you can determine a default or else return an error

What I was expecting:
if user provided a vnic profile ID: validate (use API to determine if user provided value is correct)
if user did not provide: do nothing (default will be assigned later)

Does that make sense? I could definitely have mistaken assumptions here.

rgolangh · 2020-05-04T06:32:33Z

@patrickdillon it really seems that adding this to machine spec makes more sense. So I added openshift/cluster-api-provider-ovirt#45 and this will find its way into #3399
After that I'll move vnic_profile_id from platform and into machine pool

Signed-off-by: Roy Golan <rgolan@redhat.com>

The vnic profile that all the VM shares can now be configurable through the platform and is part of the wizard, in case there are multiple for the selected network. ```yaml platform: ovirt: vnicProfileID: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx ``` Signed-off-by: Roy Golan <rgolan@redhat.com>

rgolangh · 2020-05-05T21:28:28Z

/test e2e-ovirt

openshift-ci-robot · 2020-05-05T21:29:38Z

@rgolangh: This pull request references Bugzilla bug 1816714, 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)

Details

In response to this:

Bug 1816714: ovirt - add vnic profile id to platform

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-06T14:46:13Z

/test e2e-ovirt

rgolangh · 2020-05-06T14:46:32Z

/test e2e-openstack

patrickdillon · 2020-05-06T15:08:12Z

/lgtm

abhinavdahiya · 2020-05-06T15:18:02Z

/approve

openshift-ci-robot · 2020-05-06T15:18:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, sdodson

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya,sdodson]

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

openshift-ci-robot · 2020-05-06T16:53:49Z

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

Details

In response to this:

Bug 1816714: ovirt - add vnic profile id to platform

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-cherrypick-robot · 2020-05-06T16:54:03Z

@sferich888: #3406 failed to apply on top of branch "release-4.4":

Applying: ovirt: Add platform validation
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/asset/installconfig/installconfig.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/asset/installconfig/installconfig.go
CONFLICT (content): Merge conflict in pkg/asset/installconfig/installconfig.go
Patch failed at 0002 ovirt: Add platform validation

Details

In response to this:

/cherry-pick release-4.4

Cherrypicking this for https://bugzilla.redhat.com/show_bug.cgi?id=1820575

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 force-pushed the bz1816714 branch 2 times, most recently from 5f9076b to eaced32 Compare April 3, 2020 13:13

rgolangh changed the title ~~ovirt: add vnic profile~~ Bug 1816714: ovirt - add vnic profile id to platform 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

Jmainguy mentioned this pull request Apr 3, 2020

oVirt/RHV - openshift-install throws terraform error. #3266

Closed

openshift-ci-robot requested review from Gal-Zaidman and sandrobonazzola April 7, 2020 00:55

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2020

openshift-ci-robot assigned patrickdillon Apr 14, 2020

rgolangh force-pushed the bz1816714 branch 2 times, most recently from fa64f87 to d12512d Compare April 24, 2020 23:38

rgolangh force-pushed the bz1816714 branch 2 times, most recently from 516b2b4 to 0414c6d Compare April 28, 2020 14:31

rgolangh force-pushed the bz1816714 branch from 0173c57 to 4fe2f6e Compare May 1, 2020 11:38

patrickdillon reviewed May 1, 2020

View reviewed changes

rgolangh force-pushed the bz1816714 branch 2 times, most recently from 9339ec8 to 7f10dfd Compare May 1, 2020 15:20

rgolangh closed this May 2, 2020

rgolangh reopened this May 2, 2020

openshift-ci-robot added the bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. label May 2, 2020

patrickdillon reviewed May 3, 2020

View reviewed changes

rgolangh added 2 commits May 5, 2020 23:19

ovirt: a simpler function to get a connection to oVirt API

4819be6

Signed-off-by: Roy Golan <rgolan@redhat.com>

ovirt: Add platform validation

44e4d56

Signed-off-by: Roy Golan <rgolan@redhat.com>

rgolangh force-pushed the bz1816714 branch from 7f10dfd to 0426979 Compare May 5, 2020 20:45

rgolangh force-pushed the bz1816714 branch from 0426979 to 2d2fe0b Compare May 5, 2020 21:01

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

openshift-merge-robot merged commit 7737b2a into openshift:master May 6, 2020

patrickdillon mentioned this pull request May 7, 2020

Bug 1820575: Cherry-pick oVirt PR 3406 vNIC profiles #3569

Merged

Conversation

rgolangh commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgolangh commented Apr 3, 2020

Uh oh!

openshift-ci-robot commented Apr 3, 2020

Uh oh!

openshift-ci-robot commented Apr 3, 2020

Uh oh!

Jmainguy commented Apr 3, 2020

Uh oh!

sdodson commented Apr 7, 2020

Uh oh!

sdodson commented Apr 7, 2020

Uh oh!

abonillabeeche commented Apr 8, 2020

Uh oh!

sdodson commented Apr 8, 2020

Uh oh!

sferich888 commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-cherrypick-robot commented Apr 14, 2020

Uh oh!

sferich888 commented Apr 14, 2020

Uh oh!

patrickdillon commented Apr 14, 2020

Uh oh!

sdodson commented Apr 14, 2020

Uh oh!

patrickdillon commented Apr 14, 2020

Uh oh!

rgolangh commented Apr 22, 2020

Uh oh!

rgolangh commented Apr 25, 2020

Uh oh!

rgolangh commented Apr 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented May 1, 2020

Uh oh!

openshift-ci-robot commented May 2, 2020

Uh oh!

rgolangh commented May 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgolangh commented May 4, 2020

Uh oh!

rgolangh commented May 5, 2020

Uh oh!

openshift-ci-robot commented May 5, 2020

Uh oh!

rgolangh commented May 6, 2020

Uh oh!

rgolangh commented May 6, 2020

Uh oh!

patrickdillon commented May 6, 2020

Uh oh!

abhinavdahiya commented May 6, 2020

Uh oh!

openshift-ci-robot commented May 6, 2020

Uh oh!

openshift-ci-robot commented May 6, 2020

Uh oh!

openshift-cherrypick-robot commented May 6, 2020

rgolangh commented Apr 3, 2020 •

edited

Loading

sferich888 commented Apr 14, 2020 •

edited

Loading