Merge branch 'feature/vic-machine-service' [full ci]#6665
Conversation
zjs
force-pushed
the
feature/vic-machine-service
branch
from
e2957ac to
66411cf
Compare
mdubya66
left a comment
mdubya66
left a comment
There was a problem hiding this comment.
All licenses are ok Approved for vendor changes.
zjs
force-pushed
the
feature/vic-machine-service
branch
6 times, most recently
from
2d7026a to
99035e4
Compare
|
|
||
| // assume if a vsphere session key exists, we want to use that instead of user/pass | ||
| if t.CloneTicket != "" { | ||
| t.URL.User = nil // necessary? |
| if err != nil { | ||
| return nil, fmt.Errorf("Validation Error: %s", err) | ||
| } | ||
| // If dc is not set, and multiple datacenter is available, vic-machine ls will list VCHs under all datacenters. |
There was a problem hiding this comment.
s/datacenter is/datacenters are
| // If dc is not set, and multiple datacenter is available, vic-machine ls will list VCHs under all datacenters. | ||
| validator.AllowEmptyDC() | ||
|
|
||
| _, err = validator.ValidateTarget(ctx, d) |
There was a problem hiding this comment.
This error block can be collapsed.
| if err != nil { | ||
| return nil, fmt.Errorf("Target validation failed: %s", err) | ||
| } | ||
| _, err = validator.ValidateCompute(ctx, d, false) |
There was a problem hiding this comment.
This error block can be collapsed.
| return nil, util.NewError(http.StatusNotFound, fmt.Sprintf("Unable to find VCH %s: %s", d.ID, err)) | ||
| } | ||
|
|
||
| err = validate.SetDataFromVM(validator.Context, validator.Session.Finder, vch, d) |
There was a problem hiding this comment.
This error block can be collapsed.
There was a problem hiding this comment.
Declining to fix (same explanation as #6694 (comment))
|
|
||
| // getDatastoreHelper validates the VCH and returns the datastore helper for the VCH. It errors when validation fails or when datastore is not ready | ||
| func getDatastoreHelper(op trace.Operation, d *data.Data) (*datastore.Helper, error) { | ||
| // TODO (angiew): abstract some of the boilerplate into helpers in common.go |
There was a problem hiding this comment.
👍 for TODO (username)
There was a problem hiding this comment.
Issue filed to track TODO, reference added in #6720
lib/install/management/create.go
Outdated
| // if not found, do nothing | ||
| case object.DatastoreNoSuchFileError: | ||
| // otherwise force delete | ||
| // otherwise force delete |
There was a problem hiding this comment.
This comment seems out of place - it'd fit better inside the default block since that's what the comment is for.
lib/install/management/create.go
Outdated
| @@ -125,7 +137,7 @@ func (d *Dispatcher) uploadImages(files map[string]string) error { | |||
| switch err.(type) { | |||
| // if not found, do nothing | |||
There was a problem hiding this comment.
This comment seems out of place - it'd fit better inside the case object.DatastoreNoSuchFileError: block since that's what the comment is for.
| } | ||
|
|
||
| func (v *Build) ShortVersion() string { | ||
| if v == nil { |
There was a problem hiding this comment.
Don't think this is needed - if v were indeed nil we'd get a SIGSEGV in the caller.
There was a problem hiding this comment.
Instead, we can say:
if v.Version == "" && v.BuildNumber == "" && v.GitCommit == "" {
return "unknown"
}There was a problem hiding this comment.
If v is nil, we get "unknown".
There was a problem hiding this comment.
Interesting - TIL! I found this thread with some discussion: https://groups.google.com/forum/#!topic/golang-nuts/wcrZ3P1zeAk.
cc @matthewavery since we were discussing this in person earlier.
| } | ||
|
|
||
| soapClient := soap.NewClient(soapURL, s.Insecure) | ||
|
|
There was a problem hiding this comment.
Minor: unneeded whitespace.
There was a problem hiding this comment.
I'm going to ignore this to avoid further churn in this file.
This commit makes the 'Remove a container and its anonymous volume' test case more robust by force-removing the container that runs the /bin/ls command, since it's possible for the container to not have powered off fully and the container state to not have updated to 'Stopped' before the remove command is issued in the test. Before this change, it was possible for the test to fail if the container state wasn't updated on time. This was seen in CI build 14551 for PR vmware#6665. Since that test snippet's intent (verify that the anonymous volume can be used by a new container) is unaffected by how the container is removed, this change is valid. Enhances vmware#6262
hickeng
left a comment
hickeng
left a comment
There was a problem hiding this comment.
Some items that must be changed. Other comments are informational or left to your discretion - ping me directly for exposition on specific items.
Makefile
Outdated
|
|
||
| # main appliance target - depends on all top level component targets | ||
| $(appliance): isos/appliance.sh isos/appliance/* isos/vicadmin/** $(vicadmin) $(vic-init) $(portlayerapi) $(docker-engine-api) $(appliance-staging) | ||
| $(appliance): isos/appliance.sh isos/appliance/* isos/vicadmin/** $(vicadmin) $(vic-init) $(portlayerapi) $(serviceapi-server) $(docker-engine-api) $(appliance-staging) |
There was a problem hiding this comment.
I don't think this is the appliance you think it is - this is the VCH endpoint ISO target and definitely doesn't have a dependency on the serviceapi.
| @rm -f ./lib/apiservers/service/restapi/doc.go | ||
| @rm -f ./lib/apiservers/service/restapi/embedded_spec.go | ||
| @rm -f ./lib/apiservers/service/restapi/server.go | ||
| @rm -rf ./lib/apiservers/service/restapi/operations/ |
There was a problem hiding this comment.
Should you have ./lib/apiservers/service/client and ./lib/apiservers/service/models in the list of directories to be removed?
|
|
||
| // assume if a vsphere session key exists, we want to use that instead of user/pass | ||
| if t.CloneTicket != "" { | ||
| t.URL.User = nil // necessary? |
There was a problem hiding this comment.
Good question. Hoping to get an answer here: #6665 (comment)
| User string | ||
| Password *string | ||
| CloneTicket string | ||
| Thumbprint string `cmd:"thumbprint"` |
There was a problem hiding this comment.
I'm unclear why this is being added to the vic-machine command without an option by which to set it.
There was a problem hiding this comment.
@jzt may be able to provide more context, but my assumption is that this is essentially due to the poorly defined separation of responsibilities we have between the CLI code and the API and undue coupling between the CLI code and the code it calls.
I'd propose we attempt to clean this up as a part of #6032.
| uploadMaxElapsedTime = 30 * time.Minute | ||
| uploadMaxInterval = 1 * time.Minute | ||
| uploadInitialInterval = 10 * time.Second | ||
| timeFormat = "2006-01-02T15:04:05-0700" |
There was a problem hiding this comment.
Should be in lib/constants for consistent reference across components.
| } | ||
| model.Runtime.PowerState = string(powerState) | ||
|
|
||
| if public := vchConfig.ExecutorConfig.Networks["public"]; public != nil { |
There was a problem hiding this comment.
This is not correct.
See https://github.com/vmware/vic/blob/master/cmd/docker/main.go#L224 for the port choice logic.
The API is always served on the client network role (unless something is very wrong with current vic-machine) which MAY share an interface with the public network role.
| return operations.NewGetTargetTargetVchVchIDLogDefault(util.StatusCode(err)).WithPayload(err.Error()) | ||
| } | ||
|
|
||
| return operations.NewGetTargetTargetVchVchIDLogOK().WithPayload(output) |
There was a problem hiding this comment.
I take it this is NOT expected to stream the log? Is it expected to block or truncate at whatever is the end-of-file when we're reading it?
There was a problem hiding this comment.
Correct. The proposal is to eventually use a more appropriate protocol (WebSockets?) to allow for streaming of logs. (See #6702 for more information.)
| return nil, util.NewError(http.StatusNotFound, fmt.Sprintf("Unable to find VCH %s: %s", d.ID, err)) | ||
| } | ||
|
|
||
| if err := validate.SetDataFromVM(validator.Context, validator.Session.Finder, vch, d); err != nil { |
There was a problem hiding this comment.
There is a vch.FolderName method that you could call that tells you the directory name of the VM.
It looks like that has been recently changed to only provide the Base name - I am unsure why and @caglar10ur has just left. However this is the call used in lib/install/management/appliance.go:523.
The ImageStores path MAY be the same, but should not be in the case of vSAN - a VM in vSAN MUST have a dedicated namespace and I believe we place the image VMDKS into their own namespace.
|
|
||
| // getAllLogFilePaths returns a list of all log file paths under datastore folder, errors out when no log file found | ||
| func getAllLogFilePaths(op trace.Operation, helper *datastore.Helper) ([]string, error) { | ||
| res, err := helper.Ls(op, "") |
There was a problem hiding this comment.
If you look at the implementation of helper.Ls it would be trivial to change it so that you can actually provide a pattern instead of a static name. There is already a MatchPattern defined as part of the search spec.
That would basically eliminate the need for this function.
| sort.Strings(paths) | ||
|
|
||
| for _, p := range paths { | ||
| reader, err := helper.Download(op, p) |
There was a problem hiding this comment.
This will not be able to access the file if it is open via a different process (for example still being uploaded) if not accessed via the same host. I do not know if this is an issue or not.
If it is then a specific host can be chosen for the operation by setting a target host in the context: c.vm.Datastore.HostContext(op, h)
See
vic/lib/portlayer/exec/container.go
Line 525 in 0050c58
|
I've not run through the test code but have done a first pass on the rest of it (high level correctness and flow rather than low level detail). |
This commit makes the 'Remove a container and its anonymous volume' test case more robust by force-removing the container that runs the /bin/ls command, since it's possible for the container to not have powered off fully and the container state to not have updated to 'Stopped' before the remove command is issued in the test. Before this change, it was possible for the test to fail if the container state wasn't updated on time. This was seen in CI build 14551 for PR vmware#6665. Since that test snippet's intent (verify that the anonymous volume can be used by a new container) is unaffected by how the container is removed, this change is valid. Enhances vmware#6262
This commit makes the 'Remove a container and its anonymous volume' test case more robust by force-removing the container that runs the /bin/ls command, since it's possible for the container to not have powered off fully and the container state to not have updated to 'Stopped' before the remove command is issued in the test. Before this change, it was possible for the test to fail if the container state wasn't updated on time. This was seen in CI build 14551 for PR #6665. Since that test snippet's intent (verify that the anonymous volume can be used by a new container) is unaffected by how the container is removed, this change is valid. Enhances #6262
Include issue numbers with all TODOs added as a part of the initial VCH creation API project, as suggested during review of the merge commit for the VCH creation API feature branch.
Clarify an unclear comment identified during review of the merge commit for the VCH creation API feature branch.
Remove the serviceapi target as a dependency for the appliance.iso and ensure all generated code is cleaned up.
Introduce a pair of handlers for deleting VCHs within a vSphere target or datacenter. By default, deletion includes the VCH and powered off containers. Deletion may optionally include powered on containers and/or volume stores as well. If any containers remain, the VCH is not deleted. If the VCH is not deleted, the response includes a non-2xx status code. Define a suite of end-to-end tests which verify the intended deletion behavior. End-to-end tests do not attempt to verify the behavior of concurrent operations.
Registry blacklist functionality was designed, but has not been fully implemented in the engine. Remove references to it from the API.
When creating a VCH via the API, we should not write key/certificate files to the API server's disk. This change introduces the behavior as a flag so that it could be exposed as an option for the CLI at some point in the future, but does not expose it at this time.
The vic-machine CLI has differing requirements for gateway routing information, depending on the type of network. According to the CLI help: - a client gateway must specify one or more routing destinations - a public gateway must not specify any routing destinations - a management gateway must specify one or more routing destinations - a container gateway must specify exactly one routing destination This does not seem to be enforced in code, and may simply be more of a suggestion about how these gateways should be used than a requirement. Update the parsing for client, public, and management to support all zero or more routing destinations in all cases; defer to the existing ProcessNetwork code to ensure consistent validation behavior now and in the future. Additionally update the parsing for container to provide a clear error message if the expected routing destination is not supplied.
The vic-machine CLI requires that static addresses are specified as a CIDR, which allows the static address and subnet mask to be supplied in a compact way on the command line. This pattern does not allow for static addresses to be expressed in terms of a hostname. Update the API to match this convention. We may wish to allow for more flexibility in the API in the future, but there's value in at least starting with consistent behavior.
The vic-machine CLI allows IP ranges to be expressed in CIDR notation or as simple ranges in some places, but requires that CIDR notation be used in others. Initially there was a hope of making the API behave more consistently, but that requires changes to underlying logic (some of which is not well covered by existing tests). For now, be consistent with the CLI.
The vic-machine CLI supports specifying a "trust level" for each container network using the --container-network-firewall option. Support the same functionality in the API. Additionally, fix a small bug with the way ip ranges are returned by the inspect API.
Use the client network address, not the public network address, when displaying the admin portal URI and the docker host information as a part of API responses. Additionally, eliminate code duplication.
Additionally, move the binary to a subdirectory of opt to allow the ISO files to be packaged with it. See also: * http://www.tldp.org/LDP/Linux-Filesystem-Hierarchy/html/opt.html
Log each request and response to a configurable destination. Use trace.Operation to associate the request and response information with the logs for the handler to allow for easy debugging, even when there are several requests being handled concurrently. Adjust logging in server's main.go to avoid printing to stdout. Relies on external configuration for log rotation.
zjs
force-pushed
the
feature/vic-machine-service
branch
5 times, most recently
from
50d22de to
e47dfcb
Compare
|
(note to self) Interesting CI runs:
|
zjs
force-pushed
the
feature/vic-machine-service
branch
from
e47dfcb to
ad673c0
Compare
For each conceptual vic-machine operation, create a trace.Operation with an operation-local logger. Emit log messages using the log methods on the operation instead of logrus global methods. This will write to the vic-machine.log using the global trace.Logger and to the user using the operation-local logger. This ensures that both the CLI and API can print log messages to both a local file and the user (via a TTY, if the CLI, and the datastore). * In the CLI, use the operation-local logger to print to the console (using the default Logrus TTY formatting) and write to both the datastore and vic-machine.log file with the global trace.Logger (using VIC's custom log formatter). * In the API, use the the operation-local logger to write to the datastore and write to the server's console with the global trace.Logger. In both cases, use VIC's custom log formatter. Update code in packages used by vic-machine to accept a context, coerce/convert it into a trace.Operation, and use its log methods. Improve readability of logs for concurrent operations by including a operation/context in trace.Begin calls when one is available. Retain configuration for the global logrus logger in vic-machine so that we "fail ugly" (i.e., have lines with mismatched log formats) on the CLI if something uses the old pattern, but don't lose information. Update tests which parse log messages to understand this new format instead of or, where necessary, in addition to old format(s).
zjs
force-pushed
the
feature/vic-machine-service
branch
3 times, most recently
from
110545c to
dc30dd0
Compare
Define a new VCH Management API and implement the subset of that API which is necessary for building a VCH Creation Wizard as a part of the H5 Client Plugin.
This commit makes the 'Remove a container and its anonymous volume' test case more robust by force-removing the container that runs the /bin/ls command, since it's possible for the container to not have powered off fully and the container state to not have updated to 'Stopped' before the remove command is issued in the test. Before this change, it was possible for the test to fail if the container state wasn't updated on time. This was seen in CI build 14551 for PR vmware#6665. Since that test snippet's intent (verify that the anonymous volume can be used by a new container) is unaffected by how the container is removed, this change is valid. Enhances vmware#6262
9 participants
Define a new VCH Management API and implement the subset of that API which is necessary for building a VCH Creation Wizard as a part of the H5 Client Plugin.
Fixes #5721
At this stage, review should focus on understanding this large change and identifying work which may need to be completed before the merge. Feedback raised as a part of this review will be separated out into new GitHub issues for tracking.
Instructions:
make serviceapi.bin/vic-machine-server --help.cmd/vic-machine-server/Dockerfile.installer/BUILD.mdin thevic-productrepository.Known remaining work:
Add container-name-convention support to VCH creation API #6503: Add container-name-convention support to VCH creation APIvic-machine-server should generate request logs #6571: vic-machine-server should generate request logsVCH Creation API: Add support for container network trust level #6740: VCH Creation API: Add support for container network trust levelImplement API handler for VCH deletion [specific ci=Group23-VIC-Machine-Service] #6694: Implement API handler for deletion of a VCHKnown testing gaps:
Implement unit tests for the API handler for creating a VCH #6018: Implement unit tests for the API handler for creating a VCHClean up VCH Management API robot tests [specific ci=Group23-VIC-Machine-Service] #6666: Clean up VCH Management API robot testsKnown bugs:
Adjust VCH creation API to default to resource pool (not vApp) #6526: Adjust VCH creation API to default to resource poolEnsure VCH creation log streaming handles concurrent creation operations #6612: Ensure VCH creation log streaming handles concurrent creation operations[specific ci=Group23-VIC-Machine-Service] Format VCH creation log API response to text/plain #6640: Format VCH creation log API response to text/plainVCH creation API leaves certificate files on disk #6650: VCH creation API leaves certificate files on diskRevert change to PUBLIC_NETWORK variable [full ci] #6669: Revert change to PUBLIC_NETWORK variableVCH Creation API: Decide what to do if RoutingDestinations is empty #6715: VCH Creation API: Decide what to do if RoutingDestinations is emptyVCH Creation API: handle registry blacklist #6713: VCH Creation API: handle registry blacklistVCH Management APIs: Use the correct IP when determining docker host and admin portal addresses #6728: VCH Management APIs: Use the correct IP when determining docker host and admin portal addressesHandle IP ranges in the API like in the CLI [specific ci=Group23-VIC-Machine-Service] #6747: Handle IP ranges in the API like in the CLIvic-machine-server image does not include ISOs #6761: vic-machine-server image does not include ISOsKnown debt:
Minor adjustments to formatting #6719: Minor adjustments to formattingInclude issue numbers with TODOs #6720: Include issue numbers with TODOsClarify comment in common code for API handlers #6721: Clarify comment in common code for API handlersCleanup build process for vic-machine-server #6723: Cleanup build process for vic-machine-serverBelieve to be out-of-scope for 1.3: