govc: add proper VAPI for vSphere Supervisor Services #3674

fstrudel · 2025-01-10T17:57:27Z

Description

Update GET for both /supervisor-services and /supervisor-services/{id}/versions
Add GET for specific service and specific version: /supervisor-services/{id} and /supervisor-services/{id}/versions/{version}
Add DELETE for /supervisor-services/{id} and /supervisor-services/{id}/versions/{version}: DELETE for /supervisor-services/{id} and /supervisor-services/{id}/versions/{version}
Add activate/deactivate for a service version too (you can't delete a version of a service if it's not deactivated first):
PATCH for superrvisor-services/{id} and /supervisor-services/{id}/versions/{version}
Introduced a "flag" package to get common CLI checks between version and service APIs
Update namespace.bats tests to test list and get (for now)
Update VAPI simulator to use ServeHTTP to make sure the path values are set properly
Also update copyrights

How Has This Been Tested?

Ran make check and ./govc/test/namespace.bats
make test gave me some errors locally even on main so I'm curious to know if CI is ok.
Example:

$   ./govc/test/vcsim.bats 
vcsim.bats
 - vcsim rbvmomi (skipped: requires rbvmomi)
 - vcsim powercli (skipped: docker tests require linux)
 ✗ vcsim examples
   (from function `flunk' in file govc/test/test_helper.bash, line 211,
    from function `assert_success' in file govc/test/test_helper.bash, line 216,
    in test file govc/test/vcsim.bats, line 44)
     `assert_success' failed
   command failed with exit status 1: stat ../../examples/*/main.go: no such file or directory
 ✓ vcsim about
 ✓ truncated WaitForUpdatesEx
 ✓ vcsim host placement
 ✓ vcsim host config.port
 ✓ vcsim set vm properties
 ✓ vcsim vm.create
 ✓ vcsim issue #1251
 ✓ vcsim issue #3396
 - vcsim run container (skipped: docker tests require linux)
 ✓ vcsim listen
 ✓ vcsim vapi auth
 ✓ vcsim auth
 ✗ vcsim ovftool
   (from function `flunk' in file govc/test/test_helper.bash, line 211,
    from function `assert_success' in file govc/test/test_helper.bash, line 216,
    in test file govc/test/vcsim.bats, line 532)
     `assert_success' failed
   command failed with exit status 1: Opening OVA source: /Users/fstrudel/Work/govmomi/govc/test/images/ttylinux-pc_i486-16.1.ova
   The manifest validates
   Error: Internal error: Failed to connect to server
   Warning:
    - No manifest entry found for: 'ttylinux-pc_i486-16.1.ovf'.
    - File is missing from the manifest: 'ttylinux-pc_i486-16.1.ovf'.
   Completed with errors
 ✓ vcsim model load
 ✓ vcsim trace file

18 tests, 2 failures, 3 skipped

Also ran against a real VC where I added 1 dummy service with 2 versions: export GOVC_URL=...

$ cat sample-pkg.test.carvel.dev-1.0.0.yaml
apiVersion: data.packaging.carvel.dev/v1alpha1
kind: PackageMetadata
metadata:
  name: sample-pkg-testgovc.test.carvel.dev
spec:
  displayName: "sample-service for testing"
  shortDescription: "Sample core service description"
---
apiVersion: data.packaging.carvel.dev/v1alpha1
kind: Package
metadata:
  name: sample-pkg-testgovc.test.carvel.dev.1.0.0
spec:
  refName: sample-pkg-testgovc.test.carvel.dev
  version: 1.0.0
  releasedAt: 2021-05-05T18:57:06Z
  template:
    spec:
      fetch:
        - imgpkgBundle:
            image: wcp-docker-ci.artifactory.eng.vmware.com/carvel/simple-app-bundle:v0.0.0
      template:
        - ytt:
            paths:
              - config-step-2-template
              - config-step-2a-overlays
      deploy:
        - kapp: { }

$ govc namespace.service.create sample-pkg.test.carvel.dev-1.0.0.yaml

$  govc namespace.service.create sample-pkg.test.carvel.dev-1.0.0.yaml
govc: 400 Bad Request: {"messages":[{"args":["sample-pkg-testgovc.test.carvel.dev","Supervisor Service"],"default_message":"Failed to create Supervisor Service sample-pkg-testgovc.test.carvel.dev because an instance of Supervisor Service with the same identifier already exists.","localized":"Failed to create Supervisor Service sample-pkg-testgovc.test.carvel.dev because an instance of Supervisor Service with the same identifier already exists.","id":"vcenter.wcp.appplatform.supervisorservice.write.unique_violation"}]}

$ govc namespace.service.version.create  sample-pkg-testgovc.test.carvel.dev  sample-pkg.test.carvel.dev-2.0.0.yaml

$ govc namespace.service.version.deactivate sample-pkg-testgovc.test.carvel.dev 2.0.0

$ govc namespace.service.version.activate sample-pkg-testgovc.test.carvel.dev 2.0.0

$ govc namespace.service.version.rm sample-pkg-testgovc.test.carvel.dev 2.0.0
govc: 400 Bad Request: {"messages":[{"args":["sample-pkg-testgovc.test.carvel.dev","2.0.0"],"default_message":"Cannot delete the Supervisor Service (sample-pkg-testgovc.test.carvel.dev) version (2.0.0) because it is active.","localized":"Cannot delete the Supervisor Service (sample-pkg-testgovc.test.carvel.dev) version (2.0.0) because it is active.","id":"vcenter.wcp.appplatform.supervisorserviceversion.delete.activated"}]}

$ govc namespace.service.version.deactivate sample-pkg-testgovc.test.carvel.dev 2.0.0
$ govc namespace.service.version.rm  sample-pkg-testgovc.test.carvel.dev 2.0.0
$ govc namespace.service.ls sample-pkg-testgovc.test.carvel.dev 2.0.0

$ govc namespace.service.ls -json
[...]
  {
    "supervisor_service": "sample-pkg-testgovc.test.carvel.dev",
    "display_name": "sample-service for testing version",
    "state": "ACTIVATED"
  },
]

$ govc namespace.service.info -json sample-pkg-testgovc.test.carvel.dev
{
  "display_name": "sample-service for testing version",
  "state": "ACTIVATED",
  "description": "Sample core service description",
  "must_be_installed": false,
  "has_default_versions_registered": false
}

{
  "must_be_installed": false,
  "has_default_versions_registered": false,
  "description": "Sample core service description",
  "state": "ACTIVATED",
  "display_name": "sample-service for testing version"
}

Guidelines

Please read and follow the CONTRIBUTION guidelines of this project.

dougm

Thanks @fstrudel ! Couple of minor suggestions.

As for the bats tests, seems some assume they are being run within the govc/test directory and fail otherwise.

dougm · 2025-01-13T17:44:02Z

cli/namespace/service/create.go

+	if err := cmd.ServiceVersionFlag.Process(ctx); err != nil {
+		return err
+	}
+	return nil


Need to return cmd.ClientFlag.Process() here. When create didn't have a Process method directly attached, the embedded flags.ClientFlag's Process method would be called. While the govc command still works w/o calling ClientFlag.Process, there's some optional feature that won't work such as debug/trace, session persistence, etc.

See: https://github.com/vmware/govmomi/blob/main/cli/flags/client.go#L199

dougm · 2025-01-13T17:46:25Z

cli/namespace/service/version/create.go

+	if err := cmd.ServiceVersionFlag.Process(ctx); err != nil {
+		return err
+	}
+	return nil


Need to return cmd.ClientFlag.Process() here.

dougm · 2025-01-13T17:49:40Z

vapi/namespace/simulator/simulator.go

-
+// © Broadcom. All Rights Reserved.
+// The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
+// SPDX-License-Identifier: Apache-2.0
 package simulator


Just need an empty line here in between header and package to make Boilerplate Check happy.

dougm · 2025-01-13T17:53:18Z

vapi/namespace/simulator/simulator.go

 	},
 }

 func (h *Handler) listServices(w http.ResponseWriter, r *http.Request) {
 	switch r.Method {
 	case http.MethodGet:
+		supervisorServices := make([]namespace.SupervisorServiceSummary, 0, len(supervisorServicesMap))
+		for _, service := range supervisorServicesMap {
+			supervisorServices = append(supervisorServices, service)


nit: since you already allocated with make, this can be: supervisorServices[i] = service

dougm · 2025-01-14T20:09:41Z

@fstrudel can you also add "Fixes #3624" in the commit message?
And if you use govc: prefix, this will get included in that section of the generated release notes.
(See https://github.com/vmware/govmomi/blob/main/CONTRIBUTING.md#format-of-the-commit-message)

dougm

Thanks @fstrudel , just need to fix for the CI checks:

make doc
Amend the commit with -s for DCO

fstrudel · 2025-01-16T16:16:52Z

Thanks @fstrudel , just need to fix for the CI checks:

make doc

Amend the commit with -s for DCO

Yes, just realized there were checks I could not see (I needed to scroll down... :/). Will fix.

- Update GET for both /supervisor-services and /supervisor-services/{id}/versions - Add GET for specific service and specific version: /supervisor-services/{id} and /supervisor-services/{id}/versions/{version} - Add DELETE for /supervisor-services/{id} and /supervisor-services/{id}/versions/{version}: DELETE for /supervisor-services/{id} and /supervisor-services/{id}/versions/{version} - Add activate/deactivate for a service version too (you can't delete a version of a service if it's not deactivated first): PATCH for superrvisor-services/{id} and /supervisor-services/{id}/versions/{version} - Update namespace.bats tests to test list and get - Update VAPI simulator to use `ServeHTTP` to make sure the path vlaues are set properly - Also update copyrights Fixes vmware#3624 Testing Done: Ran `make install check doc` and `./govc/test/namespace.bats` Also ran against a real VC where I added 1 dummy service with 2 versions: `export GOVC_URL=...` ``` $ cat sample-pkg.test.carvel.dev-1.0.0.yaml apiVersion: data.packaging.carvel.dev/v1alpha1 kind: PackageMetadata metadata: name: sample-pkg-testgovc.test.carvel.dev spec: displayName: "sample-service for testing" shortDescription: "Sample core service description" --- apiVersion: data.packaging.carvel.dev/v1alpha1 kind: Package metadata: name: sample-pkg-testgovc.test.carvel.dev.1.0.0 spec: refName: sample-pkg-testgovc.test.carvel.dev version: 1.0.0 releasedAt: 2021-05-05T18:57:06Z template: spec: fetch: - imgpkgBundle: image: wcp-docker-ci.artifactory.eng.vmware.com/carvel/simple-app-bundle:v0.0.0 template: - ytt: paths: - config-step-2-template - config-step-2a-overlays deploy: - kapp: { } $ govc namespace.service.create sample-pkg.test.carvel.dev-1.0.0.yaml $ govc namespace.service.create sample-pkg.test.carvel.dev-1.0.0.yaml govc: 400 Bad Request: {"messages":[{"args":["sample-pkg-testgovc.test.carvel.dev","Supervisor Service"],"default_message":"Failed to create Supervisor Service sample-pkg-testgovc.test.carvel.dev because an instance of Supervisor Service with the same identifier already exists.","localized":"Failed to create Supervisor Service sample-pkg-testgovc.test.carvel.dev because an instance of Supervisor Service with the same identifier already exists.","id":"vcenter.wcp.appplatform.supervisorservice.write.unique_violation"}]} $ govc namespace.service.version.create sample-pkg-testgovc.test.carvel.dev sample-pkg.test.carvel.dev-2.0.0.yaml $ govc namespace.service.version.deactivate sample-pkg-testgovc.test.carvel.dev 2.0.0 $ govc namespace.service.version.activate sample-pkg-testgovc.test.carvel.dev 2.0.0 $ govc namespace.service.version.rm sample-pkg-testgovc.test.carvel.dev 2.0.0 govc: 400 Bad Request: {"messages":[{"args":["sample-pkg-testgovc.test.carvel.dev","2.0.0"],"default_message":"Cannot delete the Supervisor Service (sample-pkg-testgovc.test.carvel.dev) version (2.0.0) because it is active.","localized":"Cannot delete the Supervisor Service (sample-pkg-testgovc.test.carvel.dev) version (2.0.0) because it is active.","id":"vcenter.wcp.appplatform.supervisorserviceversion.delete.activated"}]} $ govc namespace.service.version.deactivate sample-pkg-testgovc.test.carvel.dev 2.0.0 $ govc namespace.service.version.rm sample-pkg-testgovc.test.carvel.dev 2.0.0 $ govc namespace.service.ls sample-pkg-testgovc.test.carvel.dev 2.0.0 $ govc namespace.service.ls -json [...] { "supervisor_service": "sample-pkg-testgovc.test.carvel.dev", "display_name": "sample-service for testing version", "state": "ACTIVATED" }, ] $ govc namespace.service.info -json sample-pkg-testgovc.test.carvel.dev { "display_name": "sample-service for testing version", "state": "ACTIVATED", "description": "Sample core service description", "must_be_installed": false, "has_default_versions_registered": false } { "must_be_installed": false, "has_default_versions_registered": false, "description": "Sample core service description", "state": "ACTIVATED", "display_name": "sample-service for testing version" } ``` Signed-off-by: Fanny Strudel <fanny.strudel@broadcom.com>

dougm

Thanks @fstrudel , also added you to the team, so you should be able to merge now with the approval.

dougm reviewed Jan 13, 2025

View reviewed changes

fstrudel force-pushed the update-supervisor-services branch 2 times, most recently from 9b35328 to 8b9536b Compare January 16, 2025 00:50

dougm reviewed Jan 16, 2025

View reviewed changes

fstrudel force-pushed the update-supervisor-services branch from 8b9536b to 81828ed Compare January 16, 2025 16:28

fstrudel changed the title ~~Services: add proper VAPI for vSphere Supervisor Services~~ govc: add proper VAPI for vSphere Supervisor Services Jan 16, 2025

dougm approved these changes Jan 16, 2025

View reviewed changes

fstrudel merged commit 08f85db into vmware:main Jan 17, 2025
13 checks passed

fstrudel deleted the update-supervisor-services branch January 17, 2025 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

govc: add proper VAPI for vSphere Supervisor Services #3674

govc: add proper VAPI for vSphere Supervisor Services #3674

fstrudel commented Jan 10, 2025 •

edited

Loading

dougm left a comment

dougm Jan 13, 2025

dougm Jan 13, 2025

dougm Jan 13, 2025

dougm Jan 13, 2025

fstrudel Jan 16, 2025

dougm commented Jan 14, 2025

dougm left a comment

fstrudel commented Jan 16, 2025

dougm left a comment

govc: add proper VAPI for vSphere Supervisor Services #3674

govc: add proper VAPI for vSphere Supervisor Services #3674

Conversation

fstrudel commented Jan 10, 2025 • edited Loading

Description

How Has This Been Tested?

Guidelines

dougm left a comment

Choose a reason for hiding this comment

dougm Jan 13, 2025

Choose a reason for hiding this comment

dougm Jan 13, 2025

Choose a reason for hiding this comment

dougm Jan 13, 2025

Choose a reason for hiding this comment

dougm Jan 13, 2025

Choose a reason for hiding this comment

fstrudel Jan 16, 2025

Choose a reason for hiding this comment

dougm commented Jan 14, 2025

dougm left a comment

Choose a reason for hiding this comment

fstrudel commented Jan 16, 2025

dougm left a comment

Choose a reason for hiding this comment

fstrudel commented Jan 10, 2025 •

edited

Loading