Fix Disk Util and Bek Disk Mounting #711
Conversation
vermashi
commented
Dec 28, 2018
vermashi
commented
- Fix the disk util disk enumeration error
- Fix the BEK Mounting permissions
- Remove KV response logging line
VMEncryption/main/DiskUtil.py
Outdated
| crypt_item.mapper_name = "osencrypt" | ||
| if encryption_status["os"] == "Encrypted" and not rootfs_crypt_item_found: | ||
| crypt_item = CryptItem() | ||
| crypt_item.mapper_name = "osencrypt" |
There was a problem hiding this comment.
try using a variable for this and other instances below.
VMEncryption/main/KeyVaultUtil.py
Outdated
| @@ -226,7 +226,7 @@ def create_secret(self, AccessToken, KeyVaultURL, secret_value, KeyEncryptionAlg | |||
|
|
|||
| self.logger.log("{0} {1}".format(result.status, result.getheaders())) | |||
| result_content = result.read() | |||
| self.logger.log("result_content is {0}".format(result_content)) | |||
| # self.logger.log("result_content is {0}".format(result_content)) | |||
There was a problem hiding this comment.
And maybe add a comment to not log the result in future since it contains secret
In reply to: 244847482 [](ancestors = 244847482)
vermashi
force-pushed
the
ade-dp-fix-disk-util
branch
from
1ef17a9
to
b062695
Compare
| @@ -16,22 +16,23 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| from DiskUtil import * | |||
| from Common import * | |||
| from Common import TestHooks | |||
There was a problem hiding this comment.
I'm not sure this test hook is even being used anymore. I would prefer removing the import, the TestHook class from Common, and switch to using mocked unit tests or otherwise in the future to fully clean this up.
There was a problem hiding this comment.
I am OK with doing this next step of clean up in a separate PR if it makes more sense to do it that way.
There was a problem hiding this comment.
will address in future PR.
| """ | ||
| self.make_sure_path_exists(mount_point) | ||
| mount_cmd = self.distro_patcher.mount_path + ' -L "' + bek_label + '" ' + mount_point + ' -o ' + option_string | ||
| return self.command_executor.Execute(mount_cmd) |
There was a problem hiding this comment.
A fair amount of confusing log spew is being generated when BEK volume is already mounted and this function is called (because the mount fails in that case and the default parameter value when not specified is to log all errors). The end goal of this function is to exit with the BEK volume mounted. If we call Execute with suppress_logging=False and simply check the return code ourselves, we could eliminate log spew in cases where BEK volume is mounted and mount fails only because it was already mounted and there was nothing to do. This is a benign case but does generate log spew that makes diagnosing real failures more difficult. Another option could be to determine if it is already mounted and if so, skip the Execute.
There was a problem hiding this comment.
This is also something we can work on in a future PR if it makes more sense to do that outside of this change.
There was a problem hiding this comment.
will address in future PR.
| def enable_encryption_format(passphrase, disk_format_query, disk_util, force=False): | ||
| logger.log('enable_encryption_format') | ||
| logger.log('disk format query is {0}'.format(disk_format_query)) | ||
|
|
||
| json_parsed = json.loads(disk_format_query) | ||
|
|
||
| if type(json_parsed) is dict: | ||
| encryption_format_items = [json_parsed,] | ||
| encryption_format_items = [json_parsed, ] |
There was a problem hiding this comment.
Is this proper syntax? I know it was from before but it looks odd..
There was a problem hiding this comment.
Did some testing and it does not cause a syntax error, although I'm not sure it is needed either (may have been left over from some prior change).
|
@vermashi Back to you |
