Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mgr/rook: Blinking lights #33366

Merged
merged 1 commit into from Mar 26, 2020
Merged

mgr/rook: Blinking lights #33366

merged 1 commit into from Mar 26, 2020

Conversation

jmolmo
Copy link
Member

@jmolmo jmolmo commented Feb 17, 2020

Blinking lights implementation

In my "dev" environment ( only vms) I have tested the new rook_cluster.blink_light method calling it manually with several different "locs" parameter.
But I need to have baremetal machines where this can be tested properly and switching on/off the disks identification lights can be verified.

My aim now is to make a integration test of this functionality to run in the Rook CI. But in parallel i will try to locate and to verify this on a real cluster.

Signed-off-by: Juan Miguel Olmo Martínez jolmomar@redhat.com

src/pybind/mgr/rook/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
@sebastian-philipp
Copy link
Contributor

mypy run-test-pre: PYTHONHASHSEED='685483167'
mypy run-test: commands[0] | mypy --config-file=../../mypy.ini cephadm/module.py mgr_module.py mgr_util.py orchestrator/__init__.py progress/module.py rook/module.py test_orchestrator/module.py
rook/module.py:34: note: In module imported here:
rook/rook_cluster.py: note: In member "_fetch" of class "KubernetesResource":
rook/rook_cluster.py:110: error: Name 'V1ListMeta' is not defined
rook/rook_cluster.py: note: In function "describe_pods":
rook/rook_cluster.py:288: error: Name 'V1Pod' is not defined
rook/rook_cluster.py: note: In member "blink_light" of class "RookCluster":
rook/rook_cluster.py:578: error: Name 'DeviceLightLoc' is not defined
rook/rook_cluster.py:578: error: Name 'string' is not defined
rook/rook_cluster.py:592: error: "List[Any]" has no attribute "path"
rook/rook_cluster.py:592: error: "List[Any]" has no attribute "dev"
rook/rook_cluster.py:596: error: "List[Any]" has no attribute "host"
rook/rook_cluster.py:632: error: "List[Any]" has no attribute "host"
rook/rook_cluster.py:632: error: "List[Any]" has no attribute "path"
rook/rook_cluster.py:632: error: "List[Any]" has no attribute "dev"
rook/module.py: note: In member "blink_device_light" of class "RookOrchestrator":
rook/module.py:452: error: Name 'DeviceLightLoc' is not defined
rook/module.py:458: error: "List[Any]" has no attribute "dev"
Found 12 errors in 2 files (checked 7 source files)

src/pybind/mgr/rook/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
@sebastian-philipp
Copy link
Contributor

you can run mypy locally as well:

cd /src/pybind/mgr ; ../../script/run_tox.sh --tox-envs=py3,mypy mgr

@jmolmo
Copy link
Member Author

jmolmo commented Feb 20, 2020

you can run mypy locally as well:

cd /src/pybind/mgr ; ../../script/run_tox.sh --tox-envs=py3,mypy mgr

Thx!.
I have seen that i can also execute mypy directly over the required source files.

src/pybind/mgr/rook/rook_cluster.py Show resolved Hide resolved
src/pybind/mgr/rook/rook_cluster.py Show resolved Hide resolved
@jmolmo
Copy link
Member Author

jmolmo commented Feb 20, 2020

I think that not all the types can be annotated properly.. or i do not know how to do that....if not is possible to reference "alien" types with mypy ... is a pity, because this implies to lose interesting comments about types that we had before "mypy" and the awesome PEP 484 has been applied as a new code purity check. :-)

@sebastian-philipp
Copy link
Contributor

I think that not all the types can be annotated properly..

Sure, but I'm a bit 👎 for artificially reducing the tesing coverage. Those two type annotations are checked by mypy already! Removing them seems a bit odd to me.

@jmolmo
Copy link
Member Author

jmolmo commented Feb 20, 2020

I think that not all the types can be annotated properly..

Sure, but I'm a bit for artificially reducing the tesing coverage. Those two type annotations are checked by mypy already! Removing them seems a bit odd to me.

If i do not remove these two type annotations I have:

[jolmomar@juanmipc mgr]$ mypy --config-file=../../mypy.ini  rook/module.py  rook/rook_cluster.py
rook/rook_cluster.py: note: In member "_fetch" of class "KubernetesResource":
rook/rook_cluster.py:110: error: Name 'V1ListMeta' is not defined
rook/rook_cluster.py: note: In function "describe_pods":
rook/rook_cluster.py:288: error: Name 'V1Pod' is not defined
Found 2 errors in 1 file (checked 2 source files)

Do you know how to tell mypy about the nature of V1Pod and V1ListMeta?

@sebastian-philipp
Copy link
Contributor

I think that not all the types can be annotated properly..

Sure, but I'm a bit for artificially reducing the tesing coverage. Those two type annotations are checked by mypy already! Removing them seems a bit odd to me.

If i do not remove these two type annotations I have:

[jolmomar@juanmipc mgr]$ mypy --config-file=../../mypy.ini  rook/module.py  rook/rook_cluster.py
rook/rook_cluster.py: note: In member "_fetch" of class "KubernetesResource":
rook/rook_cluster.py:110: error: Name 'V1ListMeta' is not defined
rook/rook_cluster.py: note: In function "describe_pods":
rook/rook_cluster.py:288: error: Name 'V1Pod' is not defined
Found 2 errors in 1 file (checked 2 source files)

Do you know how to tell mypy about the nature of V1Pod and V1ListMeta?

Yes! This is the import statement you removed! https://github.com/ceph/ceph/pull/33366/files#diff-45db35bc78bc3027f9421bd371b19e9bL31

@jmolmo
Copy link
Member Author

jmolmo commented Feb 20, 2020

I think that not all the types can be annotated properly..

Sure, but I'm a bit for artificially reducing the tesing coverage. Those two type annotations are checked by mypy already! Removing them seems a bit odd to me.

If i do not remove these two type annotations I have:

[jolmomar@juanmipc mgr]$ mypy --config-file=../../mypy.ini  rook/module.py  rook/rook_cluster.py
rook/rook_cluster.py: note: In member "_fetch" of class "KubernetesResource":
rook/rook_cluster.py:110: error: Name 'V1ListMeta' is not defined
rook/rook_cluster.py: note: In function "describe_pods":
rook/rook_cluster.py:288: error: Name 'V1Pod' is not defined
Found 2 errors in 1 file (checked 2 source files)

Do you know how to tell mypy about the nature of V1Pod and V1ListMeta?

Yes! This is the import statement you removed! https://github.com/ceph/ceph/pull/33366/files#diff-45db35bc78bc3027f9421bd371b19e9bL31

ahhhh!!!. Sorry ... fixed!

src/pybind/mgr/rook/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
@jmolmo
Copy link
Member Author

jmolmo commented Feb 20, 2020

The import has been replaced by "from kubernetes import client"

@sebastian-philipp
Copy link
Contributor

I'd like to push to your repo. could you enable this flag? https://github.blog/2016-09-07-improving-collaboration-with-forks/

@jmolmo
Copy link
Member Author

jmolmo commented Feb 21, 2020

I'd like to push to your repo. could you enable this flag? https://github.blog/2016-09-07-improving-collaboration-with-forks/

It was already enabled.

@sebastian-philipp
Copy link
Contributor

ping? could you please apply

From ff90c8335658c6668bf858dc1a555b4a56ab9d21 Mon Sep 17 00:00:00 2001
From: Sebastian Wagner <sebastian.wagner@suse.com>
Date: Fri, 21 Feb 2020 12:41:42 +0100
Subject: [PATCH] mgr/rook: Add type annotations for blink_light

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
---
 src/pybind/mgr/rook/module.py       | 1 +
 src/pybind/mgr/rook/rook_cluster.py | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/pybind/mgr/rook/module.py b/src/pybind/mgr/rook/module.py
index 098ef66280..8364873ad2 100644
--- a/src/pybind/mgr/rook/module.py
+++ b/src/pybind/mgr/rook/module.py
@@ -447,6 +447,7 @@ class RookOrchestrator(MgrModule, orchestrator.Orchestrator):
         return c
 
     def blink_device_light(self, ident_fault, on, locs):
+        # type: (str, bool, List[orchestrator.DeviceLightLoc]) -> RookCompletion
         return write_completion(
             on_complete=lambda: self.rook_cluster.blink_light(
                 ident_fault, on, locs),
diff --git a/src/pybind/mgr/rook/rook_cluster.py b/src/pybind/mgr/rook/rook_cluster.py
index c08c6aefd7..06ffabcbc6 100644
--- a/src/pybind/mgr/rook/rook_cluster.py
+++ b/src/pybind/mgr/rook/rook_cluster.py
@@ -575,7 +575,7 @@ class RookCluster(object):
         except ApiException as e:
             log.error("Error getting ceph image: {}".format(e))
 
-    def blink_light(self, ident_fault, on, locs):
+    def blink_light(self, ident_fault: str, on: bool, locs: List[orchestrator.DeviceLightLoc]):
         operation_id = str(hash(locs))
         message = ""
 
-- 
2.17.1

@jmolmo
Copy link
Member Author

jmolmo commented Feb 25, 2020

We are using in this PR Python 3 type annotations in the functions, but all the others type annotations in other functions are Python 2.
I think that probably we should change all of them to Python3... in a different PR only for that. What do you think @sebastian-philipp ?

src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
@tchaikov
Copy link
Contributor

tchaikov commented Mar 6, 2020

@jmolmo ping?

@jmolmo
Copy link
Member Author

jmolmo commented Mar 13, 2020

Tested in a real baremetal cluster:

Execution example:


[root@rook-ceph-tools-7f96779fb9-xkpd4 /]# ceph device ls
DEVICE                               HOST:DEV   DAEMONS  LIFE EXPECTANCY
WDC_WDS200T2B0A-00SM50_183503800976  cpach:sdo  osd.0 

[root@rook-ceph-tools-7f96779fb9-xkpd4 /]# ceph device light on WDC_WDS200T2B0A-00SM50_183503800976
['']
[root@rook-ceph-tools-7f96779fb9-xkpd4 /]# ceph device light off WDC_WDS200T2B0A-00SM50_183503800976
['']

In case of errors:


[root@rook-ceph-tools-7f96779fb9-xkpd4 /]# ceph device light on WDC_WDS200T2B0A-00SM50_183503800976
_exception: (422)
Reason: Unprocessable Entity
HTTP response headers: HTTPHeaderDict({'Content-Type': 'application/json', 'Date': 'Fri, 13 Mar 2020 16:36:44 GMT', 'Content-Length': '447'})
HTTP response body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Job.batch \"3822060501990855105\" is invalid: spec.template.spec.containers[0].volumeMounts[1].name: Not found: \"runudev\"","reason":"Invalid","details":{"name":"3822060501990855105","group":"batch","kind":"Job","causes":[{"reason":"FieldValueNotFound","message":"Not found: \"runudev\"","field":"spec.template.spec.containers[0].volumeMounts[1].name"}]},"code":422}


[root@rook-ceph-tools-7f96779fb9-xkpd4 /]# ceph device light on WDC_WDS200T2B0A-00SM50_183503800976
['LIB_BUG(1): BUG: Failed to open /dev/disk/by-path/pci-0000:82:00.0-sas-exp0x50030480091072bf-phy31-lun-0, error: 1, Operation not permitted \n']

@jmolmo
Copy link
Member Author

jmolmo commented Mar 13, 2020

In order to work this modification is needed to add new rights to the ceph manager module in rook:

See: Ceph: Added rook manager module rights for read pod logs

@sebastian-philipp
Copy link
Contributor

rook/module.py:35: note: In module imported here:
rook/rook_cluster.py: note: In member "remove_pods" of class "RookCluster":
rook/rook_cluster.py:362: error: "RookCluster" has no attribute "k8s"
rook/rook_cluster.py:365: error: Name 'V1DeleteOptions' is not defined

src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
src/pybind/mgr/rook/rook_cluster.py Outdated Show resolved Hide resolved
@tchaikov
Copy link
Contributor

@jmolmo needs rebase

@tchaikov
Copy link
Contributor

@jmolmo could remove the "merge" commit in this PR?

Blinking lights implementation

Signed-off-by: Juan Miguel Olmo Martínez <jolmomar@redhat.com>
@tchaikov tchaikov merged commit 633a831 into ceph:master Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants