Changed Unmanage flow - Unmanage cluster will happen per node and not via tendrl/monitor #975

anmolsachan · 2018-05-22T15:47:00Z

tendrl-bug-id: #841
bugzilla: 1583590
Signed-off-by: Anmol Sachan anmol13694@gmail.com

centos-ci · 2018-05-22T15:47:01Z

Can one of the admins verify this patch?

centos-ci · 2018-05-22T15:47:02Z

Can one of the admins verify this patch?

anmolsachan · 2018-05-23T05:55:31Z

@r0h4n @shtripat @GowthamShanmugam , Please review.

anmolsachan · 2018-05-24T12:49:23Z

@Tendrl/admins Please DO NOT merge this PR, There is some new code that went in monitoring integration, need to verify again with same first.

anmolsachan · 2018-05-24T14:02:07Z

@Tendrl/admins PR verified. Can be merged now.

shtripat · 2018-05-25T04:14:46Z

tendrl/commons/flows/unmanage_cluster/__init__.py

+                raise FlowExecutionFailedError(
+                    "Cluster is already in un-managed state"
+                )
+        if (_cluster.status is not None and


I feel this check should not be done on child task of un-manage flow, so cant you move this inside if condition at L#55?

Agreed. Will put it in first condition itself rather than L#55. Flow will be like : Check if another job being executed or not ( checked by parent node which has accepted unmanage job) -> If not , lock the cluster and then run the unmanage flow for the parent node.

shtripat · 2018-05-25T04:17:31Z

tendrl/commons/flows/unmanage_cluster/__init__.py

+                        loop_count += 1
+                        continue
+
+                # Deleting cluster details


Create a separate function for this and invoke here

shtripat · 2018-05-25T04:19:09Z

tendrl/commons/flows/unmanage_cluster/__init__.py

+                _cluster = NS.tendrl.objects.Cluster(
+                    integration_id=integration_id
+                ).load()
+                _cluster.short_name = ""


Why short_name to be un-set after un-manage? After un-manage short_name should be retained I feel, and while re-import user should have option to provide a different name or retain the same.
@nthomas-redhat @r0h4n @julienlim thoughts?

I have same thoughts, By design, it should be an option for user to retain the same name of the cluster as it was part of their infra before. Needs to be discussed.

So what I feel is, if cluster.short_name available while import populate the same in UI screen where list of nodes to be imported are listed. User still should be able to edit the field.

@shtripat @anmolsachan +1 that it would be nice to retain the short_name during the unmanage if possible so that if the same cluster was imported again, user had the option to reuse the same short name. However, if it's too complex, then we defer to this in the future as I think it's a nice-to-have.

@nthomas-redhat

@gnehapk Will this be a big change for UI ? Can this be done ?

@anmolsachan Its a small change in UI. UI will by populate cluster name field with short_name if exist else integration_id on import cluster flow. The cluster name is a editable field and can be changed by user. Created issue for the same Tendrl/ui#973

@gnehapk @shtripat Back-end will save the cluster name now. Made the required change

shtripat · 2018-05-25T04:21:39Z

tendrl/commons/objects/cluster/atoms/unmanage_cluster/__init__.py

+                    flow_id=self.parameters['flow_id']
+                )
+                loop_count = 0
+                # Wait for (no of nodes) * 6 minutes for unmanage to complete


Do we really need to wait for 6 mins per node for un-manage to complete? Its not an import where packages installation etc happen. So I feel a couple of mins would more more than required.

Sure. Will reduce the time

shtripat · 2018-05-25T04:24:41Z

tendrl/commons/objects/node/atoms/stop_integration_services/__init__.py

+        super(StopIntegrationServices, self).__init__(*args, **kwargs)
+
+    def run(self):
+        services = ["tendrl-gluster-integration"]


Write a utility function which takes list of services and stops+disables the services. The same utility could be called from both the atoms, StopMonitoringServices and StopIntegrationServices.

Sure, Will do it

anmolsachan · 2018-05-28T14:46:39Z

@Tendrl/admins @shtripat Incorporated the recommended changes. Please review

shtripat

Looks fine. Minor ones.

shtripat · 2018-05-29T02:30:14Z

tendrl/commons/objects/node/atoms/stop_integration_services/__init__.py

+
+    def run(self):
+        services = ["tendrl-gluster-integration"]
+        return service_utils.stop_service(services, self.parameters)


Make it one statement as below

return service_utils.stop_service( ['tendrl-gluster-integration'], self.parameters )

shtripat · 2018-05-29T02:30:26Z

tendrl/commons/objects/node/atoms/stop_monitoring_services/__init__.py

+
+    def run(self):
+        services = ["collectd"]
+        return service_utils.stop_service(services, self.parameters)


Same as above

shtripat · 2018-05-30T09:21:01Z

tendrl/commons/flows/unmanage_cluster/__init__.py

+                        )
+                        return False
+                    time.sleep(5)
+                    finished = True


Recent changes from @GowthamShanmugam for failed vs finished jobs to be taken care as if one of the child jobs fail, we should not straightway fail the parent job. We should still wait for all the child jobs to finish/fail and then only parent job should be marked.

Below code at L#114 still checks for finished state only. I feel if its finished or failed, in both cases we should break the loop. @GowthamShanmugam ?

yes there also he has to change like job.status in ["finished", "failed"]

shtripat · 2018-05-30T09:25:30Z

tendrl/commons/objects/cluster/atoms/unmanage_cluster/__init__.py

+                        child_job = NS.tendrl.objects.Job(
+                            job_id=child_job_id
+                        ).load()
+                        if child_job.status != "finished":


I feel this can be as below

if child_job.status not in ['finished', 'failed']

@GowthamShanmugam ^^^ ?

Will make the change

anmolsachan · 2018-05-30T15:28:57Z

@shtripat @r0h4n Made the recommended changed. Also verified the pr. Please review

shtripat

Over all looks fine. One small minor comment.
Make sure all the scenario are tested well and back to back import + unmanage + import is tested well.

shtripat · 2018-05-31T05:41:31Z

tendrl/commons/flows/unmanage_cluster/__init__.py

+                        )
+                        return False
+                    time.sleep(5)
+                    finished = True


Below code at L#114 still checks for finished state only. I feel if its finished or failed, in both cases we should break the loop. @GowthamShanmugam ?

anmolsachan · 2018-05-31T09:36:04Z

Updated And Verified.

r0h4n · 2018-06-06T07:31:51Z

@shtripat please complete your reviews on this

r0h4n · 2018-06-13T05:42:59Z

@shtripat @GowthamShanmugam Please finish reviewing this

shtripat

LGTM

r0h4n · 2018-06-13T08:09:34Z

tendrl/commons/flows/service_utils.py

@@ -0,0 +1,55 @@
+""" This utility can be used to handle(start, stop, etc.) tendrl services.


@shtripat @GowthamShanmugam Why we need another service util or did you guys miss this during review?

Could you help @anmolsachan with re-using and modifying below for this PR

https://github.com/Tendrl/commons/blob/master/tendrl/commons/utils/service.py

It does disabling of service as well so thought of writing new one. But I feel at least the code can move to https://github.com/Tendrl/commons/blob/master/tendrl/commons/utils/service.py

Sure. Will make the change

… via tendrl/monitor tendrl-bug-id: Tendrl#841 bugzilla: 1583590 Signed-off-by: Anmol Sachan <anmol13694@gmail.com>

tendrl-bug-id: Tendrl#841 bugzilla: 1583590 Signed-off-by: Anmol Sachan <anmol13694@gmail.com>

anmolsachan · 2018-07-03T09:40:38Z

Closing thing for now, It will require a lot of change because of recent merges. Will raise a new PR if required, and will link to this one.

anmolsachan requested a review from a team as a code owner May 22, 2018 15:47

anmolsachan force-pushed the master branch 2 times, most recently from 0635467 to da12256 Compare May 22, 2018 19:25

anmolsachan force-pushed the master branch from da12256 to 2f6889c Compare May 23, 2018 06:03

anmolsachan force-pushed the master branch from 2f6889c to 570230c Compare May 24, 2018 14:00

anmolsachan force-pushed the master branch from 570230c to 377ed71 Compare May 24, 2018 16:11

shtripat reviewed May 25, 2018

View reviewed changes

anmolsachan force-pushed the master branch 2 times, most recently from ec6a2a4 to ae376ce Compare May 28, 2018 14:44

shtripat reviewed May 29, 2018

View reviewed changes

anmolsachan force-pushed the master branch 3 times, most recently from 2047775 to 280cef2 Compare May 30, 2018 08:57

shtripat reviewed May 30, 2018

View reviewed changes

shtripat reviewed May 31, 2018

View reviewed changes

anmolsachan force-pushed the master branch from cdc7e6d to 22786c4 Compare May 31, 2018 09:29

anmolsachan force-pushed the master branch from 33c8765 to a91cc10 Compare May 31, 2018 09:39

shtripat reviewed Jun 13, 2018

View reviewed changes

r0h4n suggested changes Jun 13, 2018

View reviewed changes

Changed Unmanage flow - Unmanage cluster will happen per node and not…

3c27017

… via tendrl/monitor tendrl-bug-id: Tendrl#841 bugzilla: 1583590 Signed-off-by: Anmol Sachan <anmol13694@gmail.com>

anmolsachan added 4 commits June 15, 2018 14:33

Changed Unmanage flow - incorporated review comments

d0dd633

tendrl-bug-id: Tendrl#841 bugzilla: 1583590 Signed-off-by: Anmol Sachan <anmol13694@gmail.com>

Changed Unmanage flow - incorporated review

2018037

tendrl-bug-id: Tendrl#841 bugzilla: 1583590 Signed-off-by: Anmol Sachan <anmol13694@gmail.com>

Changed Unmanage flow - incorporated review changes

1f609f4

tendrl-bug-id: Tendrl#841 bugzilla: 1583590 Signed-off-by: Anmol Sachan <anmol13694@gmail.com>

Incorporated review comments on service use

4f7e659

tendrl-bug-id: Tendrl#841 bugzilla: 1583590 Signed-off-by: Anmol Sachan <anmol13694@gmail.com>

anmolsachan force-pushed the master branch from a91cc10 to 4f7e659 Compare June 15, 2018 09:37

Merge branch 'master' into master

fa09e30

anmolsachan closed this Jul 3, 2018

		@@ -0,0 +1,55 @@
		""" This utility can be used to handle(start, stop, etc.) tendrl services.

Changed Unmanage flow - Unmanage cluster will happen per node and not via tendrl/monitor #975

Changed Unmanage flow - Unmanage cluster will happen per node and not via tendrl/monitor #975

Conversation

anmolsachan commented May 22, 2018 • edited Loading

centos-ci commented May 22, 2018

centos-ci commented May 22, 2018

anmolsachan commented May 23, 2018

anmolsachan commented May 24, 2018

anmolsachan commented May 24, 2018 • 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

anmolsachan May 25, 2018 • 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

anmolsachan commented May 28, 2018 • edited Loading

shtripat 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

anmolsachan commented May 30, 2018 • edited Loading

shtripat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolsachan commented May 31, 2018

r0h4n commented Jun 6, 2018

r0h4n commented Jun 13, 2018

shtripat 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

anmolsachan commented Jul 3, 2018

anmolsachan commented May 22, 2018 •

edited

Loading

anmolsachan commented May 24, 2018 •

edited

Loading

anmolsachan May 25, 2018 •

edited

Loading

anmolsachan commented May 28, 2018 •

edited

Loading

anmolsachan commented May 30, 2018 •

edited

Loading