Add preflight check for vsock SSH port #3965

evidolob · 2024-01-19T14:15:53Z

Relates to: First part for #3855

Solution/Idea

PR's add preflight check for SSH port(2222) in not used before start.

Testing

Occupy 2222 port on your laptop( you may use nc -lkv 127.0.0.1 2222 command)
Start daemon
Try to start CRC VM
Ensure that you have error during preflight checks
Free port 2222
Try to start CRC again
Ensure that CRC starts normally

anjannath · 2024-01-25T08:20:57Z

working as expected, tested on macOS:

INFO Using bundle path /Users/anath/.crc/cache/crc_vfkit_4.14.8_arm64.crcbundle
INFO Checking if running as non-root
INFO Checking if crc-admin-helper executable is cached
INFO Checking if running on a supported CPU architecture
INFO Checking if crc executable symlink exists
INFO Checking minimum RAM requirements
INFO Checking if running emulated on Apple silicon
INFO Checking if vfkit is installed
INFO Checking if old launchd config for tray and/or daemon exists
INFO Checking if crc daemon plist file is present and loaded
INFO Checking SSH port availability
WARN Preflight checks failed during `crc start`, please try to run `crc setup` first in case you haven't done so yet
port 2222 already in use: listen tcp 127.0.0.1:2222: bind: address already in use

vyasgun

Just added a few minor comments

pkg/crc/preflight/preflight_checks_nonlinux.go

vyasgun · 2024-01-25T08:45:16Z

pkg/crc/preflight/preflight_checks_nonlinux.go

+		if err != nil {
+			return fmt.Errorf("port %d already in use: %s", constants.VsockSSHPort, err)
+		}
+		server.Close()


Suggested change

server.Close()

defer func() {

err = server.Close()

}()

Could be just return server.Close() ?

I think it's more conventional to use defer for closing connections.

vyasgun · 2024-01-25T08:45:29Z

pkg/crc/preflight/preflight_checks_nonlinux.go

+		server.Close()
+
+		// we successfully used and closed the port
+		return nil


Suggested change

return nil

return err

If the intent is to return the error from the deferred server.Close() call, I don't think this will work as expected:
https://go.dev/play/p/HyKboZE2dka

You are right. We can define the err variable as part of the function signature to set the correct return value in defer.
https://go.dev/play/p/8kaB3HQlfk6

cfergeau · 2024-01-25T10:58:22Z

pkg/crc/preflight/preflight_checks_nonlinux.go

+}
+
+func checkSSHPortFree() func() error {
+	return func() error {


Do you need this indirection? the preflights use the pattern func foo() func() error when we need to pass arguments to foo. In this case, I think you could directly use func checkSSHPortFree() error, and change check: checkSSHPortFree(), to check: checkSSHPortFree, ?

Thinking more about it, it actually would make sense to only run the preset when usermode networking is use (the network-mode config option). In which case you'd need additional arg to the check, and using this pattern would make sense :)

I don't know why this was closed as resolved, but this was neither discussed nor resolved.

@praveenkumar

cfergeau · 2024-01-25T11:08:42Z

pkg/crc/preflight/preflight_checks_nonlinux.go

+		server.Close()
+
+		// we successfully used and closed the port
+		return nil


If the intent is to return the error from the deferred server.Close() call, I don't think this will work as expected:
https://go.dev/play/p/HyKboZE2dka

cfergeau · 2024-01-25T11:08:54Z

pkg/crc/preflight/preflight_checks_nonlinux.go

+		if err != nil {
+			return fmt.Errorf("port %d already in use: %s", constants.VsockSSHPort, err)
+		}
+		server.Close()


Could be just return server.Close() ?

Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>

Co-authored-by: Gunjan Vyas <vyasgun20@gmail.com>

openshift-ci · 2024-02-08T09:36:13Z

@evidolob: 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
ci/prow/e2e-crc	`7e92185`	link	true	`/test e2e-crc`

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.

vyasgun · 2024-02-12T07:24:39Z

/approve

openshift-ci · 2024-02-12T07:24:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vyasgun
Once this PR has been reviewed and has the lgtm label, please ask for approval from evidolob. 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

pkg/crc/preflight/preflight_checks_nonlinux.go

evidolob requested review from gbraad, praveenkumar, cfergeau and anjannath January 19, 2024 14:15

evidolob force-pushed the check-ssh-port branch from ee58bc8 to 6e4e309 Compare January 22, 2024 11:00

vyasgun reviewed Jan 25, 2024

View reviewed changes

cfergeau reviewed Jan 25, 2024

View reviewed changes

evidolob and others added 2 commits February 8, 2024 09:32

add preflight check for vsock SSH port

f6060be

Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>

Change description message

7e92185

Co-authored-by: Gunjan Vyas <vyasgun20@gmail.com>

evidolob force-pushed the check-ssh-port branch from 25768d0 to 7e92185 Compare February 8, 2024 07:32

evidolob requested review from cfergeau and vyasgun February 8, 2024 07:32

praveenkumar added the has-to-be-in-release This PR need to go in coming release. label Feb 8, 2024

praveenkumar reviewed Feb 12, 2024

View reviewed changes

pkg/crc/preflight/preflight_checks_nonlinux.go Show resolved Hide resolved

praveenkumar merged commit 7929737 into crc-org:main Feb 12, 2024
18 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preflight check for vsock SSH port #3965

Add preflight check for vsock SSH port #3965

evidolob commented Jan 19, 2024

anjannath commented Jan 25, 2024

vyasgun left a comment

vyasgun Jan 25, 2024

cfergeau Jan 25, 2024

vyasgun Feb 7, 2024

vyasgun Jan 25, 2024

cfergeau Jan 25, 2024

vyasgun Feb 7, 2024

cfergeau Jan 25, 2024

cfergeau Jan 25, 2024

cfergeau Feb 12, 2024

cfergeau Feb 12, 2024

cfergeau Jan 25, 2024

cfergeau Jan 25, 2024

openshift-ci bot commented Feb 8, 2024

vyasgun commented Feb 12, 2024

openshift-ci bot commented Feb 12, 2024

-		server.Close()
+		defer func() {
+			err = server.Close()
+		}()

Add preflight check for vsock SSH port #3965

Add preflight check for vsock SSH port #3965

Conversation

evidolob commented Jan 19, 2024

Solution/Idea

Testing

anjannath commented Jan 25, 2024

vyasgun 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 8, 2024

vyasgun commented Feb 12, 2024

openshift-ci bot commented Feb 12, 2024