Skip to content

Add kubevirt platform #260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Nov 6, 2020
Merged

Add kubevirt platform #260

openshift-merge-robot merged 1 commit into from
Nov 6, 2020

Conversation

@bardielle
Copy link
Contributor

@bardielle bardielle commented Oct 12, 2020 •
edited
Loading

Our main goal is creating a tenant cluster on top of existing OpenShift/Kubernetes cluster by creating virtualMachines (using kubevirt) for every node in the tenant cluster (masters and also workers nodes)
You can find more information here: openshift/enhancements#417

@openshift-ci-robot 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 Oct 12, 2020
@gregsheremeta
Copy link
Contributor

gregsheremeta commented Oct 12, 2020

Hi, is there a Jira for this work? I would like to better understand the goal. I'm surprised to see kubevirt treated as a cloud platform -- I thought it was more of an addon to OCP.

@bardielle
Copy link
Contributor Author

bardielle commented Oct 13, 2020

@gregsheremeta The epic is https://issues.redhat.com/browse/OCPCNV-1
The main goal is creating a nested openshift cluster (called tenant cluster) over an openshift cluster (called infra cluster) using kubevirt

@bardielle bardielle force-pushed the add_kubevirt_platform branch from f95d4a6 to 662fdba Compare October 13, 2020 13:09
ravidbro
ravidbro reviewed Oct 13, 2020
View reviewed changes
Copy link

@ravidbro ravidbro left a comment

Choose a reason for hiding this comment

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

In general, I don't understand why do we need so much of code duplication between the actuators when the difference is only how to create the data for the secret when needed but all the flow is the same.
Of course, not for this PR

@bardielle bardielle force-pushed the add_kubevirt_platform branch from 662fdba to b10eba2 Compare October 13, 2020 16:17
joelddiaz
joelddiaz reviewed Oct 13, 2020
View reviewed changes
@bardielle bardielle force-pushed the add_kubevirt_platform branch from b10eba2 to 2f0b549 Compare October 14, 2020 12:18
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2020
@bardielle bardielle force-pushed the add_kubevirt_platform branch 2 times, most recently from 645975e to e06155d Compare October 14, 2020 13:09
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2020
@bardielle bardielle force-pushed the add_kubevirt_platform branch from e06155d to 94f3d43 Compare October 14, 2020 13:54
@bardielle
Copy link
Contributor Author

bardielle commented Oct 14, 2020

/test e2e-aws

@bardielle bardielle force-pushed the add_kubevirt_platform branch 2 times, most recently from 91a9ed6 to 67a2a20 Compare October 14, 2020 15:00
@bardielle
Copy link
Contributor Author

bardielle commented Oct 14, 2020

/test verify

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 14, 2020

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

Test name Commit Details Rerun command
ci/prow/e2e-aws 2f0b549 link /test e2e-aws
ci/prow/e2e-upgrade 2f0b549 link /test e2e-upgrade
ci/prow/verify 67a2a20 link /test verify

Full PR test history. Your PR dashboard.

Details

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.

@bardielle bardielle force-pushed the add_kubevirt_platform branch from 67a2a20 to 9410bbe Compare October 15, 2020 09:23
@bardielle
Copy link
Contributor Author

bardielle commented Oct 15, 2020

/test verify

@bardielle bardielle force-pushed the add_kubevirt_platform branch from 9410bbe to 0695dfb Compare October 15, 2020 10:15
@bardielle bardielle changed the title WIP Add kubevirt platform Add kubevirt platform Oct 15, 2020
@openshift-ci-robot 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 Oct 15, 2020
@bardielle
Copy link
Contributor Author

bardielle commented Oct 15, 2020

/retest

1 similar comment
@bardielle
Copy link
Contributor Author

bardielle commented Oct 15, 2020

/retest

joelddiaz
joelddiaz reviewed Oct 15, 2020
View reviewed changes
Copy link
Contributor

@joelddiaz joelddiaz left a comment

Choose a reason for hiding this comment

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

👍

@joelddiaz
Copy link
Contributor

joelddiaz commented Oct 15, 2020

@dgoodwin you want to give this PR a look? I'm satisfied with how it is, but since it is adding a new platform type, this isn't the typical PR content.

dgoodwin
dgoodwin reviewed Oct 19, 2020
View reviewed changes
Copy link
Contributor

@dgoodwin dgoodwin left a comment

Choose a reason for hiding this comment

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

Just a couple extremely minor requests, this looks good to me. Very interesting enhancement.

pkg/kubevirt/actuator.go Outdated
"context"
"fmt"
"github.com/openshift/cloud-credential-operator/pkg/operator/constants"
actuatoriface "github.com/openshift/cloud-credential-operator/pkg/operator/credentialsrequest/actuator"
Copy link
Contributor

@dgoodwin dgoodwin Oct 19, 2020

Choose a reason for hiding this comment

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

Imports should be grouped a little better, looks like standard library, random things like logrus, k8s, and openshift.

pkg/kubevirt/actuator.go Outdated
return err
}
if existingSecret != nil {
logger.Debug("Deleting existing secret")
Copy link
Contributor

@dgoodwin dgoodwin Oct 19, 2020

Choose a reason for hiding this comment

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

logger.Info for this one please.

README.md Outdated
# Cloud Providers

Currently the operator supports AWS, GCP, Azure, VMWare, OpenStack and oVirt.
Currently the operator supports AWS, Azure, GCP, kubeVirt, OpenStack. oVirt and VMWare.
Copy link
Contributor

@dgoodwin dgoodwin Oct 19, 2020

Choose a reason for hiding this comment

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

The constant in the Infrastructure API looks like "KubeVirt" which would look a little more natural to me here, could we sync up on that casing across the docs/PR for consistency?

Copy link
Contributor Author

@bardielle bardielle Nov 5, 2020

Choose a reason for hiding this comment

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

I fixed all the cases in documentation

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 5, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bardielle, dgoodwin

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:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020
@nirarg nirarg mentioned this pull request Nov 5, 2020
Merged
@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

18 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 04c0721 into openshift:master Nov 6, 2020
abutcher added a commit to abutcher/cloud-credential-operator that referenced this pull request Nov 6, 2020
@abutcher
Revert "Merge pull request openshift#260 from bardielle/add_kubevirt_…
7c146a9 
…platform"

This reverts commit 04c0721, reversing
changes made to 46b4254.
@nirarg nirarg mentioned this pull request Nov 11, 2020
Merged
@chenyosef chenyosef mentioned this pull request Nov 18, 2020
Merged
@dmage dmage mentioned this pull request Jan 5, 2021
Closed
ming1013 pushed a commit to ming1013/cloud-credential-operator that referenced this pull request Dec 15, 2025
@openshift-merge-robot
Merge pull request openshift#260 from bardielle/add_kubevirt_platform
981eb9b 
Add kubevirt platform
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dgoodwin dgoodwin dgoodwin left review comments

@csrwng csrwng Awaiting requested review from csrwng

+2 more reviewers

@joelddiaz joelddiaz joelddiaz left review comments

@ravidbro ravidbro ravidbro left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@dgoodwin dgoodwin

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@bardielle @gregsheremeta @openshift-ci-robot @joelddiaz @dgoodwin @openshift-bot @ravidbro @openshift-merge-robot