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

DB upgrade fix for changes to handle default_datastore. #1179

Merged
merged 5 commits into from
Apr 25, 2017

Conversation

lipingxue
Copy link
Contributor

@lipingxue lipingxue commented Apr 19, 2017

PR #1155 introduced the change to handle default_datastore. With that change, it requires:

  1. each tenant in tenants table must have "default_datastore" field set
  2. For each tenant in tenants table, it must has a privilege to "default_datastore"
  3. For "_DEFAULT" tenant, remove privilege to "_DEFAULT_DS" and insert privilege to "__VM_DS" and "__ALL_DS".

This change is to upgrade the DB to meet the above requirement when upgrading DB from old version (1.1) to new version(1.2).

Testing:

1. "DEFAULT" vmgroup present

Configuration:
"_DEFAULT" vmgroup has a privilege to "_DEFAULT_DS"
"vmgroup1" has "default_datastore" set, also has privilege to the "default_datastore"
"vmgroup2" has no "default_datastore" field set

[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup ls
Uuid                                  Name      Description                Default_datastore  VM_list 
------------------------------------  --------  -------------------------  -----------------  ------- 
11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default vmgroup  
 
root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup create --name=vmgroup1 --vm-list=photon-VM1.1
vmgroup 'vmgroup1' is created.  Do not forget to run 'vmgroup vm add' and 'vmgroup access add' commands to enable access control.
 
root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access add --name=vmgroup1 --datastore=sharedVmfs-1 --allow-create --volume-maxsize=500MB --volume-totalsize
=1GB
vmgroup access add succeeded
[root@sc2-rdops-vm01-dhcp-52-142:~]
[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=_DEFAULT
Datastore  Allow_create  Max_volume_size  Total_size 
---------  ------------  ---------------  ---------- 
           True          Unset            Unset      
 
[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=vmgroup1
Datastore     Allow_create  Max_volume_size  Total_size 
------------  ------------  ---------------  ---------- 
sharedVmfs-1  True          500.00MB         1.00GB     


root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup create --name=vmgroup2 
vmgroup 'vmgroup2' is created.  Do not forget to run 'vmgroup vm add' and 'vmgroup access add' commands to enable access control.

After upgrade:
For tenant "_DEFAULT", "default_datastore" is set to "_VM_DS", full access privileges to "_VM_DS" and "_ALL_DS" have been created; while access privilege to "_DEFAULT_DS" has been removed

For tenant "vmgroup1", "default_datastore" remains unchanged (sharedVmfs-1) and the access privilege to that datastore remains unchanged

For tenant "vmgroup2", "default_datastore" is set to "_VM_DS", and a full access privilege to "_VM_DS" has been added

[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup ls
Uuid                                  Name      Description                Default_datastore  VM_list      
------------------------------------  --------  -------------------------  -----------------  ------------ 
11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default vmgroup  _VM_DS                          
8b6a72f8-ef22-4f8c-9dc0-03c442808823  vmgroup1                             sharedVmfs-1       photon-VM1.1 
08646fd0-cbbb-4177-8031-8da38433b321  vmgroup2                             _VM_DS                          
 
[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=_DEFAULT
Datastore  Allow_create  Max_volume_size  Total_size 
---------  ------------  ---------------  ---------- 
_ALL_DS    True          Unset            Unset      
_VM_DS     True          Unset            Unset      
 
[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=vmgroup1
Datastore     Allow_create  Max_volume_size  Total_size 
------------  ------------  ---------------  ---------- 
sharedVmfs-1  True          500.00MB         1.00GB     
 
[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=vmgroup2
Datastore  Allow_create  Max_volume_size  Total_size 
---------  ------------  ---------------  ---------- 
_VM_DS     True          Unset            Unset      

2. "DEFAULT" vmgroup does not present

Configuration:
"vmgroup1" has "default_datastore" set, also has privilege to the "default_datastore"
"vmgroup2" has no "default_datastore" field set

[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup ls
Uuid                                  Name      Description                Default_datastore  VM_list      
------------------------------------  --------  -------------------------  -----------------  ------------ 
11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default vmgroup                                  
1dccf853-61c3-4245-a07b-180f9d4df832  vmgroup1                             sharedVmfs-1       photon-VM1.1 
bde7a773-17f3-47c0-9fae-23431fb4bc28  vmgroup2   

[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup rm --name=_DEFAULT --remove-volumes
All Volumes will be removed
vmgroup rm succeeded

[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=vmgroup1
Datastore     Allow_create  Max_volume_size  Total_size 
------------  ------------  ---------------  ---------- 
sharedVmfs-1  True          500.00MB         1.00GB                                                                

After upgrade:
No tenant "_DEFAULT" presents.
For tenant "vmgroup1", "default_datastore" remains unchanged (sharedVmfs-1) and the access privilege to that datastore remains unchanged

For tenant "vmgroup2", "default_datastore" is set to "_VM_DS", and a full access privilege to "_VM_DS" has been added

[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup ls
Uuid                                  Name      Description  Default_datastore  VM_list      
------------------------------------  --------  -----------  -----------------  ------------ 
1dccf853-61c3-4245-a07b-180f9d4df832  vmgroup1               sharedVmfs-1       photon-VM1.1 
bde7a773-17f3-47c0-9fae-23431fb4bc28  vmgroup2               _VM_DS                          
 
[root@sc2-rdops-vm01-dhcp-52-142:~]
[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=vmgroup1
Datastore     Allow_create  Max_volume_size  Total_size 
------------  ------------  ---------------  ---------- 
sharedVmfs-1  True          500.00MB         1.00GB     
 
[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=vmgroup2
Datastore  Allow_create  Max_volume_size  Total_size 
---------  ------------  ---------------  ---------- 
_VM_DS     True          Unset            Unset      

@lipingxue
Copy link
Contributor Author

I have done the following test manually.

1. "DEFAULT" vmgroup present

Configuration:
"_DEFAULT" vmgroup has a privilege to "_DEFAULT_DS"
"vmgroup1" has "default_datastore" set, also has privilege to the "default_datastore"
"vmgroup2" has no "default_datastore" field set

root@sc2-rdops-vm02-dhcp-88-137:~]
[root@sc2-rdops-vm02-dhcp-88-137:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup ls
Uuid                                  Name      Description                Default_datastore  VM_list 
------------------------------------  --------  -------------------------  -----------------  ------- 
11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default vmgroup                             
e82dd74f-8985-407c-ae87-009aa5b282b0  vmgroup1                             vsanDatastore              
8cc5401d-f0b0-4f5b-8516-a5407be1f03f  vmgroup2                                                         
 
[root@sc2-rdops-vm02-dhcp-88-137:~]
[root@sc2-rdops-vm02-dhcp-88-137:~]
[root@sc2-rdops-vm02-dhcp-88-137:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=_DEFAULT
Datastore  Allow_create  Max_volume_size  Total_size 
---------  ------------  ---------------  ---------- 
           True          Unset            Unset      
 
[root@sc2-rdops-vm02-dhcp-88-137:~]
[root@sc2-rdops-vm02-dhcp-88-137:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=vmgroup1
Datastore      Allow_create  Max_volume_size  Total_size 
-------------  ------------  ---------------  ---------- 
vsanDatastore  True          500.00MB         1.00GB     
 
[root@sc2-rdops-vm02-dhcp-88-137:~]
[root@sc2-rdops-vm02-dhcp-88-137:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=vmgroup2
Datastore  Allow_create  Max_volume_size  Total_size 
---------  ------------  ---------------  ---------- 

After upgrade, check the content in "tenants" table and "privileges" table to make sure it is upgraded as expected (end-to-end testing can be done after PR #1155 is merged).

[root@sc2-rdops-vm02-dhcp-88-137:~] /usr/lib/vmware/sqlite/bin/sqlite3 /etc/vmware/vmdkops/auth-db
SQLite version 3.10.2 2016-01-20 15:27:19
Enter ".help" for usage hints.
sqlite> select * from tenants;
11111111-1111-1111-1111-111111111111|_DEFAULT|This is a default vmgroup|__VM_DS://
e82dd74f-8985-407c-ae87-009aa5b282b0|vmgroup1||/vmfs/volumes/vsan:5263d5bc92964afb-977fc1968eaa1777
8cc5401d-f0b0-4f5b-8516-a5407be1f03f|vmgroup2||__VM_DS://
sqlite>
sqlite> select * from privileges;
e82dd74f-8985-407c-ae87-009aa5b282b0|/vmfs/volumes/vsan:5263d5bc92964afb-977fc1968eaa1777|1|500|1024
11111111-1111-1111-1111-111111111111|__VM_DS://|1|0|0
8cc5401d-f0b0-4f5b-8516-a5407be1f03f|__VM_DS://|1|0|0
11111111-1111-1111-1111-111111111111|__ALL_DS://|1|0|0
 

2. "DEFAULT" vmgroup does not present

Configuration:
"vmgroup1" has "default_datastore" set, also has privilege to the "default_datastore"
"vmgroup2" has no "default_datastore" field set

[root@sc2-rdops-vm02-dhcp-88-137:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup ls
Uuid                                  Name      Description  Default_datastore  VM_list 
------------------------------------  --------  -----------  -----------------  ------- 
e82dd74f-8985-407c-ae87-009aa5b282b0  vmgroup1               vsanDatastore              
8cc5401d-f0b0-4f5b-8516-a5407be1f03f  vmgroup2                                           
 
[root@sc2-rdops-vm02-dhcp-88-137:~]
[root@sc2-rdops-vm02-dhcp-88-137:~]
[root@sc2-rdops-vm02-dhcp-88-137:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=vmgroup1
Datastore      Allow_create  Max_volume_size  Total_size 
-------------  ------------  ---------------  ---------- 
vsanDatastore  True          500.00MB         1.00GB     
 
[root@sc2-rdops-vm02-dhcp-88-137:~]
[root@sc2-rdops-vm02-dhcp-88-137:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup access ls --name=vmgroup2
Datastore  Allow_create  Max_volume_size  Total_size 
---------  ------------  ---------------  ---------- 

After upgrade, check the content in "tenants" table and "privileges" table to make sure it is upgraded as expected (end-to-end testing can be done after PR #1155 is merged).

[root@sc2-rdops-vm02-dhcp-88-137:~] /usr/lib/vmware/sqlite/bin/sqlite3 /etc/vmware/vmdkops/auth-db
SQLite version 3.10.2 2016-01-20 15:27:19
Enter ".help" for usage hints.
sqlite> select * from tenants;
e82dd74f-8985-407c-ae87-009aa5b282b0|vmgroup1||/vmfs/volumes/vsan:5263d5bc92964afb-977fc1968eaa1777
8cc5401d-f0b0-4f5b-8516-a5407be1f03f|vmgroup2||__VM_DS://
11111111-1111-1111-1111-111111111111|_DEFAULT|This is a default vmgroup|__VM_DS
sqlite> select * from privileges;
e82dd74f-8985-407c-ae87-009aa5b282b0|/vmfs/volumes/vsan:5263d5bc92964afb-977fc1968eaa1777|1|500|1024
8cc5401d-f0b0-4f5b-8516-a5407be1f03f|__VM_DS://|1|0|0
11111111-1111-1111-1111-111111111111|__VM_DS|1|0|0
11111111-1111-1111-1111-111111111111|__ALL_DS://|1|0|0

@@ -561,10 +567,46 @@ def handle_upgrade_1_1_to_1_2(self):
"""
sql_script = script.format(DB_MAJOR_VER, DB_MINOR_VER)
self.conn.executescript(sql_script)

# check if _DEFAULT tenant exist, if not, insert _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.

If user has explicitly deleted default tenant ( for tighter access control), is it correct to create it again?


# insert full access privilege to "__ALL_DS" for "_DEFAULT" tenant
all_ds_privilege = (auth_data_const.DEFAULT_TENANT_UUID, auth_data_const.ALL_DS_URL, 1, 0, 0)
self.conn.execute("INSERT OR IGNORE INTO privileges(tenant_id, datastore_url, allow_create, max_volume_size, usage_quota) VALUES (?, ?, ?, ?, ?)",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need ignore here?

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. The handle_upgrade_1_1_to_1_2 method has become quite complicated. I'd recommend to refactor it to extract a few private methods. Not a big concern though.

logging.debug("handle_upgrade_1_1_to_1_2: Insert _DEFAULT tenant to tenants table")

# update the tenants table to set "default_datastore" to "__VM_DS" if "default_datastore" is ""
self.conn.execute("UPDATE OR IGNORE tenants set default_datastore_url = ? where default_datastore_url = ?", (auth_data_const.VM_DS_URL, "" ))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove a redundant space after "IGNORE"

id = r['id']
ds_url = r['default_datastore_url']
privilege = (id, ds_url, 1, 0, 0)
self.conn.execute("INSERT OR IGNORE INTO privileges(tenant_id, datastore_url, allow_create, max_volume_size, usage_quota) VALUES (?, ?, ?, ?, ?)",
Copy link
Contributor

Choose a reason for hiding this comment

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

tip: this can be done using executemany

Copy link
Contributor

@pshahzeb pshahzeb left a comment

Choose a reason for hiding this comment

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

LGTM. Please test end to end, the upgrade scenario after the merge of #1155

@msterin msterin changed the title DB upgrade change for changes to handle default_datastore. DB upgrade fix for changes to handle default_datastore. Apr 22, 2017
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. 2 major issues

  • it is not clear from the comments what the update needs to accomplish
  • too much hard-to-read python code instead of SQL scripts (maybe it's OK but it's hard to tell without more clarity on what is the end result)


# go through tenants table, insert full access privilege to "default_datastore" for each tenant if not present
for r in result:
id = r['id']
Copy link
Contributor

Choose a reason for hiding this comment

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

why it is not something like this for lines 578-587 ?

INSERT OR IGNORE INTO privileges(tenant_id, datastore_url, allow_create, max_volume_size, usage_quota)
  SELECT tenant.id, tenant.default_datastore_url, 1, 0, 0 FROM tenants

logging.debug("handle_upgrade_1_1_to_1_2: update vms table Done")

# update the tenants table to set "default_datastore" to "__VM_DS" if "default_datastore" is ""
self.conn.execute("UPDATE OR IGNORE tenants SET default_datastore_url = ? where default_datastore_url = ?", (auth_data_const.VM_DS_URL, "" ))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need to pass "" when you can put it in the query directly
self.conn.execute("UPDATE OR IGNORE tenants SET default_datastore_url = ? where default_datastore_url = \"\"", (auth_data_const.VM_DS_URL) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


result = cur.fetchall()
logging.debug("handle_upgrade_1_1_to_1_2: Check DEFAULT tenant exist")
if result:
Copy link
Contributor

Choose a reason for hiding this comment

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

it's a better practice to say "if not result: return"
this way you avoid deep nesting and the code is easier to read

logging.debug("handle_upgrade_1_1_to_1_2: Check DEFAULT tenant exist")
if result:
# _DEFAULT tenant exists
# insert full access privilege to "__ALL_DS" for "_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 am not sure what exactly should be the end result here.

I think it is "if there is a default tenant. and it was not edited, then open up ALL_DS for it"
Or something like that. Can you summarize, or point to issue with design ?

Generally I think there should be (1) quick comment about what is the change (e.g. if there is a default tenant. and it was not edited, then open up ALL_DS for it") and then desirably a single SQL script to do it.
I can help with the script once it's clear what's the end result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the end result is:

  1. When default tenant exists, and it was not edited, then open up ALL_DS for it(Remove the access privilege to "_DEFAULT_DS" and insert an full access privilege to "ALL_DS"
  2. When default tenant exists, and it was edited (access privilege to _DEFAULT_DS has been edited by user). I am not sure what is the right behavior in this case. I think it would be replace the "datastore_url" field in existing access privilege to "_DEFAULT_DS" from "_DEFAULT_DS_URL" to "_ALL_DS_URL".
  3. When default tenant does not exist, do nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think case2 is not possible since we don't expose the name "_DEFAULT_DS" to user so I don't think user can remove or modify the access privilege to "_DEFAULT_DS" unless they read the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I think there are only two cases:

  1. DEFAULT tenant exists, and access privilege to "_DEFAULT_DS" is not edited. Replace the access privilege to "_DEFAULT_DS" with full access privilege to "_ALL_DS".
  2. DEFAULT tenant does not exist(deleted by user). Do nothing.
    @msterin Does it make sense to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the tenant exists and default_ds is edited - will it work correctly in the new design ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't show the datastore name "_DEFAULT_DS" when display the access privilege for tenant "_DEFAULT". So it is very unlikely that the "default_ds" is edited.

But in case it has been edited, user may need to tighten the access privilege to "__ALL_DS" if he/she wants.
For example:
Before upgrade, the access privilege to "_DEFAULT_DS" is something like this(allow_create, set 'volume-maxsize" to 500MB and 'volume-totalsize" to 1GB)
("_DEFAULT_TENANT_UUID, "_DEFAULT_DS", 1, 500, 1024).
After upgrade, the above entry will be removed, and a full access privilege to "_ALL_DS" will be inserted.
("_DEFAULT_TENANT_UUID, "_ALL_DS", 1, 0, 0).
With this option, we can guarantee the "volume-totalsize" be "unlimited" based on the constraints we put in code ("volume-totalsize" for "_VM_DS" and "_ALL_DS" must be unlimited).

Another option is:
Don't remove the entry,
("_DEFAULT_TENANT_UUID, "_DEFAULT_DS", 1, 500, 1024).
just replace the "datastore_url" column with "_ALL_DS"
("_DEFAULT_TENANT_UUID, "_ALL_DS", 1, 500, 1024)

But this break the constraint that "volume-totalsize" must be "unlimit" for "_VM_DS" and "_ALL_DS". We put this constraint before our code cannot handle the "usage_quota" for "_VM_DS" and "_ALL_DS".

self.conn.commit()
return None
except sqlite3.Error as e:
error_msg = "Error when upgrading auth DB VMs table"
error_msg = "Error when upgrading auth DB table"
Copy link
Contributor

Choose a reason for hiding this comment

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

please do not hide the exception but add it to the message(not only logging,error)

@lipingxue lipingxue force-pushed the handle_upgrade_for_default_ds.liping branch from ac02755 to f38124c Compare April 23, 2017 06:41
@lipingxue lipingxue force-pushed the handle_upgrade_for_default_ds.liping branch from f38124c to 36214fd Compare April 23, 2017 14:40
@lipingxue
Copy link
Contributor Author

@msterin I have addressed your comments, please take a look. Thanks!

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.

Generally looks good.
A few nit/comments/questions inside.
Please add test info - what was the 1.1 DB variants you used, what is the output, how did you check the result is correct.
Also, can we do some test automation here ? (optional. Manual is fine for this a long as it's clear what did we test)

@@ -561,11 +568,43 @@ def handle_upgrade_1_1_to_1_2(self):
"""
sql_script = script.format(DB_MAJOR_VER, DB_MINOR_VER)
self.conn.executescript(sql_script)

logging.debug("handle_upgrade_1_1_to_1_2: update vms table Done")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think upgrade logging can mainly go to info. Upgrade errors are notorious and we'd better have all data no matter what the logging level was. Plus, upgrade is one time event so we do not pollute the logs...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

result = cur.fetchall()

self.conn.execute("INSERT OR IGNORE INTO privileges(tenant_id, datastore_url, allow_create, max_volume_size, usage_quota)"
"SELECT tenants.id, tenants.default_datastore_url, 1, 0, 0 FROM tenants")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: just for readability, a single string is nicer.
"""INSERT or IGNORE ....
SELECT ..."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

logging.debug("handle_upgrade_1_1_to_1_2: Check DEFAULT tenant exist")
if result:
# _DEFAULT tenant exists
# insert full access privilege to "__ALL_DS" for "_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.

What if the tenant exists and default_ds is edited - will it work correctly in the new design ?

@@ -30,7 +30,7 @@
# since we rely on it to locate log file name after config is loaded.
LOG_CONFIG_FILE = "/etc/vmware/vmdkops/log_config.json"

LOG_LEVEL_DEFAULT = 'INFO'
LOG_LEVEL_DEFAULT = 'DEBUG'
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, I suggest keeping INFO here and really use logging.inf() instead of logging.debug(). We may need a bit better message but generally what's there is acceptble

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@lipingxue
Copy link
Contributor Author

@msterin I have addressed you comments and added the test info in the PR description. Please take a look. Thanks!

@lipingxue
Copy link
Contributor Author

I talked with Mark, and he is OK with the change after I addressed his latest comments. I will merge it.

@lipingxue lipingxue merged commit deeec23 into master Apr 25, 2017
@shuklanirdesh82 shuklanirdesh82 deleted the handle_upgrade_for_default_ds.liping branch April 27, 2017 21:21
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.

5 participants