Implement "config rm --unlink". #1186

lipingxue · 2017-04-24T16:50:28Z

Fixed #1138.
This PR includes the change to implement "config rm --unlink".

[ ] In SingleNode, rm --local will remove the local DB, rm --unlink will return an error message to say this command is not supported in SingleNode configuration.

[ ] In MultiNode, rm --unlink will print a message and remove the local symlink. rm --local will return an error message to say rm --unlink is not support in MultiNode configuration.

Testing:

Configured in SingleNodeMode:

run config rm

[root@sc2-rdops-vm01-dhcp-34-30:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py config rm
 
        Shared DB removal is not supported. For removing  local configuration, use --local flag.
        For removing shared DB,  run 'vmdkops_admin config rm --unlink' on ESX hosts using this DB,
        and manually remove the vmdkops_config.db file from shared storage.

run config rm --unlink

[root@sc2-rdops-vm01-dhcp-34-30:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py config rm --unlink
Warning: For extra safety, removal operation requires '--confirm' flag.
[root@sc2-rdops-vm01-dhcp-34-30:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py config rm --unlink --confirm
'rm --unlink' is not supported when Config DB  is in SingleNode mode. Use 'rm --local' to remove the local link or local DB.

run config rm --local

root@sc2-rdops-vm01-dhcp-34-30:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py config rm --local --confirm
Moved /etc/vmware/vmdkops/auth-db to backup file /etc/vmware/vmdkops/auth-db.bak_Thu_Apr_20_23:13:07_2017

[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py config rm --unlink --confirm
Removed link /etc/vmware/vmdkops/auth-db
[root@sc2-rdops-vm01-dhcp-52-142:~]
[root@sc2-rdops-vm01-dhcp-52-142:~]
[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py status
=== Service:
Version: 0.13.87ce18c-0.0.1
Status: Running
Pid: 734227
Port: 1019
LogConfigFile: /etc/vmware/vmdkops/log_config.json
LogFile: /var/log/vmware/vmdk_ops.log
LogLevel: INFO
=== Authorization Config DB:
DB_SharedLocation: N/A
DB_Mode: NotConfigured (no local DB, no symlink to shared DB)
DB_LocalPath: N/A

Configured in MultiNodeMode:

run config rm

[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py config rm
 
        Shared DB removal is not supported. For removing  local configuration, use --local flag.
        For removing shared DB,  run 'vmdkops_admin config rm --unlink' on ESX hosts using this DB,
        and manually remove the vmdkops_config.db file from shared storage.

run config rm --local

[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py config rm --local
Warning: For extra safety, removal operation requires '--confirm' flag.
[root@sc2-rdops-vm01-dhcp-52-142:~]
[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py config rm --local --confirm
'rm --local' is not supported when Config DB is in MultiNode mode.Use 'rm --unlink' to remove the local link to shared DB.

run config rm --unlink

[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py config rm --unlink --confirm
Removed link /etc/vmware/vmdkops/auth-db
[root@sc2-rdops-vm01-dhcp-52-142:~]
[root@sc2-rdops-vm01-dhcp-52-142:~]
[root@sc2-rdops-vm01-dhcp-52-142:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py status
=== Service:
Version: 0.13.87ce18c-0.0.1
Status: Running
Pid: 734227
Port: 1019
LogConfigFile: /etc/vmware/vmdkops/log_config.json
LogFile: /var/log/vmware/vmdk_ops.log
LogLevel: INFO
=== Authorization Config DB:
DB_SharedLocation: N/A
DB_Mode: NotConfigured (no local DB, no symlink to shared DB)
DB_LocalPath: N/A

shaominchen · 2017-04-24T17:25:24Z

esx_service/cli/vmdkops_admin.py

@@ -1301,16 +1305,38 @@ def config_rm(args):
    # This asks for double confirmation, and removes the local link or DB (if any)
    # NEVER deletes the shared database - instead prints help

-    if not args.local:
+    if not args.local and not args.unlink:
        return err_out("""
        Shared DB removal is not supported. For removing  local configuration, use --local flag.


Nit: remove extra space - "removing local"

The whole message has been rewritten per Mark's comments.

shaominchen · 2017-04-24T17:25:50Z

esx_service/cli/vmdkops_admin.py

        return err_out("""
        Shared DB removal is not supported. For removing  local configuration, use --local flag.
-        For removing shared DB,  run 'vmdkops_admin config rm --local' on ESX hosts using this DB,
+        For removing shared DB,  run 'vmdkops_admin config rm --unlink' on ESX hosts using this DB,


Nit: remove extra space - "DB, run"

Same above.

shaominchen · 2017-04-24T17:26:39Z

esx_service/cli/vmdkops_admin.py

@@ -1301,16 +1305,38 @@ def config_rm(args):
    # This asks for double confirmation, and removes the local link or DB (if any)
    # NEVER deletes the shared database - instead prints help

-    if not args.local:
+    if not args.local and not args.unlink:
        return err_out("""
        Shared DB removal is not supported. For removing  local configuration, use --local flag.


Nit: I'd suggest to move "Shared DB removal is not supported" to the last statement.

Same above.

shaominchen · 2017-04-24T17:28:27Z

esx_service/cli/vmdkops_admin.py

        and manually remove the {} file from shared storage.
        """.format(auth_data.CONFIG_DB_NAME))

+    # Check the existing config mode


Can we extract this section as a separate method for code reuse?

Issue #1187 is filed to track this.

shaominchen · 2017-04-24T17:30:49Z

esx_service/cli/vmdkops_admin.py

+            mode = auth.mode # for usage outside of the 'with'
+        except auth_data.DbAccessError as ex:
+            return err_out(str(ex))
+
    if not args.confirm:


Let's move this argument check before line 1315.

shaominchen · 2017-04-24T17:34:18Z

esx_service/cli/vmdkops_admin.py

+        pass
+    elif mode == auth_data.DBMode.MultiNode:
+        if args.local:
+            return err_out("'rm --local' is not supported when " + DB_REF + "is in MultiNode mode."


Nit: add a space before and after "is in MultiNode mode."

DB_REF already included the space.

shaominchen · 2017-04-24T17:36:06Z

esx_service/cli/vmdkops_admin.py

+    elif mode == auth_data.DBMode.SingleNode:
+        if args.unlink:
+            return err_out("'rm --unlink' is not supported when " + DB_REF + " is in SingleNode mode."
+                        " Use 'rm --local' to remove the local link or local DB.")


Why "local link or local DB"? Do we also create a symlink to local DB? If yes, we should say "remove local link and local DB" or simply "remove local DB configuration"

msterin

Thanks for putting it together. Some of the messages need rework (suggestions included) , but mainly - the new code is unnecessary complex. Suggestions are also included - please take a loo

msterin · 2017-04-24T17:27:26Z

esx_service/cli/vmdkops_admin.py

        return err_out("""
        Shared DB removal is not supported. For removing  local configuration, use --local flag.
-        For removing shared DB,  run 'vmdkops_admin config rm --local' on ESX hosts using this DB,
+        For removing shared DB,  run 'vmdkops_admin config rm --unlink' on ESX hosts using this DB,
        and manually remove the {} file from shared storage.


I think the message no longer matches the functionality.
Here it should say:
"""
DB removal is irreversible operation. Please use --local flag for removing DB in SingeNode mode,
and use --unlink to unlink from DB in MultiNode mode".
Note that --unlink will not remove a shared DB, but simply configure the current ESXi host to stop using it.
For removing shared DB, run 'vmdkops_admin config rm --unlink' on ESXi hosts using this DB, and then manually remove the actual DB file from shared storage."""

msterin · 2017-04-24T17:29:28Z

esx_service/cli/vmdkops_admin.py

    if not args.confirm:
        return err_out("Warning: For extra safety, removal operation requires '--confirm' flag.")

+    if mode == auth_data.DBMode.NotConfigured:
+        pass


I think it should say "Nothing to do - DB is not configured".
It also should be handled together with all other "noops" csses - see below

msterin · 2017-04-24T17:33:26Z

esx_service/cli/vmdkops_admin.py

        and manually remove the {} file from shared storage.
        """.format(auth_data.CONFIG_DB_NAME))

+    # Check the existing config mode


Missing check
`if args.local and args.unlink: err_out("cannot use both flags together" + help message above about when to use what)

msterin · 2017-04-24T17:37:32Z

esx_service/cli/vmdkops_admin.py

+    if mode == auth_data.DBMode.NotConfigured:
+        pass
+    elif mode == auth_data.DBMode.MultiNode:
+        if args.local:


the whole code should be simpler , something like this:

if mode == DBMode.MutiNode: if arg.local: err_out(...) else: move_to_backup return service_reset if mode == DBMode.SingleNode: if args.unlink: err_out else: rm link return service_reset # all other cases print("Noting to do. Mode =<mode>"

msterin · 2017-04-24T17:40:48Z

esx_service/cli/vmdkops_admin.py

+            auth.connect()
+            info = auth.get_info()
+            mode = auth.mode # for usage outside of the 'with'
+        except auth_data.DbAccessError as ex:


I think it's generally a bad idea to catch exception half way in command line.
We should let them bubble to main and just print proper error message there.
I think that's what we already do, don't we ?
Can you check what's the behavior if there is no catch here, and connect() does throw DBAccessError ? (this is not a blocker so if you are out of time feel free to skip this comment)

Here we cannot let the code go through. When the exception happens, we cannot get the correct config mode from DB. And our following code depends on the config mode. We have similar logic in the "config_init()" too.

lipingxue · 2017-04-24T20:44:06Z

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

shaominchen

LGTM.

govint · 2017-04-26T05:15:15Z

@lipingxue, pls. add tests as below,

For both local and multi-node cases
a. Remove the DB and then run vmgroup commands to create/delete a group. Just to see if anything is broken and the user is able to use the system normally.
For multi-node case
a. Two ESX hosts with shared DS.
b. Config rm --unlink the DB on one node, confirm that the other node is able to use the DB with no issue.
c. Check the node (in (b)) can restore the link on "config init". The next config init on the node that did the "config rm --unlink" should check that there is already a DB on the shared DS and link to it. Confirm that this is happening.

govint · 2017-04-26T05:23:21Z

esx_service/cli/vmdkops_admin.py

+    if mode == auth_data.DBMode.MultiNode:
+        if args.local:
+            return err_out("'rm --local' is not supported when " + DB_REF + "is in MultiNode mode."
+                           " Use 'rm --unlink' to remove the local link to shared DB.")


Is it necessary to have a --local and --unlink. A --unlink is sufficient for both single and multi-node cases. In the single node case it destroys the DB (or backs it up) and in the multi-node case simply delinks the host from the shared DB. Since the configured mode is already known to the system, it can differentiate the cases and figure how to interpret the unlink.

The --local just seems extra and user needs to keep track of the mode.

A host may have a local datastore and have a DB on that and a shared DS with a DB on that. How does the user decide which DB on which DS to remove? Don't we need to pass the DS name on the command line?? Or multiple shared DS'es on the same host.

govint · 2017-04-26T05:26:40Z

esx_service/cli/vmdkops_admin.py

+        and use '--unlink' to unlink from DB in MultiNode mode.
+        """
+        )
+
    if not args.confirm:
        return err_out("Warning: For extra safety, removal operation requires '--confirm' flag.")


Not this change, why does the user need to provide two options?? --local and --confirm, it would make sense if the confirmation is asked by the CLI. If the user provides "rm --local" then they want to remove the DB, the user typing out another "--confirm" doesn't make any difference - they still want to remove the DB.

govint

Can you also try this out on VSAN and say how the DB is configured there? Is it local or multi-node?

pshahzeb · 2017-04-27T04:38:01Z

@govint
I did the the tests that you had mentioned with this change. Following are the results

After removing DB in both modes, the admin command fails with following error:

Please init configuration with 'vmdkops_admin.py config init' before changing it.

For this multinode test, the behavior is exactly the same as you described. i.e. the node that unlinks the DB detects the symlink when config init done again. Access from the second ESX node remains consistent. I also verified that vmgroups are consistently visible across ESX nodes when config init is done ( init -> unlink > init).

Can you also try this out on VSAN and say how the DB is configured there? Is it local or multi-node?

I did the above tests for shared mode on VSAN itself. It's configured the same way. Were you implying something else?

govint · 2017-04-27T04:50:28Z

@pshahzeb, thanks for this verification.

For VSAN my doubt was since its a single datastore across all hosts in the cluster (yes?) is the DB visible to all nodes and hence is it local or multi-node? And whether the link to the DB is visible to all nodes (its a single datastore). Is it then a single link or per node?

govint

Looks good.

lipingxue added 2 commits April 24, 2017 09:46

Implement "config rm --unlink".

e9fd900

Fix error message.

8ca2a56

vmwclabot added the cla-not-required label Apr 24, 2017

shaominchen reviewed Apr 24, 2017

View reviewed changes

msterin reviewed Apr 24, 2017

View reviewed changes

Address comments from Mark and Sam.

cb36cfe

shaominchen approved these changes Apr 24, 2017

View reviewed changes

govint reviewed Apr 26, 2017

View reviewed changes

govint approved these changes Apr 27, 2017

View reviewed changes

pshahzeb merged commit 43a34d2 into master Apr 27, 2017

shuklanirdesh82 deleted the config_rm_unlink.liping branch April 27, 2017 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement "config rm --unlink". #1186

Implement "config rm --unlink". #1186

lipingxue commented Apr 24, 2017 •

edited

Loading

shaominchen Apr 24, 2017

lipingxue Apr 24, 2017

shaominchen Apr 24, 2017

lipingxue Apr 24, 2017

shaominchen Apr 24, 2017

lipingxue Apr 24, 2017

shaominchen Apr 24, 2017

lipingxue Apr 24, 2017

shaominchen Apr 24, 2017

lipingxue Apr 24, 2017

shaominchen Apr 24, 2017

lipingxue Apr 24, 2017

shaominchen Apr 24, 2017

lipingxue Apr 24, 2017

msterin left a comment

msterin Apr 24, 2017

lipingxue Apr 24, 2017

msterin Apr 24, 2017

lipingxue Apr 24, 2017

msterin Apr 24, 2017

lipingxue Apr 24, 2017

msterin Apr 24, 2017

lipingxue Apr 24, 2017

msterin Apr 24, 2017

lipingxue Apr 24, 2017

lipingxue commented Apr 24, 2017

shaominchen left a comment

govint commented Apr 26, 2017

govint Apr 26, 2017 •

edited

Loading

govint Apr 26, 2017

govint left a comment

pshahzeb commented Apr 27, 2017 •

edited

Loading

govint commented Apr 27, 2017

govint left a comment

Implement "config rm --unlink". #1186

Implement "config rm --unlink". #1186

Conversation

lipingxue commented Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msterin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lipingxue commented Apr 24, 2017

shaominchen left a comment

Choose a reason for hiding this comment

govint commented Apr 26, 2017

govint Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint left a comment

Choose a reason for hiding this comment

pshahzeb commented Apr 27, 2017 • edited Loading

govint commented Apr 27, 2017

govint left a comment

Choose a reason for hiding this comment

lipingxue commented Apr 24, 2017 •

edited

Loading

govint Apr 26, 2017 •

edited

Loading

pshahzeb commented Apr 27, 2017 •

edited

Loading