Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[specific ci=Group23-VIC-Machine-Service] container-name-convention support added to vic-machine API #6634

Conversation

AngieCris
Copy link
Contributor

@AngieCris AngieCris commented Oct 25, 2017

Fixes #6503

container-name-convention support is now added to vic-machine create API and inspect API.

Details

  1. vic-machine create API
    POST request body includes a container object that specifies the configurations for containers on the VCH such as network, CPU, memory (not implemented yet). Within container field, name_convention field is a string that specifies a naming convention for vSphere display names for containers.
    name_convention must contain either {id} or {name} as token (including both tokens in naming convention is currently not supported).
    For example, if the naming convention is container-{id} and there's a container running on the VCH, the vSphere display name for this container would be container-short_id where short_id is the short ID of that container.
    Note that this only changes the display names of containers on vSphere, not actual Docker container names.

An example of vic-machine create API request body:

{
	"name": "vch-test-1",
	"storage":{
		"image_stores":["ds://datastore1"]
	},
	"network":{
		"bridge":{
			"ip_range":"172.16.0.0/12",
			"port_group":{"name":"bridge"}
		},
		"public":{
			"port_group":{"name":"vm-network"}
		},
	},
	"auth":{
		"no_tls":true
	},
	"container": {
                "name_convention":"container-{id}"
        }
}
  1. vic-machine inspect API
    The response body of POST request now includes a field name_convention field under container of type string that specifies the vSphere display naming convention for containers running on the VCH.

Example vic-machine inspect GET response body:

{
  "auth": {
    "client": {
      "certificate_authorities": []
    }
  },
  "compute": {
    "cpu": {
      "shares": {
        "level": "normal"
      }
    },
    "memory": {
      "shares": {
        "level": "normal"
      }
    },
    "resource": {
      "id": "resgroup-v35"
    }
  },
  "container":{
      "name_convention": "container-{id}",
  },
  "endpoint": {
    "cpu": {
      "sockets": 1
    },
    "memory": {
      "units": "MiB",
      "value": 2048
    },
    "operations_credentials": {}
  },
  "name": "vch-test-1",
  "network": {
    "bridge": {
      "ip_range": "172.16.0.0/12",
      "port_group": {
        "id": "dvportgroup-23"
      }
    },
    "client": {
      "nameservers": [],
      "port_group": {
        "id": "dvportgroup-22"
      }
    },
    "container": [],
    "management": {
      "nameservers": [],
      "port_group": {
        "id": "dvportgroup-21"
      }
    },
    "public": {
      "nameservers": [],
      "port_group": {
        "id": "dvportgroup-22"
      }
    }
  },
  "registry": {
    "blacklist": null,
    "certificate_authorities": [],
    "insecure": [],
    "whitelist": []
  },
  "runtime": {
    "admin_portal": "https://10.160.213.153:2378",
    "docker_host": "10.160.213.153:2375",
    "power_state": "poweredOn",
    "upgrade_status": "Up to date"
  },
  "storage": {
    "base_image_size": {
      "units": "KiB",
      "value": 8000000
    },
    "image_stores": [
      "ds://datastore1/vch-test-1"
    ],
    "volume_stores": []
  },
  "version": "v1.2.0-rc1-0-a3b3132"
}

Remaining work
In integration test, verify container naming convention with docker

@@ -266,6 +266,10 @@ func vchToModel(op trace.Operation, vch *vm.VirtualMachine, d *data.Data, execut
model.SyslogAddr = strfmt.URI(syslogConfig.Network + "://" + syslogConfig.RAddr)
}

if containerNameConvention := vchConfig.ContainerNameConvention; containerNameConvention != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we simply do this here?

if vchConfig.ContainerNameConvention != "" {
    model.ContainerNameConvention = vchConfig.ContainerNameConvention
}

@@ -803,6 +803,10 @@
"type": "string",
"format": "uri",
"pattern": "^(tc|ud)p:\/\/.*"
},
"container_name_convention": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the comments in vic-machine/common/container.go, it seems like we expect more container-related properties to be added in the future:

		// other container flags to to added"
		// default container memory
		// default container cpu
		// default container network

Given that, I think this should probably be structured along the lines of the following to give us more flexibility to eventually represent those properties as a part of this JSON structure:

"container": {
    "type": "object",
    "properties": {
        "name_convention": {
            "type": "string",
            "pattern": "^.*(\\{id\\}|\\{name\\}).*"
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, thanks

Copy link
Member

@zjs zjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mdharamadas1
Copy link
Contributor

mdharamadas1 commented Oct 27, 2017

@AngieCris #6527 is closed which this PR is referring to for adding integration tests. Does that mean we need to add tests as part of this then for scenarios covered here?

@AngieCris
Copy link
Contributor Author

@mdharamadas1 adding tests for this functionality now, will let you know when it's done

@zjs zjs force-pushed the feature/vic-machine-service branch 2 times, most recently from 2faf0a5 to c98e4aa Compare October 27, 2017 19:55
@zjs zjs force-pushed the feature/vic-machine-service branch 2 times, most recently from e2957ac to 66411cf Compare October 31, 2017 23:19
@AngieCris AngieCris force-pushed the container-name-convention-API branch from 1c89e96 to bf5b2f7 Compare November 1, 2017 16:54
@AngieCris AngieCris closed this Nov 1, 2017
@AngieCris AngieCris force-pushed the container-name-convention-API branch from bf5b2f7 to 66411cf Compare November 1, 2017 17:12
@AngieCris AngieCris reopened this Nov 1, 2017
Copy link
Member

@zjs zjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@AngieCris
Copy link
Contributor Author

AngieCris commented Nov 6, 2017

Currently the integration test only verifies of inspect output contains the container naming convention supplied when creating the specific VCH.
Still pending a more complicated integration test that involves checking vSphere display name of container with docker, and docker regression test.
Using docker in VIC machine API integration tests will be captured later in separate issues.

@zjs zjs force-pushed the feature/vic-machine-service branch 4 times, most recently from dfd6a3a to eb07b69 Compare November 6, 2017 21:23
@AngieCris AngieCris force-pushed the container-name-convention-API branch 2 times, most recently from 6d0e535 to 9576e8e Compare November 6, 2017 21:58
@zjs zjs force-pushed the feature/vic-machine-service branch 2 times, most recently from 2d7026a to 99035e4 Compare November 7, 2017 01:23
@AngieCris AngieCris force-pushed the container-name-convention-API branch from 009a4ac to bb141ee Compare November 7, 2017 18:33
Copy link
Contributor

@mdharamadas1 mdharamadas1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@AngieCris AngieCris merged commit 8aec817 into vmware:feature/vic-machine-service Nov 8, 2017
zjs pushed a commit that referenced this pull request Nov 15, 2017
zjs pushed a commit that referenced this pull request Nov 16, 2017
zjs pushed a commit that referenced this pull request Nov 20, 2017
AngieCris added a commit to AngieCris/vic that referenced this pull request Nov 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants