Library based ssh #1209

aerosouund · 2024-06-25T11:30:54Z

What this PR does / why we need it:
Replaces ssh.sh in run.go with golang library based ssh that goes directly to the node rather than the node container as a middle man

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

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:

Use library based ssh in run.go

kubevirt-bot · 2024-06-25T11:31:03Z

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.

aerosouund · 2024-06-25T11:31:38Z

@xpivarc @brianmcarey @dhiller

cluster-provision/gocli/utils/ssh/ssh.go

cluster-provision/gocli/cmd/run.go

cluster-provision/gocli/utils/ssh/ssh.go

acardace · 2024-06-27T08:44:28Z

cluster-provision/gocli/utils/ssh/ssh.go

+	if err != nil {
+		return "", fmt.Errorf("Error creating forwarded ssh connection: %s", err)
+	}
+	jumpHost := ssh.NewClient(ncc, chans, reqs)


In general I'd prefer if you moved all this init and config part into a separate function that gets executed once per SSHClient instantiation, I'd expect that you establish the SSH connection once and keep it alive for the lifetime of the SSHClient struct.

In practice I'd have modify the interface to have a Command() function, this way you'd establish the connection once when you instantiate the ssh client and then Command() would just sent commands using the open ssh session and a Close() function that closes the socket. This way you have 1 socket per connection instead of 1 per command.

That said, this is an optimization, take it a as follow-up work.

Acknowledged. i think its a good idea as well
we can implement it in a follow up PR

cluster-provision/gocli/utils/ssh/ssh.go

xpivarc · 2024-06-26T14:40:04Z

cluster-provision/gocli/utils/ssh/key.go

@@ -0,0 +1,29 @@
+package utils


dbfa492 commit message. The part about using struct and what it takes seems redundant. A more interesting part would be to see why we need this package. Something general/summary as below:

Example:

Introduce SSH package The package provides simple interface for SSH to specific node and implementation of this interface. The implementation is based on std ssh package and we use the SSH port exposed on master node. Jump is used to SSH to other nodes.

xpivarc · 2024-06-26T14:41:34Z

cluster-provision/gocli/cmd/run.go

@@ -3,7 +3,6 @@ package cmd
 import (
 	"bufio"
 	"bytes"
-	"errors"


Please adjust commit messages to something like https://www.conventionalcommits.org/en/v1.0.0/

cluster-provision/gocli/utils/ssh/ssh.go

cluster-provision/gocli/docker/adapter.go

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

xpivarc

Few more questions/comment but looks good to me

cluster-provision/gocli/utils/ssh/ssh.go

xpivarc · 2024-07-04T08:56:50Z

cluster-provision/gocli/cmd/run.go


-			if !success {
-				return fmt.Errorf("provisioning node %s failed", nodeName)
+		if x+1 == 1 {


This is change of logic, you should preserve the logic and change this in next commit. I think this is fine nevertheless, @brianmcarey wdyt?

cluster-provision/gocli/cmd/run.go

xpivarc · 2024-07-05T08:08:19Z

cluster-provision/gocli/cmd/run.go

+		}
+
+		// move the scripts to the same location they were in in the nodecontainer to enable command abstraction
+		for _, cmd := range []string{"sudo mkdir /scripts", "sudo cp -r /home/vagrant/scripts/* /scripts"} {


Why can't we do this in previous command?

you mean sudo mkdir /scripts && sudo cp -r /home/vagrant/scripts/* /scripts ?

I mean the previous command (scp) -r /scripts [email protected]%d:/home/vagrant/scripts moves the scripts to the VM and here we move them within the VM. Why can't we set the location right with the scp?

@xpivarc
Because editing the root directory requires sudo privileges and the root user does not have an ssh key so you cant scp as him

Ah make sense, I'll have one more pass

xpivarc · 2024-07-08T13:48:57Z

@dhiller @brianmcarey Can you help me merge this one (missing approver right on this repo :) )

brianmcarey · 2024-07-11T10:11:01Z

/cc

Keep ssh in a single place by leveraging the adapter and passing it as a param for functions that did docker exec themselves; prepareDeviceForAssignment and prepareEtcdDataMount. Replace the logic for checking if there's a special provision script for a node to running the script for node01 at index 1 Signed-off-by: aerosouund <[email protected]>

The config package introduces holder structs that contain the params for a node and the k8s optional deployments on a kubevirt provider. To pass this config directly to other methods without having to pass every parameter to the functions that run the node or k8s options Signed-off-by: aerosouund <[email protected]>

Package the logic of node launching and configuring its linux system into provisionNode and running the optional k8s deployments into provisionK8sOptions. Both functions take as a parameter an ssh client and a struct that represents a holder to their config from the config package Signed-off-by: aerosouund <[email protected]>

Introduce a testing package based on mock that tests the provisionNode and provisionK8sOptions methods and include the logic to run this package in the main test suite of the cmd package Signed-off-by: aerosouund <[email protected]>

Use an implementation based on ssh'ing directly to nodes rather than through their node container. Move the scripts directory on the node during provisioning and remove it after finishing Signed-off-by: aerosouund <[email protected]>

dhiller · 2024-07-15T08:57:51Z

/test check-provision-k8s-1.28

dhiller · 2024-07-15T08:58:00Z

/test check-provision-k8s-1.30

dhiller

Thank you for this work @aerosouund !

/approve

dhiller · 2024-07-15T09:25:52Z

/lgtm

brianmcarey · 2024-07-15T09:27:10Z

/test ?

kubevirt-bot · 2024-07-15T09:27:15Z

@brianmcarey: The following commands are available to trigger required jobs:

/test check-provision-alpine-with-test-tooling
/test check-provision-centos-base
/test check-provision-k8s-1.28
/test check-provision-k8s-1.29
/test check-provision-k8s-1.30
/test check-provision-manager
/test check-up-kind-ovn
/test check-up-kind-sriov

The following commands are available to trigger optional jobs:

/test check-gocli
/test check-up-kind-1.27-vgpu
/test check-up-kind-1.30-vgpu

Use /test all to run the following jobs that were automatically triggered:

check-provision-alpine-with-test-tooling
check-provision-k8s-1.28
check-provision-k8s-1.29
check-provision-k8s-1.30
check-provision-manager
check-up-kind-1.27-vgpu
check-up-kind-1.30-vgpu
check-up-kind-sriov

In response to this:

/test ?

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-07-15T09:27:41Z

/test check-gocli
/test check-provision-centos-base

kubevirt-bot · 2024-07-15T12:42:13Z

@aerosouund: 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	`1c993f7`	link	false	`/test check-up-kind-1.27-vgpu`

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.

brianmcarey

/approve

thanks @aerosouund !

kubevirt-bot · 2024-07-16T06:36:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianmcarey, dhiller

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~~ [brianmcarey,dhiller]

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

brianmcarey · 2024-07-16T06:36:56Z

/override check-provision-alpine-with-test-tooling

Not related to these changes.

kubevirt-bot · 2024-07-16T06:36:59Z

@brianmcarey: Overrode contexts on behalf of brianmcarey: check-provision-alpine-with-test-tooling

In response to this:

/override check-provision-alpine-with-test-tooling

Not related to these changes.

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.

[a087e7e Automatic bump of CentOS Stream to latest](kubevirt/kubevirtci#1219) [83e15cc alpine,image: Use full image name for qemu-user-static image](kubevirt/kubevirtci#1218) [aa7d876 Library based ssh](kubevirt/kubevirtci#1209) [c43f010 fix missing whitespace](kubevirt/kubevirtci#1216) [9bb50bf enable vm-image-builder for s390x.](kubevirt/kubevirtci#1207) ```release-note NONE ``` Signed-off-by: kubevirt-bot <[email protected]>

xpivarc · 2024-07-17T08:04:36Z

cluster-provision/gocli/cmd/run.go

+	cmds := []string{
+		fmt.Sprintf("mkdir -p %s", etcdDataDir),
+		fmt.Sprintf("test -d %s", etcdDataDir),
+		fmt.Sprintf("mount -t tmpfs -o size=%s tmpfs %s", mountSize, etcdDataDir),


@aerosouund This needs to run with sudo, see failure in kubevirt/kubevirt#12310

[f3a5054 fix: Run etcd commads with sudo](kubevirt/kubevirtci#1223) [a087e7e Automatic bump of CentOS Stream to latest](kubevirt/kubevirtci#1219) [83e15cc alpine,image: Use full image name for qemu-user-static image](kubevirt/kubevirtci#1218) [aa7d876 Library based ssh](kubevirt/kubevirtci#1209) [c43f010 fix missing whitespace](kubevirt/kubevirtci#1216) [9bb50bf enable vm-image-builder for s390x.](kubevirt/kubevirtci#1207) ```release-note NONE ``` Signed-off-by: kubevirt-bot <[email protected]>

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

kubevirt-bot requested review from jean-edouard and ormergi June 25, 2024 11:30

kubevirt-bot added the size/L label Jun 25, 2024

xpivarc reviewed Jun 25, 2024

View reviewed changes

aerosouund force-pushed the library-based-ssh branch 2 times, most recently from c99528b to b5ea285 Compare June 25, 2024 16:52

acardace reviewed Jun 27, 2024

View reviewed changes

aerosouund force-pushed the library-based-ssh branch from b5ea285 to 8c487c9 Compare June 27, 2024 14:06

aerosouund requested review from acardace and xpivarc June 27, 2024 14:07

aerosouund force-pushed the library-based-ssh branch from 8c487c9 to 6add7e6 Compare June 29, 2024 13:21

kubevirt-bot added size/XXL and removed size/L labels Jun 29, 2024

aerosouund force-pushed the library-based-ssh branch from 6add7e6 to 8d12b8d Compare June 29, 2024 13:32

xpivarc reviewed Jul 1, 2024

View reviewed changes

aerosouund force-pushed the library-based-ssh branch 3 times, most recently from cb7fecc to 8a3bd30 Compare July 4, 2024 10:29

chore: Vendoring commit

14809ad

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

aerosouund force-pushed the library-based-ssh branch from 8a3bd30 to 101007b Compare July 4, 2024 12:10

xpivarc reviewed Jul 5, 2024

View reviewed changes

aerosouund force-pushed the library-based-ssh branch from 101007b to 4db5c1f Compare July 5, 2024 08:33

aerosouund force-pushed the library-based-ssh branch from 4db5c1f to f1cf73c Compare July 9, 2024 13:27

kubevirt-bot requested a review from brianmcarey July 11, 2024 10:11

xpivarc mentioned this pull request Jul 11, 2024

kubevirtci, cluster-up: move cluster spin up logic into gocli #1110

Open

aerosouund force-pushed the library-based-ssh branch from 8a8315a to bb55f9c Compare July 13, 2024 12:11

aerosouund added 4 commits July 14, 2024 12:14

test: Testing logic for ssh

f0f8adf

Introduce a testing package based on mock that tests the provisionNode and provisionK8sOptions methods and include the logic to run this package in the main test suite of the cmd package Signed-off-by: aerosouund <[email protected]>

aerosouund force-pushed the library-based-ssh branch from bb55f9c to 1c993f7 Compare July 14, 2024 09:41

dhiller reviewed Jul 15, 2024

View reviewed changes

kubevirt-bot assigned dhiller Jul 15, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2024

brianmcarey reviewed Jul 16, 2024

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2024

kubevirt-bot merged commit aa7d876 into kubevirt:main Jul 16, 2024
11 of 12 checks passed

kubevirt-bot mentioned this pull request Jul 17, 2024

Bump kubevirtci kubevirt/kubevirt#12310

Merged

xpivarc reviewed Jul 17, 2024

View reviewed changes

dhiller mentioned this pull request Jul 17, 2024

ssh issue wrt sudo when using etcd in-memory #1222

Closed

brianmcarey mentioned this pull request Jul 29, 2024

Adding s390x arch support in k8s 1.28 provider #1201

Closed

8 tasks

aerosouund mentioned this pull request Aug 28, 2024

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

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Library based ssh #1209

Library based ssh #1209

aerosouund commented Jun 25, 2024

kubevirt-bot commented Jun 25, 2024

aerosouund commented Jun 25, 2024

acardace Jun 27, 2024 •

edited

Loading

aerosouund Jun 27, 2024

xpivarc Jun 26, 2024

xpivarc Jun 26, 2024

xpivarc left a comment

xpivarc Jul 4, 2024

xpivarc Jul 5, 2024

aerosouund Jul 5, 2024

xpivarc Jul 8, 2024

aerosouund Jul 8, 2024 •

edited

Loading

xpivarc Jul 8, 2024

xpivarc commented Jul 8, 2024

brianmcarey commented Jul 11, 2024

dhiller commented Jul 15, 2024

dhiller commented Jul 15, 2024

dhiller left a comment

dhiller commented Jul 15, 2024

brianmcarey commented Jul 15, 2024

kubevirt-bot commented Jul 15, 2024

brianmcarey commented Jul 15, 2024

kubevirt-bot commented Jul 15, 2024 •

edited

Loading

brianmcarey left a comment

kubevirt-bot commented Jul 16, 2024

brianmcarey commented Jul 16, 2024

kubevirt-bot commented Jul 16, 2024

xpivarc Jul 17, 2024

Library based ssh #1209

Library based ssh #1209

Conversation

aerosouund commented Jun 25, 2024

kubevirt-bot commented Jun 25, 2024

aerosouund commented Jun 25, 2024

acardace Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpivarc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerosouund Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpivarc commented Jul 8, 2024

brianmcarey commented Jul 11, 2024

dhiller commented Jul 15, 2024

dhiller commented Jul 15, 2024

dhiller left a comment

Choose a reason for hiding this comment

dhiller commented Jul 15, 2024

brianmcarey commented Jul 15, 2024

kubevirt-bot commented Jul 15, 2024

brianmcarey commented Jul 15, 2024

kubevirt-bot commented Jul 15, 2024 • edited Loading

brianmcarey left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Jul 16, 2024

brianmcarey commented Jul 16, 2024

kubevirt-bot commented Jul 16, 2024

Choose a reason for hiding this comment

acardace Jun 27, 2024 •

edited

Loading

aerosouund Jul 8, 2024 •

edited

Loading

kubevirt-bot commented Jul 15, 2024 •

edited

Loading