Connectivity diagnostic with VC/ESXi from appliance #3210

vburenin · 2016-11-17T17:27:02Z

INFO[2016-11-17T09:21:25-08:00] Waiting for IP information
INFO[2016-11-17T09:21:31-08:00] Waiting for major appliance components to launch

INFO[2016-11-17T09:21:31-08:00] Checking connectivity with the target vCenter/ESXi
INFO[2016-11-17T09:21:31-08:00] VC/ESXi API Ping Test: https://10.0.1.152 is reacheable and responds to ICMP ping request
INFO[2016-11-17T09:21:31-08:00] VC/ESXi API Test: https://10.0.1.152 API responds as expected

INFO[2016-11-17T09:21:34-08:00] Initialization of appliance successful
INFO[2016-11-17T09:21:34-08:00]
INFO[2016-11-17T09:21:34-08:00] VCH Admin Portal:
INFO[2016-11-17T09:21:34-08:00] https://10.0.1.156:2378
INFO[2016-11-17T09:21:34-08:00]
INFO[2016-11-17T09:21:34-08:00] Published ports can be reached at:
INFO[2016-11-17T09:21:34-08:00] 10.0.1.156
INFO[2016-11-17T09:21:34-08:00]
INFO[2016-11-17T09:21:34-08:00] Docker environment variables:
INFO[2016-11-17T09:21:34-08:00] DOCKER_HOST=10.0.1.156:2375
INFO[2016-11-17T09:21:34-08:00]
INFO[2016-11-17T09:21:34-08:00] Environment saved in vic-docker/vic-docker.env
INFO[2016-11-17T09:21:34-08:00]
INFO[2016-11-17T09:21:34-08:00] Connect to docker:
INFO[2016-11-17T09:21:34-08:00] docker -H 10.0.1.156:2375 info
INFO[2016-11-17T09:21:34-08:00] Installer completed successfully

INFO[2016-11-17T09:19:08-08:00] Waiting for IP information
INFO[2016-11-17T09:19:13-08:00] Waiting for major appliance components to launch
INFO[2016-11-17T09:19:13-08:00] Checking connectivity with the target vCenter/ESXi
ERRO[2016-11-17T09:19:14-08:00] VC/ESXi API Ping Test: https://brokenesx is unknown host name
INFO[2016-11-17T09:19:14-08:00] Collecting ha-host hostd.log
ERRO[2016-11-17T09:19:14-08:00] --------------------
ERRO[2016-11-17T09:19:14-08:00] vic-machine-linux failed: Access to VC/ESXi failed a ping test

Fixes #3066

# Conflicts: # cmd/vic-machine/create/create.go

…c-diagnostics

cgtexmex · 2016-11-17T17:52:51Z

It would be nice if the error message vic-machine-linux failed: Access to VC/ESXi failed a ping test indicated that the issue is between the appliance and vc/ESXi and not some where else...potentially even listing the source and target.

vburenin · 2016-11-17T18:32:44Z

@cgtexmex Good point! I would really appreciate if people could take a look at error texts to rephrase them in a more human friendly way.

caglar10ur

This is not portable, there is no guarantee that ping binary will be there to use. Also parsing a command output is show stopper for me. Please consider using golang.org/x/net/icmp. Eg; bonneville-appliance (internal reference) https://enatai-git.eng.vmware.com/bonneville/bonneville-appliance/blob/master/setup.go#L74

vburenin · 2016-11-17T20:02:29Z

@caglar10ur golang.org/x/net/icmp is a new vendored dependency. I can create an issue to ICMP when vendor is unlocked.

vburenin · 2016-11-17T20:37:55Z

@caglar10ur #3219

it a hard piece for this PR. Here is a new one specifically for your request: #3219

fdawg4l · 2016-11-17T20:52:14Z

vendor/github.com/vburenin/nsync/cwaitgroup.go

+// such as connections, opened files, etc. Thus, ControlWaitGroup fits well to limit the number
+// of running goroutines.
+type ControlWaitGroup struct {
+	sem     *Semaphore


Where is this defined?

fdawg4l · 2016-11-17T21:00:56Z

vendor/github.com/vburenin/nsync/cwaitgroup.go

+// dynamic so it only make sense to use it for logging purposes.
+func (cwg *ControlWaitGroup) Waiting() int {
+	cwg.mu.Lock()
+	w := cwg.waiting


If Semaphore is a buffered channel where the channel size is the value of the semaphore, then waiting just turns into value - len(semaphor).

Also using a buffered channel means you don't need mu.

oh, it's accidentally sneaked in. I will remove this file.

caglar10ur · 2016-11-17T21:20:05Z

using icmp pkg produces way less code than this diag pkg (and much easier to wortk with IMHO) so I'm still not sure why we prefer that. From that link above

// PrivilegedPing sends a ipv4.ICMPTypeEcho message to the given IP and returns true if it responds with a ipv4.ICMPTypeEchoReply
// Terminates if an error occurs
func PrivilegedPing(ip net.IP, seq int) bool {
    c, err := icmp.ListenPacket("ip4:icmp", "0.0.0.0")
    if err != nil {
        GuestRPCErrorf("Unable to listen: %s", err)
        os.Exit(ERR)
    }
    defer c.Close()

    // craft the icmp message
    wm := icmp.Message{
        Type: ipv4.ICMPTypeEcho,
        Code: 0,
        Body: &icmp.Echo{
            ID:   os.Getpid() & 0xffff,
            Seq:  1 << uint(seq),
            Data: []byte("KNOCK-IF-YOU-CAN-HEAR-ME"),
        },
    }

    // marshal it to write buffer
    wb, err := wm.Marshal(nil)
    if err != nil {
        GuestRPCErrorf("Unable to marshall the icmp message: %s", err)
        os.Exit(ERR)
    }

    dst := &net.IPAddr{IP: ip}
    // write it to wire
    if n, err := c.WriteTo(wb, dst); err != nil {
        GuestRPCErrorf("Unable to write: %s", err)
        os.Exit(ERR)
    } else if n != len(wb) {
        GuestRPCErrorf("Unable to write: got %v; want %v", n, len(wb))
        os.Exit(ERR)
    }

    // create read buffer
    rb := make([]byte, 1500)

    // Set read deadling to 3 sec.
    if err := c.SetReadDeadline(time.Now().Add(3 * time.Second)); err != nil {
        GuestRPCErrorf("Unable to set deadline: %s", err)
        os.Exit(ERR)
    }

    // read it from wire to read buffer
    n, _, err := c.ReadFrom(rb)
    if err != nil {
        GuestRPCErrorf("Unable to read: %s", err)
        os.Exit(ERR)
    }

    // parse the icmp message
    rm, err := icmp.ParseMessage(iana.ProtocolICMP, rb[:n])
    if err != nil {
        GuestRPCErrorf("Unable to parse: %s", err)
        os.Exit(ERR)
    }

    // type assert to see whether we have a response
    switch rm.Type {
    case ipv4.ICMPTypeEchoReply:
        return true
    default:
        return false
    }
}

but I'm fine with this if others are OK with it so I'll let them to decide

vburenin · 2016-11-17T21:27:26Z

@caglar10ur it is just PING for IPv4. I will need to take care about name resolution, IPv6, etc. I agreed that this needs to be done, but not today. Issue has been filed, so I will take care about this after this PR for the next release.

caglar10ur · 2016-11-17T21:36:25Z

we don't support anything other than ipv4 at this time and we have net.LookupAddr for name resolution, just saying...

hmahmood · 2016-11-18T01:04:20Z

cmd/vic-machine/create/create.go

+		return fmt.Errorf("Could not run network diagnostic on appliance")
+	}
+
+	// In case of fatal error, log error and exist.


s/exist/exit

hmahmood · 2016-11-18T01:05:26Z

cmd/vic-machine/create/create.go

+	}
+
+	// Checking if VC/ESXi can respond to ping request.
+	const pingTestTxt = "VC/ESXi API Ping Test:"


I don't think we need to log that we're running a ping test. We could just report that we can't reach the VCH if the ping test fails.

well, I am on the other side about it ;)

hmahmood · 2016-11-18T17:38:16Z

pkg/diag/diag.go

+
+const (
+	// PingStatusOk Host name was resolved and all pings went through.
+	PingStatusOk = 0


I am not sure we need such detailed information about the ping output. How about just the output and exit code from the command, and the client can figure out what went wrong? In our case, we just need to know if the exit code != 0 to report bad connectivity.

Giving more detailed diagnostics is better I think.

vburenin · 2016-11-18T18:43:31Z

@caglar10ur This code example is not the good one. It doesn't take into account so many things: It doesn't check reponse ID, doesn't check sequence, doesn't track sequences that are being sent and received, it doesn't take into account possible intermitent delays and that packets may be delivered in different order, etc. I am working on my own code for this - but it is already a large one. Definitely not for this PR.

net.LookupAddr -> net.LookupIP

hickeng · 2016-11-18T19:07:40Z

cmd/vic-machine/create/create.go

@@ -1160,9 +1157,66 @@ func (c *Create) Run(cliContext *cli.Context) (err error) {
 		return err
 	}

+	// vic-init will try to reach out to the vCenter/ESXi host.
+	log.Info("Checking connectivity with the target vCenter/ESXi")


This message is unclear:
Checking VCH connectivity with vSphere target

hickeng · 2016-11-18T19:12:34Z

cmd/vic-machine/create/create.go

+
+	// Checking if VC/ESXi can respond to ping request.
+	const pingTestTxt = "VC/ESXi API Ping Test:"
+	cd, err := executor.CheckVCPingFromAppliance(ctx, vch, vchConfig.Target)


Don't put a ping test in here - there are plenty of people who will be blocking ICMP packets.
The only thing we care about is whether the endpointVM can talk to vSphere API - just the HEAD request will suffice.

Ping is solely for diagnostics in the failure case.

No ICMP responses is totally ok test result. It is not in the fatal category, it will be just logged on warning level saying ping is not working.

…c-diagnostics

hickeng · 2016-11-18T19:41:56Z

@vburenin
This issue is about providing a means to determine:
a. can the target be resolved?
b. can we get an HTTP response from the target?

Additional diagnostics to aid in troubleshooting don't belong in vic-machine but in vic-admin.
If we wish to be able to report health status via vic-machine in general, that's a completely different PR.

I'd much prefer we add a structured mechanism to allow the components to report status, rather than add narrow checks like this into vic-machine. Done properly this can be leveraged to report health via vSphere alerts and in vic-admine as well as in all the other vic-machine operations.
Until we have a structured mechanism my request are:

drop/icebox this PR in it's current form
modify components to update the vchconfig "status" field for the session with an appropriate (concise) error message if connection to vsphere fails (and potentially other fatal paths), and another message e.g. "initialization successful", when the portlayer is fully up
modify ensureApplianceInitializes to recognize the success message and wait for that in vic-machine, reporting and error and existing if an unexpected status is found.

This allows for reporting a MUCH broader range of errors, with the errors determined by inline product code rather than sideline diagnostics. This status can also be checked by vic-machine ls and vic-machine inspect to provide a quick gauge of health.

hickeng · 2016-11-18T19:43:36Z

pkg/diag/diag.go

@@ -0,0 +1,229 @@
+// Copyright 2016 VMware, Inc. All Rights Reserved.


This should be under pkg/vsphere - it's specifically checking the vsphere API

hickeng · 2016-11-18T19:47:23Z

pkg/diag/diag.go

+// CheckPing runs ping to check if vSphere target is resolvable and pingable.
+func CheckPing(hn string) int {
+	cmd := exec.Command("ping", "-c", "4", "-W", "3", "-i", "0.1", hn)
+	return runPing(cmd.CombinedOutput())


This is likely to fail intermittently as it's in a race with the childReaper.

@hickeng hm. Any suggestion on that?

…one later.

vburenin · 2016-11-18T21:13:35Z

@hickeng I reduced scope of vSphere API test to focus purely on responses from client.Get
The rest will can do next time.

hmahmood · 2016-11-19T00:00:38Z

pkg/vsphere/diag/diag.go

+	if err != nil {
+		errTxt := err.Error()
+		op.Errorf("Query error: %s", err)
+		if strings.Contains(errTxt, "no such host") {


Is it possible to actually check for errno's here instead of comparing error strings?

Nope. I wish it was the case.

hmahmood · 2016-11-19T00:15:26Z

lgtm

andrewtchin · 2016-11-19T04:56:09Z

lgtm

vburenin added 3 commits November 17, 2016 10:44

Check VC/ESXi availability from the appliance/

3ced0ad

Merge branch 'master' into basic-diagnostics

b60cdb3

# Conflicts: # cmd/vic-machine/create/create.go

Better diagnostic text.

00c9d5d

vmwclabot added the cla-not-required label Nov 17, 2016

vburenin added 3 commits November 17, 2016 11:27

Merge branch 'master' into basic-diagnostics

ab29cbc

Fixed "make check" complains.

e835e9b

Merge branch 'basic-diagnostics' of github.com:vburenin/vic into basi…

25c5f70

…c-diagnostics

Added mention of appliance in the error text.

7588c42

caglar10ur previously requested changes Nov 17, 2016

View reviewed changes

vburenin mentioned this pull request Nov 17, 2016

Implement own ICMP test to do diagnostics instead of running "ping" command. #3219

Closed

fdawg4l reviewed Nov 17, 2016

View reviewed changes

removed accidentally added file.

6146421

Merge branch 'master' into basic-diagnostics

43b5321

hmahmood reviewed Nov 18, 2016

View reviewed changes

hickeng reviewed Nov 18, 2016

View reviewed changes

vburenin added 2 commits November 18, 2016 13:35

Updated logs and comments to refer vSphere instead of ESXi/vCenter.

f059080

Merge branch 'basic-diagnostics' of github.com:vburenin/vic into basi…

9ecc720

…c-diagnostics

hickeng reviewed Nov 18, 2016

View reviewed changes

Removed ping test due to high complexity of implementation. Will be d…

7c1c3db

…one later.

vburenin added 2 commits November 18, 2016 15:36

Merge branch 'master' into basic-diagnostics

0fc34a8

Merge branch 'master' into basic-diagnostics

80c8cca

hmahmood reviewed Nov 19, 2016

View reviewed changes

Merge branch 'master' into basic-diagnostics

6245945

vburenin merged commit 4645870 into vmware:master Nov 19, 2016

vburenin deleted the basic-diagnostics branch November 19, 2016 05:36

stuclem mentioned this pull request Nov 24, 2016

Document that vic-machine checks connectivity and name resolution #3092

Closed

stuclem mentioned this pull request Mar 21, 2017

Document that vic-machine checks connectivity and name resolution vmware/vic-product#53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connectivity diagnostic with VC/ESXi from appliance #3210

Connectivity diagnostic with VC/ESXi from appliance #3210

vburenin commented Nov 17, 2016

cgtexmex commented Nov 17, 2016

vburenin commented Nov 17, 2016

caglar10ur left a comment •

edited

Loading

vburenin commented Nov 17, 2016

vburenin commented Nov 17, 2016

fdawg4l Nov 17, 2016

fdawg4l Nov 17, 2016

fdawg4l Nov 17, 2016

vburenin Nov 17, 2016

caglar10ur commented Nov 17, 2016

vburenin commented Nov 17, 2016

caglar10ur commented Nov 17, 2016

hmahmood Nov 18, 2016

hmahmood Nov 18, 2016

vburenin Nov 18, 2016

hmahmood Nov 18, 2016

vburenin Nov 18, 2016

vburenin commented Nov 18, 2016

hickeng Nov 18, 2016

hickeng Nov 18, 2016

vburenin Nov 18, 2016

hickeng commented Nov 18, 2016 •

edited

Loading

hickeng Nov 18, 2016

hickeng Nov 18, 2016

vburenin Nov 18, 2016 •

edited

Loading

vburenin commented Nov 18, 2016

hmahmood Nov 19, 2016

vburenin Nov 19, 2016

hmahmood commented Nov 19, 2016

andrewtchin commented Nov 19, 2016

		@@ -0,0 +1,229 @@
		// Copyright 2016 VMware, Inc. All Rights Reserved.

Connectivity diagnostic with VC/ESXi from appliance #3210

Connectivity diagnostic with VC/ESXi from appliance #3210

Conversation

vburenin commented Nov 17, 2016

cgtexmex commented Nov 17, 2016

vburenin commented Nov 17, 2016

caglar10ur left a comment • edited Loading

Choose a reason for hiding this comment

vburenin commented Nov 17, 2016

vburenin commented Nov 17, 2016

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

caglar10ur commented Nov 17, 2016

vburenin commented Nov 17, 2016

caglar10ur commented Nov 17, 2016

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

vburenin commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hickeng commented Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vburenin Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

vburenin commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmahmood commented Nov 19, 2016

andrewtchin commented Nov 19, 2016

caglar10ur left a comment •

edited

Loading

hickeng commented Nov 18, 2016 •

edited

Loading

vburenin Nov 18, 2016 •

edited

Loading