Skip to content
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.

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

Add KubeVirt platform as infrastructure for Openshift installation #4350

Merged
merged 21 commits into from Dec 4, 2020

Conversation

nirarg
Copy link
Contributor

@nirarg nirarg commented Nov 5, 2020

This PR is part of task done to add KubeVirt platform as infra provider for Openshift.

The change introduced in this PR purpose is to provide a way to install Openshift on KubeVirt infrastructure

For more information:

Related PRs/changes done as part of this task:

  1. Created openshift/cluster-api-provider-kubevirt repository, hosts an implementation of a provider for KubeVirt for the OpenShift machine-api: https://github.com/openshift/cluster-api-provider-kubevirt
  2. openshift/machine-api-operator: Add Kubevirt provider machine-api-operator#716 (comment)
  3. openshift/cloud-credential-operator: Add kubevirt platform cloud-credential-operator#260
  4. openshift/machine-config-operator: Add kubevirt platform machine-config-operator#2098
  5. openshift/api: Add KubeVirt platform type api#734

Additional job is done to have tests and CI coverage (work in progress in github.com/openshift/release repository)

@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 Nov 5, 2020
@nirarg nirarg force-pushed the master branch 2 times, most recently from f6fcc06 to 69b999d Compare November 10, 2020 09:02
@nirarg nirarg force-pushed the master branch 2 times, most recently from 1168e07 to 6b0ed2d Compare November 12, 2020 19:32
@nirarg
Copy link
Contributor Author

nirarg commented Nov 15, 2020

/retest

@nirarg nirarg changed the title [WIP] Add KubeVirt platform as infrastructure for Openshift installation Add KubeVirt platform as infrastructure for Openshift installation Nov 16, 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 Nov 16, 2020
@nirarg
Copy link
Contributor Author

nirarg commented Nov 16, 2020

/retest

2 similar comments
@nirarg
Copy link
Contributor Author

nirarg commented Nov 17, 2020

/retest

@nirarg
Copy link
Contributor Author

nirarg commented Nov 17, 2020

/retest

go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
path = "/etc/hostname"

content {
content = <<EOF
Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't this be a one-liner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,64 @@
variable "cluster_id" {
description = "The ID of Openshift cluster"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: OpenShift

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


variable "pv_access_mode" {
type = string
description = "The access mode which all the persistant volumes should be created with [ReadWriteOnce,ReadOnlyMany,ReadWriteMany]"
Copy link
Contributor

Choose a reason for hiding this comment

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

ReadOnlyMany doesn't make sense here, I think its not worth mentioning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

"encoding/json"

v1 "github.com/openshift/cluster-api-provider-kubevirt/pkg/apis/kubevirtprovider/v1alpha1"
// "github.com/openshift/installer/pkg/rhcos"
Copy link
Contributor

Choose a reason for hiding this comment

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

remove unused imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

description: Kubevirt is the configuration used when installing on Kubevirt.
properties:
cpu:
description: CPU is the mount of cpus used
Copy link
Contributor

Choose a reason for hiding this comment

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

mount/amount - need to change in go code and regenerate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

description: DefaultMachinePlatform is the default configuration used when installing on Kubevirt for machine pools which do not define their own platform configuration.
properties:
cpu:
description: CPU is the mount of cpus used
Copy link
Contributor

Choose a reason for hiding this comment

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

mount/amount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

// MachinePool stores the configuration for a machine pool installed
// on kubevirt.
type MachinePool struct {
// CPU is the mount of cpus used
Copy link
Contributor

Choose a reason for hiding this comment

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

s/mount/amount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

// NewClient creates our client wrapper object for the actual kubeVirt and kubernetes clients we use.
func NewClient() (Client, error) {
loadingRules := clientcmd.NewDefaultClientConfigLoadingRules()
// if you want to change the loading rules (which files in which order), you can do so here
Copy link
Contributor

Choose a reason for hiding this comment

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

those comments are from the example, I'm not sure we need those here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed comments

return result, nil
}

// VirtualMachine
Copy link
Contributor

Choose a reason for hiding this comment

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

needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the comments

return nil, err
}
msg := fmt.Sprintf("Failed to get VirtualMachine, with error: %v", err)
log.Printf("[Error] %s", msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not log.Errorf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


// NetworkAttachmentDefinition

func (c *client) GetNetworkAttachmentDefinition(ctx context.Context, name string, namespace string) (*unstructured.Unstructured, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ctx unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

return c.kubernetesClient.CoreV1().Secrets(namespace).List(context.Background(), metav1.ListOptions{})
}

func (c *client) DeleteSecret(namespace string, name string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

add context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The context is created internally in the client using context.Background()

return c.kubernetesClient.CoreV1().Secrets(namespace).Get(context.Background(), name, metav1.GetOptions{})
}

func (c *client) ListSecret(namespace string) (*corev1.SecretList, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The context is created internally in the client using context.Background()

nirarg and others added 13 commits December 4, 2020 07:42
* Add specific parameters to request from user
* Add an option to request "Mobile Network CIDR" from user, enabled only in kubevirt platform
…rt provider

* Those configurations are used in the "manifests" stage
* Use providerSpec (declared in cluster_api_provider_kubevirt) inside the configurations
* Use provider Infra cluster kubeconfig to generate the following secrets:
  - cloud-creds-secret.yaml template
  - role-cloud-creds-secret-reader.yaml template
* Add kubevirt platform parameters to cloudproviderconfig, include: namespace
* Skip DNS config and its CRD generation for kubevirt platform
* Modify bootstrap bootkube.sh

data/data/bootstrap
…ions (multus network)

* Set APIVIP for ignition access
* Set mcscertkey for APIVIP
* Use the same image used by openstack and ovirt
* Update tfvars with cluster's values
* Update metadata object, Saved for destroy operation
This is done in order to allow the bootstrap to notify the cluster when it done
@nirarg
Copy link
Contributor Author

nirarg commented Dec 4, 2020

/test e2e-gcp

Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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:

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 Dec 4, 2020
@staebler
Copy link
Contributor

staebler commented Dec 4, 2020

Holding for gcp and azure tests, since they have some vendor changes.

/test e2e-azure
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2020
@nirarg
Copy link
Contributor Author

nirarg commented Dec 4, 2020

/retest

1 similar comment
@nirarg
Copy link
Contributor Author

nirarg commented Dec 4, 2020

/retest

@openshift-merge-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-ovirt 69245ca link /test e2e-ovirt
ci/prow/e2e-crc 69245ca link /test e2e-crc
ci/prow/e2e-azure 69245ca link /test e2e-azure

Full PR test history. Your PR dashboard.

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.

@staebler
Copy link
Contributor

staebler commented Dec 4, 2020

The Azure installation is succeeding but the e2e tests are failing.

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2020
@openshift-merge-robot openshift-merge-robot merged commit 9e380eb into openshift:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants