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

Jump to bottom

mgr/rook: Fix container id and image id in 'orch ps' #38106

Merged
merged 3 commits into from Dec 8, 2020
Merged

mgr/rook: Fix container id and image id in 'orch ps' #38106

merged 3 commits into from Dec 8, 2020

Conversation

varshar16
Copy link
Contributor

Fixes: https://tracker.ceph.com/issues/47513
Signed-off-by: Varsha Rao varao@redhat.com

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@varshar16 varshar16 requested a review from a team as a code owner November 16, 2020 11:43
travisn
travisn reviewed Nov 16, 2020
View reviewed changes
src/pybind/mgr/rook/module.py Outdated
@@ -374,7 +374,7 @@ def _list_daemons(self, service_name=None, daemon_type=None, daemon_id=None, hos
for p in pods:
sd = orchestrator.DaemonDescription()
sd.hostname = p['hostname']
sd.container_id = p['name']
sd.container_id = p['container_id'][0:12]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the container_id and container_image_id used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are just displayed in output of 'orch ps' and 'orch ls' commands.

# ceph orch ps
NAME   HOST      STATUS         REFRESHED  AGE  VERSION    IMAGE NAME                           IMAGE ID      CONTAINER ID          
mgr.a  minikube  Running (89s)  0s ago     89s  <unknown>  192.168.0.138:5000/ceph/ceph:latest  cc82ea0cb1ef  docker://c5555d21c3c  
mon.a  minikube  Running (79s)  0s ago     79s  <unknown>  192.168.0.138:5000/ceph/ceph:latest  cc82ea0cb1ef  docker://7f689c1f1a4  
osd.0  minikube  Running (72m)  0s ago     72m  <unknown>  192.168.0.138:5000/ceph/ceph:latest  106dcc228585  docker://ce7fce2fb92  

# ceph orch ls
NAME  RUNNING  REFRESHED  AGE  PLACEMENT  IMAGE NAME                           IMAGE ID      
mgr       1/1  0s ago     98s  count:1    192.168.0.138:5000/ceph/ceph:latest  cc82ea0cb1ef  
mon       1/1  0s ago     88s  count:1    192.168.0.138:5000/ceph/ceph:latest  cc82ea0cb1ef

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question is: it it worth it to display the container id? The image id is interesting to know which ceph container is being used. but why bothering with the container id?

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 don't think so. I kept container id just to maintain consistent interface. Do you plan to remove it completely from the orchestrator module ?

src/pybind/mgr/rook/rook_cluster.py Outdated
@@ -335,6 +335,8 @@ def predicate(item):
"labels": d['metadata']['labels'],
'phase': d['status']['phase'],
'container_image_name': image_name,
'container_image_id': d['status']['container_statuses'][0]['image_id'].split('@sha256:')[1],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems awkward to extract these details. Are the @sha256 and docker:// 100% reliable in all types of K8s clusters? Maybe they are, I'm just not sure. Even better if we don't actually need to use them anywhere, from the question above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not. I have added additional check for sha256 and removed docker:// as it is not reliable.

@varshar16 varshar16 force-pushed the wip-fix-rook-orch-ps-img-id-name branch from 1607bc5 to 12d5675 Compare November 17, 2020 11:59
@tchaikov
Copy link
Contributor

@sebastian-philipp @travisn is this change good to merge? i cannot find any test suite exercising the rook mgr plugin.

@varshar16
mgr/rook: Fix rook_cluster label value
4daad2a 
rook_cluster label value is cluster namespace instead of cluster name.

Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
mgr/rook: Fix image id in 'orch ps'
f84d5df 
Fixes: https://tracker.ceph.com/issues/47513
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
mgr/rook: Remove container_id in 'orch ps'
e6b85ee 
As container id is not useful.

Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16 varshar16 force-pushed the wip-fix-rook-orch-ps-img-id-name branch from 12d5675 to e6b85ee Compare November 27, 2020 09:38
@varshar16
Copy link
Contributor Author

I have removed container id.

@varshar16
Copy link
Contributor Author

@sebastian-philipp @travisn is this change good to merge? i cannot find any test suite exercising the rook mgr plugin.

Currently there are no tests for rook.

travisn
travisn reviewed Nov 30, 2020
View reviewed changes
src/pybind/mgr/rook/rook_cluster.py
@@ -329,12 +330,16 @@ def predicate(item):
image_name = c['image']
break

image_id = d['status']['container_statuses'][0]['image_id']
image_id = image_id.split(prefix)[1] if prefix in image_id else image_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the sha256: prefix? If this is just for display, maybe the user wants to differentiate between the sha256, docker, and other prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travisn I removed it, to keep it consistent with cephadm. You want display the entire image id docker.io/ceph/daemon-base@sha256:21be74b3068 ?

@sebastian-philipp what do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for showing the real value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency with cephadm sounds good.

travisn
travisn approved these changes Dec 7, 2020
View reviewed changes
src/pybind/mgr/rook/rook_cluster.py
@@ -329,12 +330,16 @@ def predicate(item):
image_name = c['image']
break

image_id = d['status']['container_statuses'][0]['image_id']
image_id = image_id.split(prefix)[1] if prefix in image_id else image_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency with cephadm sounds good.

@sebastian-philipp
Copy link
Contributor

https://pulpito.ceph.com/swagner-2020-12-07_12:36:07-rados:cephadm-wip-swagner2-testing-2020-12-07-1137-distro-basic-smithi/

failurs:

@sebastian-philipp sebastian-philipp merged commit 712a640 into ceph:master Dec 8, 2020
@varshar16 varshar16 deleted the wip-fix-rook-orch-ps-img-id-name branch December 8, 2020 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebastian-philipp sebastian-philipp sebastian-philipp approved these changes

@travisn travisn travisn approved these changes

Assignees
No one assigned
Labels
bug-fix needs-review orchestrator pybind rook wip-swagner2-testing
Projects
None yet
Milestone
No milestone
4 participants
@varshar16 @tchaikov @sebastian-philipp @travisn