Update VCH inspect API to output TLS friendly docker host and admin portal address by AngieCris · Pull Request #7533 · vmware/vic

AngieCris · 2018-03-16T20:48:49Z

[specific ci=6-09-Inspect]

Update VCH inspect API to output TLS friendly docker host and admin portal address
Include --compute-resource for vic-machine commands in robot tests

API related tests are going to be in PR: #7290
(#7290 is blocked by this PR)

codecov-io · 2018-03-16T21:21:07Z

Codecov Report

Merging #7533 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #7533     +/-   ##
=========================================
- Coverage   31.51%   31.42%   -0.1%     
=========================================
  Files         281      281             
  Lines       42020    42042     +22     
=========================================
- Hits        13244    13210     -34     
- Misses      27616    27671     +55     
- Partials     1160     1161      +1

Impacted Files	Coverage Δ
lib/install/management/inspect.go	`12.65% <0%> (-0.24%)`	⬇️
...piservers/service/restapi/handlers/vch_list_get.go	`0% <0%> (ø)`	⬆️
lib/apiservers/service/restapi/handlers/common.go	`0% <0%> (ø)`	⬆️
lib/apiservers/service/restapi/handlers/vch_get.go	`0% <0%> (ø)`	⬆️
lib/portlayer/attach/communication/lazy.go	`68.18% <0%> (-22.73%)`	⬇️
lib/portlayer/attach/communication/connector.go	`54.2% <0%> (-7.48%)`	⬇️
lib/dns/cache.go	`89.85% <0%> (-4.35%)`	⬇️
lib/portlayer/attach/communication/interactor.go	`24.08% <0%> (-2.92%)`	⬇️
pkg/logmgr/logmgr.go	`67.36% <0%> (+1.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74f673f...37ff7fe. Read the comment docs.

zjs · 2018-03-16T21:25:24Z

lib/apiservers/service/restapi/handlers/vch_get.go

-	model.Runtime.DockerHost, model.Runtime.AdminPortal = getAddresses(vchConfig)
+	model.Runtime.DockerHost, model.Runtime.AdminPortal, err = getAddresses(executor, vchConfig)
+	if err != nil {
+		op.Warn("No client IP assigned.")


How about logging err instead of "No client IP assigned." (In this case, it's basically the same message, but if we ever add another error to getAddresses, this might be misleading.)

Thanks! fixed

zjs · 2018-03-16T21:26:01Z

lib/apiservers/service/restapi/handlers/vch_list_get.go

-			dockerHost, adminPortal = getAddresses(vchConfig)
+			dockerHost, adminPortal, err = getAddresses(executor, vchConfig)
+			if err != nil {
+				op.Warnf("No client IP address assigned for VCH %s", id)


As above; we should base the log message on err instead.

zjs · 2018-03-16T21:27:48Z

lib/apiservers/service/restapi/handlers/common.go

+
+		dockerHost := ""
+		if !vchConfig.HostCertificate.IsNil() {
+			dockerHost = fmt.Sprintf("%s:%d", hostIP, opts.DefaultTLSHTTPPort)


I think the pattern from the old code, where we set a dockerPort based on vchConfig.HostCertificate.IsNil() and then have a single dockerHost = fmt.Sprintf("%s:%d", publicIP, dockerPort) is a little cleaner.

mhagen-vmware

lgtm, probably appropriate to add a negative test that doesn't include the resource pool now? I assume it throws an error now?

AngieCris · 2018-03-19T15:51:50Z

@mhagen-vmware This change is to update API to behave the same as CLI, the API related tests will be added later in #7290, including both positive and negative tests.
The change on CLI is just some refactoring, the behavior stays the same, so I didn't touch the 6-09-Inspect test suite. But running 6-09 on CI now throws an error if --resource-pool is not given, so I added that to the test suite.

zjs

But running 6-09 on CI now throws an error if --resource-pool is not given, so I added that to the test suite.

Is this a bug? It doesn't seem like resource pool should be required for inspect.

zjs · 2018-03-20T21:53:11Z

lib/apiservers/service/restapi/handlers/common.go

+			hostIP = executor.GetTLSFriendlyHostIP(clientIP, cert, vchConfig.CertificateAuthorities)
 		}
+
+		var dockerPort string


Why not just define dockerPort as an integer? That should avoid the need to fmt.Sprintf("%d", below.

I was trying to mimic CLI implementation:

if !conf.HostCertificate.IsNil() { d.DockerPort = fmt.Sprintf("%d", opts.DefaultTLSHTTPPort) } else { d.DockerPort = fmt.Sprintf("%d", opts.DefaultHTTPPort) }

CLI is setting the d.DockerPort field of type string in executor, but in API we're not using the docker IP/port from executor fields.
Fixing it now.

AngieCris · 2018-03-21T16:48:54Z

@zjs mistyped sorry...it's --compute-resource option for vic-machine inspect.
Yes it's required for all vic-machine operations under VC.
We didn't need it on CI because we ran on ESX earlier.

zjs · 2018-03-21T17:11:25Z

Our documentation doesn't suggest that --compute-resource is required for VC. Is this requirement a result of the way we've configured our CI systems, or could this be a change in behavior?

AngieCris · 2018-03-21T18:06:50Z

@zjs From the documentation:

You specify the --compute-resource option in the following circumstances:
A vCenter Server instance includes multiple instances of standalone hosts or clusters, or a mixture of standalone hosts and clusters.
You want to deploy the VCH to a specific resource pool in your environment.

Also here:

If you specify the id option in vic-machine commands, you do not need to specify the compute-resource option.

When I was testing against my nimbus VC, I have to provide --compute-resource because I have multiple clusters on my VC. I'm not sure about whether our CI infra contains multiple pools/clusters/mixture of hosts and clusters. If CI only has one cluster/pool, --compute-resource parameter is probably not required. @mhagen-vmware
I guess it's the best for us to include --compute-resource anyway, so that the tests would work under different configurations.

…ortal address (vmware#7533) 1. Update VCH inspect API to output TLS friendly docker host and admin portal address 2. Include --compute-resource for vic-machine commands in robot tests

AngieCris added 5 commits March 16, 2018 11:22

choose tls friendly address to API inspect output

d656af7

Merge branch 'master' of github.com:vmware/vic into vic/7321

1963484

add test-resource to inspect robot test

1481a5a

go fmt

ddb0701

fixup! add test-resource to inspect robot test

30fa47b

AngieCris self-assigned this Mar 16, 2018

AngieCris requested review from hickeng, mhagen-vmware and zjs March 16, 2018 20:48

vmwclabot added the cla-not-required label Mar 16, 2018

zjs reviewed Mar 16, 2018

View reviewed changes

fixup! choose tls friendly address to API inspect output

f29d03c

mhagen-vmware approved these changes Mar 19, 2018

View reviewed changes

Merge branch 'master' of github.com:vmware/vic into vic/7321

5dff3ff

zjs reviewed Mar 20, 2018

View reviewed changes

fixup! choose tls friendly address to API inspect output

850c094

AngieCris added 2 commits March 21, 2018 11:28

Merge branch 'master' of github.com:vmware/vic into vic/7321

e929efb

Merge branch 'master' of github.com:vmware/vic into vic/7321

042d178

zjs approved these changes Mar 22, 2018

View reviewed changes

Merge branch 'master' of github.com:vmware/vic into vic/7321

37ff7fe

AngieCris merged commit 5368c6d into vmware:master Mar 23, 2018

AngieCris deleted the vic/7321 branch March 23, 2018 18:23

Conversation

AngieCris commented Mar 16, 2018

Uh oh!

codecov-io commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zjs Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

AngieCris Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

zjs Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

zjs Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

AngieCris Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

mhagen-vmware left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AngieCris commented Mar 19, 2018

Uh oh!

zjs left a comment

Choose a reason for hiding this comment

Uh oh!

zjs Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

AngieCris Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AngieCris commented Mar 21, 2018

Uh oh!

zjs commented Mar 21, 2018

Uh oh!

AngieCris commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Mar 16, 2018 •

edited

Loading

mhagen-vmware left a comment •

edited

Loading

AngieCris Mar 21, 2018 •

edited

Loading

AngieCris commented Mar 21, 2018 •

edited

Loading