-
Notifications
You must be signed in to change notification settings - Fork 95
DB upgrade fix for changes to handle default_datastore. #1179
Conversation
I have done the following test manually. 1. "DEFAULT" vmgroup present Configuration:
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).
2. "DEFAULT" vmgroup does not present Configuration:
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).
|
esx_service/utils/auth_data.py
Outdated
@@ -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 |
There was a problem hiding this comment.
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?
esx_service/utils/auth_data.py
Outdated
|
||
# 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 (?, ?, ?, ?, ?)", |
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
esx_service/utils/auth_data.py
Outdated
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, "" )) |
There was a problem hiding this comment.
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"
esx_service/utils/auth_data.py
Outdated
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 (?, ?, ?, ?, ?)", |
There was a problem hiding this comment.
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
There was a problem hiding this 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
There was a problem hiding this 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)
esx_service/utils/auth_data.py
Outdated
|
||
# go through tenants table, insert full access privilege to "default_datastore" for each tenant if not present | ||
for r in result: | ||
id = r['id'] |
There was a problem hiding this comment.
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
esx_service/utils/auth_data.py
Outdated
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, "" )) |
There was a problem hiding this comment.
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) )
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- 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"
- 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".
- When default tenant does not exist, do nothing.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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".
- DEFAULT tenant does not exist(deleted by user). Do nothing.
@msterin Does it make sense to you?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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".
esx_service/utils/auth_data.py
Outdated
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" |
There was a problem hiding this comment.
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)
ac02755
to
f38124c
Compare
f38124c
to
36214fd
Compare
@msterin I have addressed your comments, please take a look. Thanks! |
There was a problem hiding this 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)
esx_service/utils/auth_data.py
Outdated
@@ -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") |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
esx_service/utils/auth_data.py
Outdated
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") |
There was a problem hiding this comment.
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 ..."""
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 ?
esx_service/utils/log_config.py
Outdated
@@ -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' |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@msterin I have addressed you comments and added the test info in the PR description. Please take a look. Thanks! |
I talked with Mark, and he is OK with the change after I addressed his latest comments. I will merge it. |
PR #1155 introduced the change to handle default_datastore. With that change, it requires:
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
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 unchangedFor tenant "vmgroup2", "default_datastore" is set to "_VM_DS", and a full access privilege to "_VM_DS" has been added
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
After upgrade:
No tenant "_DEFAULT" presents.
For tenant "vmgroup1", "default_datastore" remains unchanged (
sharedVmfs-1
) and the access privilege to that datastore remains unchangedFor tenant "vmgroup2", "default_datastore" is set to "_VM_DS", and a full access privilege to "_VM_DS" has been added