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
Merged

DB upgrade fix for changes to handle default_datastore. #1179

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c7193ad
DB update change for handle default_datastore.
lipingxue Apr 19, 2017
6ddfb5e
Address review comments from Shahzeb.
lipingxue Apr 19, 2017
716857d
Saving changes.
lipingxue Apr 23, 2017
36214fd
Address comments from Mark.
lipingxue Apr 23, 2017
bc5565f
Address additional comments from Mark.
lipingxue Apr 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions esx_service/utils/auth_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,8 +548,15 @@ def handle_upgrade_1_1_to_1_2(self):
the vm names for existing records. We try to populate them and keep None for vms
for which the name couldn't be found. The vmdk_ops admin code which tries to use
this vm names handles names which are None
In 1.2, "default_datastore" field must be set in tenants table, so the upgrade process
will try to set the "default_datastore" field if needed
In 1.2, for each tenant in tenants table, a privilege to "default_datastore" must be
present, the upgrade process will try to create this privilege if needed
In 1.2, for "_DEFAULT" tenant, privilege to "_DEFAULT_DS" need to be removed, and privilege
to "__VM_DS" and "__ALL_DS" need to be inserted
"""
try:
logging.debug("handle_upgrade_1_1_to_1_2: Start")
self.conn.create_function('name_from_uuid', 1, vmdk_utils.get_vm_name_by_uuid)
# Alter vms table to add a new column name vm_name to store vm name
# update all the existing records with the vm_name.
Expand All @@ -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.


# 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,))
logging.debug("handle_upgrade_1_1_to_1_2: update default_datastore in tenants table")

cur = self.conn.execute("SELECT * FROM tenants")
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: Insert privilege to default_datastore in privileges table")

cur = self.conn.execute("SELECT * FROM tenants WHERE id = ?",
(auth_data_const.DEFAULT_TENANT_UUID,)
)

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

# _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".

all_ds_privilege = (auth_data_const.DEFAULT_TENANT_UUID, auth_data_const.ALL_DS_URL, 1, 0, 0)
self.conn.execute("INSERT INTO privileges(tenant_id, datastore_url, allow_create, max_volume_size, usage_quota) VALUES (?, ?, ?, ?, ?)",
all_ds_privilege)
logging.debug("handle_upgrade_1_1_to_1_2: Insert privilege to __ALL_DS for _DEFAULT tenant in privileges table")
# remove access privilege to "DEFAULT_DS"
self.conn.execute("DELETE FROM privileges WHERE tenant_id = ? AND datastore_url = ?",
[auth_data_const.DEFAULT_TENANT_UUID, auth_data_const.DEFAULT_DS_URL])
logging.debug("handle_upgrade_1_1_to_1_2: Remove privilege to _DEFAULT_DS for _DEFAULT tenant in privileges table")
self.conn.commit()
return None
except sqlite3.Error as e:
error_msg = "Error when upgrading auth DB VMs table"
logging.error("handle_upgrade_1_1_to_1_2. %s: %s", error_msg, str(e))
error_msg = "Error when upgrading auth DB table({})".format(str(e))
logging.error("handle_upgrade_1_1_to_1_2. %s", error_msg)
raise DbUpgradeError(self.db_path, error_msg)

def __handle_upgrade(self):
Expand Down
1 change: 1 addition & 0 deletions esx_service/utils/auth_data_const.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
DEFAULT_DS = '_DEFAULT'
DEFAULT_DS_URL = DEFAULT_DS + "_URL"
ORPHAN_TENANT = "_ORPHAN"

VM_DS = '_VM_DS'
VM_DS_URL = VM_DS + "://"
ALL_DS = '_ALL_DS'
Expand Down
2 changes: 1 addition & 1 deletion esx_service/utils/log_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


# Defaults for log files - used to generate conf file if it is missing
# Note: log file location should be synced with CI and 'make'
Expand Down