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 #2098

Merged

Conversation

ravidbro
Copy link

@ravidbro ravidbro commented Sep 17, 2020

Add KubeVirt as a platform for running OpenShift nodes as VMs.

More details at 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 Sep 17, 2020
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2020
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I love the idea. So meta!

But I really strongly think before we try to land this in 4.7 we need to invest a lot in de-duping and rationalizing the platform-specific code in the MCO. We simply cannot carry another copy of the kubelet configs for example.

@cgwalters
Copy link
Member

Some of that is in #2079

@ravidbro
Copy link
Author

Some of that is in #2079

I agree.
When I created the PR I saw that #2079 wasn't approved yet and also that it doesn't contain all the files (e.g. kubelet, keepalived).
I think I will wait for the templates de-dup PR and then I will rebase on it.

@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 29, 2020
@ravidbro ravidbro changed the title WIP: Add kubevirt platform Add kubevirt platform Nov 1, 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 1, 2020
@ravidbro
Copy link
Author

ravidbro commented Nov 1, 2020

/retest

@ravidbro
Copy link
Author

ravidbro commented Nov 1, 2020

@cgwalters I changed the PR to have only the bump of openshift/api library and the manifests of the bootstrap based on #2071 .
I will add the required files for the template after #2079 will be merged.

@ravidbro
Copy link
Author

ravidbro commented Nov 1, 2020

/retest

1 similar comment
@ravidbro
Copy link
Author

ravidbro commented Nov 1, 2020

/retest

@chenyosef
Copy link

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2020
--volume /usr/share/zoneinfo:/usr/share/zoneinfo \
--net=host \
--pull missing \
docker://registry.svc.ci.openshift.org/ovirt/qemu-guest-agent:4.2
Copy link
Member

Choose a reason for hiding this comment

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

This must be fixed before this can merge.

Copy link
Author

Choose a reason for hiding this comment

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

For now, I removed the guest agent, we can work around without that

@cgwalters
Copy link
Member

/hold
For the above at the minimum.
Can you please add a link in the PR and commit message to openshift/enhancements#417 at least?

@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 Nov 2, 2020
@ravidbro ravidbro force-pushed the add_kubevirt_platform branch 2 times, most recently from b16b74e to 8858fc4 Compare November 3, 2020 10:17
@ravidbro
Copy link
Author

ravidbro commented Nov 3, 2020

/hold
For the above at the minimum.
Can you please add a link in the PR and commit message to openshift/enhancements#417 at least?

I added a reference to the enhancement both in the PR and in the commit message.

@ravidbro
Copy link
Author

ravidbro commented Nov 3, 2020

/retest

@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 Nov 17, 2020
@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Nov 17, 2020

Q: is installer intending to merge this PR for 4.7? openshift/installer#4350

/override ci/prow/e2e-gcp-op

Please note this job is required for this PR to merge..

@ravidbro
Copy link
Author

yes, the installer PR is intended to merge for 4.7.
We need also this (MCO) PR to go in since we are committed for a customer to have dev-preview of this feature in 4.7.
The enhancement is not merged but there is already an agreement on that, you can see @crawford comment

@chenyosef
Copy link

chenyosef commented Nov 18, 2020

It feels premature to merge this seeing that the enhancement is not approved - missing both approved & lgtm) as well as a test plan & dev preview criteria: openshift/enhancements#417

@kikisdeliveryservice Thanks for your review. As for the test plan - our plan is to use the same testing framework as other installer platforms, i.e. using OpenShift CI (you can see WIP in openshift/release#13547, openshift/release#12765, openshift/release#13165), including the conformance testing of the tenant cluster). We will update the enhancement to reflect that (like @ravidbro mentioned, the enhancement itself was already discussed and approved by both PMs and @crawford, even though wasn't merged yet).
More over, as for the timeline - yes, we planned to integrate this to 4.7, and already did so in several other components (openshift/cloud-credential-operator#260, https://github.com/openshift/cluster-api-provider-kubevirt, https://github.com/openshift/release/blob/0ac2cccd3e33a748523cb484a20ea27307a3f8b6/core-services/image-mirroring/openshift/mapping_origin_4_7#L140)

Given the above I don't feel like this should be merged until the above is satisfied.

/hold

Given the above can you please consider removing the hold ?

Thanks,
Chen

@kikisdeliveryservice
Copy link
Contributor

hi @chenyosef & @ravidbro !

Thanks for the info.

2 Qs:

  1. what is being delivered for dev preview/alpha? (the enhancement doesn't say)
  2. when do you intend on having any basic e2e on this? I'd assume that once all of this merges, we'd be able to reopen WIP Adding e2e kubevirt release#13165 and get that working soon after (so we wouldnt be waiting multiple releases right)?

@ravidbro
Copy link
Author

  1. The dev preview will have fully functional environment installed as IPI (except of automating csi driver install)
    The reason for the dev preview is that for this version it is tested only by us and the CI and not by QE.
  2. We expect the CI to land in the next few weeks

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Nov 19, 2020

1. The dev preview will have fully functional environment installed as IPI (except of automating csi driver install)
   The reason for the dev preview is that for this version it is tested only by us and the CI and not by QE.

2. We expect the CI to land in the next few weeks

Great! Thanks for the info! Our gcp-op is required to run and pass before merging this PR but we are having issues with that test for the last week that will be solved once #2229 merges. To avoid unnecessary retests (it will def fail rn), I'll remove the hold tomorrow.

Add: In the meantime it looks like this PR needs a rebase anyway.

Signed-off-by: Ravid Brown <ravid@redhat.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2020
More details at openshift/enhancements#417

Signed-off-by: Ravid Brown <ravid@redhat.com>
@ravidbro
Copy link
Author

1. The dev preview will have fully functional environment installed as IPI (except of automating csi driver install)
   The reason for the dev preview is that for this version it is tested only by us and the CI and not by QE.

2. We expect the CI to land in the next few weeks

Great! Thanks for the info! Our gcp-op is required to run and pass before merging this PR but we are having issues with that test for the last week that will be solved once #2229 merges. To avoid unnecessary retests (it will def fail rn), I'll remove the hold tomorrow.

Add: In the meantime it looks like this PR needs a rebase anyway.

Thank you for the review.
I see that the fix was merged and I already rebased my branch to include all the fixes from master.
I will highly appreciate it if you can cancel the hold at this point.

@chenyosef
Copy link

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bcrochet, chenyosef, ravidbro, sinnykumari

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

@ravidbro
Copy link
Author

/retest

3 similar comments
@chenyosef
Copy link

/retest

@ravidbro
Copy link
Author

/retest

@ravidbro
Copy link
Author

/retest

@openshift-merge-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/okd-e2e-aws 6d005c4 link /test okd-e2e-aws

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.

@ravidbro
Copy link
Author

I see that all tests passed but the 'okd' one which fails constantly on "toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit" because docker started with their rate-limiting.

@kikisdeliveryservice can you cancel the hold?
highly appreciated if you can help us to meet our goal.

@kikisdeliveryservice
Copy link
Contributor

I'm in PST so I just am going thru things.

/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 Nov 19, 2020
@ravidbro
Copy link
Author

/skip

@ravidbro
Copy link
Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 3add4b9 into openshift:master Nov 20, 2020
@ravidbro ravidbro deleted the add_kubevirt_platform branch November 20, 2020 09:02
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