Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Make sure _DEFAULT created after remove is created with constant UUID #988

Merged
merged 2 commits into from
Mar 3, 2017

Conversation

lipingxue
Copy link
Contributor

This PR includes the following
Make sure _DEFAULT created after remove is created with constant UUID(issue 986).

Fix the issue that volume create fails on a datastore whose name includes spaces (issue 916).

Please review it.

Copy link
Contributor

@msterin msterin left a comment

Choose a reason for hiding this comment

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

see inline

description=description,
vms=vms,
privileges=privileges)
if name == auth.DEFAULT_TENANT:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually prefer

if name == auth.DEFAULT_TENANT:
   tenant_uuid = auth.DEFAULT_TENANT_UUID
else:
   tenant_uuid = None

error_msg, tenant = auth_mgr.create_tenant(name=name, ..., tenant_uuid=tenant_uuid)

@@ -683,7 +683,7 @@ def get_vol_path(datastore, tenant_name=None):

if tenant_name and not os.path.isdir(path):
# The mkdir command is used to create "tenant_name" folder inside DOCK_VOLS_DIR on "datastore"
cmd = "{0} {1}".format(MKDIR_CMD, path)
cmd = "{0} '{1}'".format(MKDIR_CMD, path)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch!

description=description,
vms=vms,
privileges=privileges)
if name == auth.DEFAULT_TENANT:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to repeat my previous comment - I do think we should handle the tenant uuid inside the create_tenant() API. IMO, uuid should not be handled by the API users. It's the API's responsibility to take care of the appropriate uuids.

If you recall, exposing uuid to the API users has caused multiple unnecessary changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

so would you suggest "create_tenant" and "create_default_tenant" APIs, or just put

if name == DEFAULT_TENANT_NAME
  uuid = DEFAULT_UUID
else: 
  uuid = generate_uuid

into the create_tenant ? (I guess the latter, right ?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. I'm suggesting the latter.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

… (issue 986).

Fix the issue that volume create fail on a datastore whose name includes spaces (issue 916)..
@lipingxue lipingxue force-pushed the create_default_tenant_with_constant_uuid.liping branch from 0d9ca81 to e1faafc Compare March 3, 2017 17:23
@lipingxue lipingxue force-pushed the create_default_tenant_with_constant_uuid.liping branch from e1faafc to 5276de2 Compare March 3, 2017 18:13
@lipingxue
Copy link
Contributor Author

@msterin @shaominchen I have addressed your comments.

Copy link
Contributor

@shaominchen shaominchen left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants