KV provider #1230

aerosouund · 2024-07-23T11:17:02Z

What this PR does / why we need it:

Create the KubevirtProvider type and make it the main entity for cluster actions

Part of: #1110

Special Notes
This branch is based off opts. PR: #1217
it introduces the same commits but after opts gets merged it will only introduce commits of the kubevirt provider

Checklist

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:

Create kubevirt provider, move run and provision logic to it

kubevirt-bot · 2024-07-23T11:17:11Z

Hi @aerosouund. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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-sigs/prow repository.

kubevirt-bot · 2024-07-23T11:17:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign awels for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

aerosouund · 2024-07-23T11:18:15Z

cc: @acardace @xpivarc @dhiller @brianmcarey

Two new opts that represent the two scripts used in the provision phase (provision linux and provision k8s). Using go embed to include any necessary config files then run the commands on a node using libssh Signed-off-by: aerosouund <aerosound161@gmail.com>

The KubevirtProvider is a struct representing an arbitrary Kubevirtci running cluster. It holds all config flags and options that are in the run and provision commands. A Kubevirt provider can be created in two ways, by creating a cluster using the Start method, or from an already running cluster. For this to be possible then json representation of the struct is persisted on the dnsmasq container and later read to parse the deployed settings Or through the normal constructor which uses the option pattern to avoid a bloated function signature The logic that was previously in run.go has been split to several methods to facilitate readability and testing (runNFSGanesha, runRegistry, prepareQemuCmd, prepareDeviceMappings) and dnsmasq creation logic got moved to its own method instead of existing in its own package Floating methods such as waitForVMToBeUp, nodeNameFromIndex, nodeContainer.. etc were grouped to be methods of the struct Signed-off-by: aerosouund <aerosound161@gmail.com>

dhiller · 2024-08-26T14:08:50Z

/test all

kubevirt-bot · 2024-08-26T15:01:49Z

@aerosouund: The following tests 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-provision-k8s-1.29	`28752d9`	link	true	`/test check-provision-k8s-1.29`
check-provision-k8s-1.30	`28752d9`	link	true	`/test check-provision-k8s-1.30`
check-up-kind-1.27-vgpu	`28752d9`	link	false	`/test check-up-kind-1.27-vgpu`
check-up-kind-1.30-vgpu	`28752d9`	link	false	`/test check-up-kind-1.30-vgpu`
check-provision-k8s-1.31	`28752d9`	link	true	`/test check-provision-k8s-1.31`
check-provision-k8s-1.28	`28752d9`	link	true	`/test check-provision-k8s-1.28`

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-sigs/prow repository. I understand the commands that are listed here.

To avoid having to read each flag and return an error if its unset leverage the FlagMap, a map of flag name to FlagConfig. a FlagConfig is the type of this flag (string, int, uint16, bool or array of string) and the option function that sets the value of this flag on the KubevirtProvider struct. During parsing of flags this map is being iterated on and each option gets appended to an array to later be used in the KubevirtProvider constructor. The run method's role is now to parse the flags and pass them to the provider and just call Start. All the floating methods in run.go are removed after being moved to the provider. Signed-off-by: aerosouund <aerosound161@gmail.com>

This functionality now exists in the KubevirtProvider type and doesn't need a package of its own Signed-off-by: aerosouund <aerosound161@gmail.com>

The KubevirtProvider type is what provides the methods that run a node or run the k8s options. Testing logic has been moved to a Base Provider Suite Signed-off-by: aerosouund <aerosound161@gmail.com>

Implement provisioning logic on the provider and leverage the linux and k8s provisioning instead of the scripts. Leverage the same pattern of creating an unpriviliged sshClient to add the ssh key to the root user then use a priviliged client. Include a map of version major, minor to a version with major, minor and patch to pass it to the provisionk8s opt. Remove unwanted floating methods such as _cmd, performPhase and implement copyDirectory as a method of the provider type. Change provision.go command to call the Provision method. Change type of port flags to uint16 to match the struct type. Change default value of random-ports to true. THIS COMMIT BREAKS THE KUBEVIRTCI RELEASE AND VERSIONING PATTERNS Signed-off-by: aerosouund <aerosound161@gmail.com>

Signed-off-by: aerosouund <aerosound161@gmail.com>

aerosouund · 2024-08-26T15:21:47Z

I think a reason for this flake, the one where the CI job keeps waiting on the node to stop is that the node shuts down before reporting the status back to the ssh agent which makes it not know whether this happened or no

this code previously looked like this

_cmd(cli, nodeContainer(prefix, nodeName), "ssh.sh sudo shutdown now -h", "shutting down the node")

and now it looks like this (this PR introduces this change)

    if err = sshClient.Command("sudo shutdown now -h"); err != nil {
        return err
    }

which results in a golang error, since previously the error wasn't being handled

failed to execute command: sudo shutdown now -h

and so what i suggest we do is that we don't handle the error, i don't know if we also need to force the code to continue executing after a certain duration but this may not be necessary. since previously when ssh.sh would fail the gocli wouldn't get this failure and the CI run hangs. but since we now ssh directly its guaranteed that if there's an error you will get it

I will push a commit with this suggestion

Signed-off-by: aerosouund <aerosound161@gmail.com>

kubevirt-bot · 2024-09-09T13:13:44Z

PR needs rebase.

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-sigs/prow repository.

brianmcarey · 2024-09-10T16:31:15Z

cluster-provision/gocli/opts/k8sprovision/conf/crio-yum.repo

@@ -0,0 +1,6 @@
+[isv_kubernetes_addons_cri-o_stable_v1.28]
+name=CRI-O v1.28 (Stable) (rpm)


I think we will need some version handling here - cri-o version should match the k8s version being deployed.

yeah agreed, after inspecting the existing state of the code. some providers share crio version but occasionally one provider might deviate. will account for this

xpivarc · 2024-09-11T12:40:40Z

I think a reason for this flake, the one where the CI job keeps waiting on the node to stop is that the node shuts down before reporting the status back to the ssh agent which makes it not know whether this happened or no

this code previously looked like this
_cmd(cli, nodeContainer(prefix, nodeName), "ssh.sh sudo shutdown now -h", "shutting down the node")
and now it looks like this (this PR introduces this change)
    if err = sshClient.Command("sudo shutdown now -h"); err != nil {
        return err
    }
which results in a golang error, since previously the error wasn't being handled

failed to execute command: sudo shutdown now -h

Is this from the _cmd?

and so what i suggest we do is that we don't handle the error, i don't know if we also need to force the code to continue executing after a certain duration but this may not be necessary. since previously when ssh.sh would fail the gocli wouldn't get this failure and the CI run hangs. but since we now ssh directly its guaranteed that if there's an error you will get it

I will push a commit with this suggestion

aerosouund · 2024-09-11T14:34:38Z

@xpivarc

Is this from the _cmd?

yes, i believe so. since when it was executed through ssh.sh it would timeout
and if executed and expected to handle the error it crashes

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jul 23, 2024

kubevirt-bot requested review from davidvossel and stu-gott July 23, 2024 11:17

kubevirt-bot added the size/XXL label Jul 23, 2024

aerosouund force-pushed the kv-provider branch from 5165a3a to c69b93f Compare July 23, 2024 15:20

aerosouund changed the title ~~Kv provider~~ KV provider Jul 28, 2024

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2024

aerosouund mentioned this pull request Aug 14, 2024

Bootc provisioner #1247

Open

8 tasks

aerosouund force-pushed the kv-provider branch from c69b93f to 1ff08d2 Compare August 18, 2024 10:46

kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 18, 2024

aerosouund force-pushed the kv-provider branch from 1ff08d2 to b54838d Compare August 23, 2024 09:03

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2024

aerosouund added 2 commits August 23, 2024 12:46

aerosouund force-pushed the kv-provider branch from b54838d to 28752d9 Compare August 23, 2024 09:46

aerosouund added 5 commits August 26, 2024 18:08

refactor: Delete the nodesconfig package

2080c4d

This functionality now exists in the KubevirtProvider type and doesn't need a package of its own Signed-off-by: aerosouund <aerosound161@gmail.com>

testing: Move testing logic to the providers package

8d61da0

The KubevirtProvider type is what provides the methods that run a node or run the k8s options. Testing logic has been moved to a Base Provider Suite Signed-off-by: aerosouund <aerosound161@gmail.com>

feat: Introduce retries in ssh connection in the ssh client

86c176f

Signed-off-by: aerosouund <aerosound161@gmail.com>

aerosouund force-pushed the kv-provider branch from 28752d9 to 86c176f Compare August 26, 2024 15:08

fix: Don't handle ssh shutdown error

ea8daed

Signed-off-by: aerosouund <aerosound161@gmail.com>

aerosouund mentioned this pull request Aug 28, 2024

add aerosouund to the KubeVirt org kubevirt/project-infra#3609

Merged

7 tasks

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2024

brianmcarey reviewed Sep 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KV provider #1230

KV provider #1230

aerosouund commented Jul 23, 2024

kubevirt-bot commented Jul 23, 2024

kubevirt-bot commented Jul 23, 2024

aerosouund commented Jul 23, 2024

dhiller commented Aug 26, 2024

kubevirt-bot commented Aug 26, 2024

aerosouund commented Aug 26, 2024 •

edited

Loading

kubevirt-bot commented Sep 9, 2024

brianmcarey Sep 10, 2024

aerosouund Sep 11, 2024

xpivarc commented Sep 11, 2024

aerosouund commented Sep 11, 2024 •

edited

Loading

		@@ -0,0 +1,6 @@
		[isv_kubernetes_addons_cri-o_stable_v1.28]
		name=CRI-O v1.28 (Stable) (rpm)

KV provider #1230

Are you sure you want to change the base?

KV provider #1230

Conversation

aerosouund commented Jul 23, 2024

kubevirt-bot commented Jul 23, 2024

kubevirt-bot commented Jul 23, 2024

aerosouund commented Jul 23, 2024

dhiller commented Aug 26, 2024

kubevirt-bot commented Aug 26, 2024

aerosouund commented Aug 26, 2024 • edited Loading

kubevirt-bot commented Sep 9, 2024

brianmcarey Sep 10, 2024

Choose a reason for hiding this comment

aerosouund Sep 11, 2024

Choose a reason for hiding this comment

xpivarc commented Sep 11, 2024

aerosouund commented Sep 11, 2024 • edited Loading

aerosouund commented Aug 26, 2024 •

edited

Loading

aerosouund commented Sep 11, 2024 •

edited

Loading