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

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

/test all

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

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

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

aerosouund · 2024-09-26T14:32:24Z

/hold

aerosouund · 2024-10-07T09:55:50Z

/test check-provision-k8s-1.29

aerosouund · 2024-10-07T09:58:49Z

/test check-provision-k8s-1.31

aerosouund · 2024-10-15T12:26:13Z

/test check-provision-k8s-1.29

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Signed-off-by: aerosouund <[email protected]>

This functionality now exists in the KubevirtProvider type and doesn't need a package of its own Signed-off-by: aerosouund <[email protected]>

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 <[email protected]>

Signed-off-by: aerosouund <[email protected]>

…uint16 All references to ports in the codebase use uint not uint16. There is no reason to keep the ports as they are Signed-off-by: aerosouund <[email protected]>

kubevirt-bot · 2024-10-31T13:37:34Z

@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-up-kind-1.27-vgpu	`28752d9`	link	false	`/test check-up-kind-1.27-vgpu`
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.

aerosouund · 2024-11-01T09:59:50Z

@brianmcarey @dhiller @xpivarc
The PR is finally ready, highly appreciate a review when you have time

kubevirt-bot · 2024-11-08T16:58:22Z

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-12-03T09:17:28Z

/test check-provision-k8s-1.30

brianmcarey

Thanks for this @aerosouund - just a couple of comments inline

/cc @xpivarc

brianmcarey · 2024-12-03T09:24:06Z

cluster-provision/gocli/pkg/libssh/ssh.go

+
+	operation := func() error {
+		client, err = ssh.Dial("tcp", net.JoinHostPort("127.0.0.1", fmt.Sprint(s.sshPort)), s.config)
+		return err


Why did you remove the more verbose error message here?

That's fine, I can reintroduce it. I probably just removed it unintentionally

brianmcarey · 2024-12-03T09:28:48Z

cluster-provision/gocli/providers/base_provider_test.go

@@ -35,6 +37,18 @@ var _ = Describe("Node Provisioning", func() {
 		}

 		k8sClient = k8s.NewTestClient(reactors...)
+		kp = NewKubevirtProvider("k8s-1.30", "", &client.Client{}, []KubevirtProviderOption{


Could we get k8s-1.30 dynamically from what folders are existing? This will fail when removing the k8s-1.30 provider?

No its unrelated to the folder and off the top of my mind it won't fail if we drop 1.30 altogether
the version validation logic has been moved to the run command, and since this is being defined in line then it wont be obstructed by this logic

cluster-provision/gocli/cmd/run.go:143

allowedK8sVersions := []string{"k8s-1.28", "k8s-1.29", "k8s-1.30", "k8s-1.31"} var validVersion bool for _, v := range allowedK8sVersions { if k8sVersion == v { validVersion = true } } if !validVersion { return fmt.Errorf("Invalid k8s version passed, please use one of k8s-1.28, k8s-1.29, k8s-1.30 or k8s-1.31") }

I don't think anywhere else in the code does this validation but I will double check

brianmcarey · 2024-12-03T09:33:02Z

cluster-provision/gocli/providers/provision.go

+	_, err = kp.Docker.ContainerCommit(ctx, node.ID, container.CommitOptions{
+		Reference: target,
+		Comment:   "PROVISION SUCCEEDED",
+		Author:    "gocli",


Should the author be "The KubeVirt Authors"?

Yeah, we can change it

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 force-pushed the kv-provider branch from b54838d to 28752d9 Compare August 23, 2024 09:46

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

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

cluster-provision/gocli/opts/k8sprovision/conf/crio-yum.repo Outdated Show resolved Hide resolved

aerosouund force-pushed the kv-provider branch from ea8daed to 234e2b1 Compare September 26, 2024 14:32

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

kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 26, 2024

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

aerosouund force-pushed the kv-provider branch from 890a184 to 7b5f284 Compare October 18, 2024 10:58

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 Oct 18, 2024

aerosouund force-pushed the kv-provider branch from 929a8f9 to 18d2f70 Compare October 19, 2024 17:20

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 Oct 25, 2024

aerosouund force-pushed the kv-provider branch 2 times, most recently from 361171e to 22465ce Compare October 30, 2024 13:58

aerosouund added 5 commits October 30, 2024 18:39

refactor: Rewrite provision.go to use the KubevirtProvider

06b4f3d

Signed-off-by: aerosouund <[email protected]>

refactor: Delete the nodesconfig package

23ce5d6

This functionality now exists in the KubevirtProvider type and doesn't need a package of its own Signed-off-by: aerosouund <[email protected]>

aerosouund force-pushed the kv-provider branch from 22465ce to 01aaf22 Compare October 30, 2024 15:40

aerosouund added 3 commits October 31, 2024 13:36

testing: Move testing logic to the providers package

97fb04d

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 <[email protected]>

feat: Introduce retries in ssh connection in the ssh client

9ab179c

Signed-off-by: aerosouund <[email protected]>

refactor: Change the return type of GetPublicPort to uint instead of …

6d52569

…uint16 All references to ports in the codebase use uint not uint16. There is no reason to keep the ports as they are Signed-off-by: aerosouund <[email protected]>

aerosouund force-pushed the kv-provider branch from 01aaf22 to 6d52569 Compare October 31, 2024 10:37

aerosouund mentioned this pull request Nov 6, 2024

Container control plane #1318

Draft

8 tasks

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

brianmcarey reviewed Dec 3, 2024

View reviewed changes

kubevirt-bot requested a review from xpivarc December 3, 2024 09:47

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

aerosouund commented Aug 26, 2024 •

edited

Loading

xpivarc commented Sep 11, 2024

aerosouund commented Sep 11, 2024 •

edited

Loading

aerosouund commented Sep 26, 2024

aerosouund commented Oct 7, 2024

aerosouund commented Oct 7, 2024

aerosouund commented Oct 15, 2024

kubevirt-bot commented Oct 31, 2024 •

edited

Loading

aerosouund commented Nov 1, 2024

kubevirt-bot commented Nov 8, 2024

brianmcarey commented Dec 3, 2024

brianmcarey left a comment

brianmcarey Dec 3, 2024

aerosouund Dec 3, 2024

brianmcarey Dec 3, 2024

aerosouund Dec 3, 2024 •

edited

Loading

brianmcarey Dec 3, 2024 •

edited

Loading

aerosouund Dec 3, 2024

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

aerosouund commented Aug 26, 2024 • edited Loading

xpivarc commented Sep 11, 2024

aerosouund commented Sep 11, 2024 • edited Loading

aerosouund commented Sep 26, 2024

aerosouund commented Oct 7, 2024

aerosouund commented Oct 7, 2024

aerosouund commented Oct 15, 2024

kubevirt-bot commented Oct 31, 2024 • edited Loading

aerosouund commented Nov 1, 2024

kubevirt-bot commented Nov 8, 2024

brianmcarey commented Dec 3, 2024

brianmcarey left a comment

Choose a reason for hiding this comment

brianmcarey Dec 3, 2024

Choose a reason for hiding this comment

aerosouund Dec 3, 2024

Choose a reason for hiding this comment

brianmcarey Dec 3, 2024

Choose a reason for hiding this comment

aerosouund Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

brianmcarey Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

aerosouund Dec 3, 2024

Choose a reason for hiding this comment

aerosouund commented Aug 26, 2024 •

edited

Loading

aerosouund commented Sep 11, 2024 •

edited

Loading

kubevirt-bot commented Oct 31, 2024 •

edited

Loading

aerosouund Dec 3, 2024 •

edited

Loading

brianmcarey Dec 3, 2024 •

edited

Loading