remove kubevirt related kwok manifests from make cluster-up

alaypatel07 commented 2 months ago

What this PR does / why we need it:

kwok needs kubevirt CRDs to be installed before it can handle KubeVirt related stages. When running make cluster-up kubevirt is still not installed. This is causing bugs in running performance tests. KubeVirt related stages will be created in KubeVirt e2e tests before starting the tests as part of the setup.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR. Approvers are expected to review this list.

[ ] Design: A design document was considered and is present (link) or not required
[ ] PR: The PR description is expressive enough and will help future contributors
[ ] Code: Write code that humans can understand and Keep it simple
[ ] Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
[ ] Upgrade: Impact of this change on upgrade flows was considered and addressed if required
[ ] Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
[ ] Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
[ ] Community: Announcement to kubevirt-dev was considered

Release note:

None

alaypatel07 commented 2 months ago

cc @brianmcarey @xpivarc The setup will be done as part of https://github.com/kubevirt/kubevirt/pull/12117

brianmcarey commented 2 months ago

/cc

brianmcarey commented 2 months ago

@alaypatel07 what would be the issue with restarting the kwok controller after kubevirt is installed?

alaypatel07 commented 2 months ago

@brianmcarey restarting the kwok controller pod seems like a hack.

Since the e2e test already carries kwok objects like fake nodes https://github.com/kubevirt/kubevirt/pull/12117/files#diff-b1f53ed317ff089a7374611210b0b12f1e5cf165a8cb1748a4cedcd665141805R120, IMO it only makes sense to set up kubevirt related kwok manifests in the e2e tests.

This also allows for future e2e test possibilities like creating a stage (example of stage https://github.com/kubevirt/kubevirtci/blob/a087e7e153c809f9c1087cb8d8378b2bc1c6489c/cluster-provision/k8s/1.28/manifests/kwok/kubevirt/stage/vmi-ready.yaml) for VMIs to be stuck in scheduling state to check pressure on scheduler

xpivarc commented 2 months ago

Would it make sense to deploy only the stages as part of e2e?

alaypatel07 commented 2 months ago

Would it make sense to deploy only the stages as part of e2e?

@xpivarc The only other thing being deployed apart from stages is the rbac for kubevirt CRDs. The CRDs are not even registered before rbac is created here since kubevirt is not installed. So I am not sure if it makes sense to just have stages be delayed.

https://github.com/kubevirt/kubevirtci/blob/a087e7e153c809f9c1087cb8d8378b2bc1c6489c/cluster-provision/k8s/1.28/manifests/kwok/kubevirt/rbac/role.yaml

xpivarc commented 1 month ago

@alaypatel07 Ok, let's make it work in kubevirt/kubevirt. Please drop the local changes here and we can merge

alaypatel07 commented 1 month ago

@xpivarc done, PTAL

@Sreeja1725 can you please create the RBAC and kwok stages deleted from here in the e2e test setup https://github.com/kubevirt/kubevirt/pull/12117?

kubevirt-bot commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianmcarey

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](https://github.com/kubevirt/kubevirtci/blob/main/OWNERS)~~ [brianmcarey] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

kubevirt-commenter-bot commented 1 month ago

/retest-required This bot automatically retries required jobs that failed/flaked on approved PRs. Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot commented 1 month ago

@alaypatel07: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
check-up-kind-1.27-vgpu	4fe276db95a67f18e09a0dacdb6d2ecfef105920	link	false	`/test check-up-kind-1.27-vgpu`

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).

alaypatel07 commented 1 month ago

@brianmcarey can you confirm if the kubevirtci image from these changes were pushed in kubevirt repo? I couldnt find any recent bot commits.

cc @Sreeja1725

brianmcarey commented 1 month ago

@brianmcarey can you confirm if the kubevirtci image from these changes were pushed in kubevirt repo? I couldnt find any recent bot commits.

cc @Sreeja1725

It should be included when this bump (https://github.com/kubevirt/kubevirt/pull/12488) merges

kubevirt / kubevirtci

remove kubevirt related kwok manifests from make cluster-up #1225