Skip to content

Conversation

calmkart
Copy link
Contributor

@calmkart calmkart commented Oct 11, 2019

as this issue #68
add a command to retain pod labels in fork mode.
more details in readme.
@aylei PTAL, thanks!

Copy link
Owner

@aylei aylei left a comment

Choose a reason for hiding this comment

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

I take a glance of the code and it looks good to me, will review it more carefully tomorrow.

Copy link
Owner

@aylei aylei left a comment

Choose a reason for hiding this comment

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

Rest LGTM

docs/zh-cn.md Outdated
# configMap的data应为labels键值对
# 使用的configMap名通过 --pod-labels-configmap-name 设置,默认为kubectl-debug-pod-labels-configmap
# 使用的configMap命名空间通过 --pod-labels-configmap-namespace 设置,默认为default
kubectl debug POD_NAME --fork --pod-labels-configmap-name <k8s_configmap_name> --pod-labels-configmap-namespace <namespace>
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think ConfigMap is a proper solution for this issue, because the requirement of custom labels is various from Pod to Pod. Consider the situation in #68 , the pod identity is unique across pods, it would be better if user can specify the labels in the command line directly.

Copy link
Contributor Author

@calmkart calmkart Oct 16, 2019

Choose a reason for hiding this comment

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

of cause custom labels is various from Pod to Pod, so the configmap name is diffrent for every pod.
if i want to debug pod podA, i can create a configmap named podALabelsConfigmap, and use it.
if i want to debug pod podB, i can create a configmap named podBLbalesConfigmap, and use it.

use the configmap we can save the debug-lables-values: pod key: value. EX:
i want to debug podA, i create a configmap called podALabelsConfigmap. 1 hour after, i want to debug podB, i create a configmap called podBLebelsConfigmap. 1 hour after, there is a reason to debug podA again. i can use the existing configmap podALablesConfigmap.

even though we needn't save the labels configmap to use after, we can edit the default labels configmap debug-pod-lables-configmap to debug another pod. if we didn't need labels, we can delete the configmap.

in my opinion, use command line to set labels is complicated and hard to see.(and can't save the label state for each pod)
if we use configmap, it's very easy and clear.(and can save the label state)

Copy link
Owner

Choose a reason for hiding this comment

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

Good point. I agree that a default set of labels will be helpful.

Setting labels from command line is definitely verbose, but referencing labels from a configmap is even more so because the users are forced to manage the lifecycle of the configmaps created for label reference.

IMHO, we don't have to support arbitrary labeling because the kubectl label sub-command has already did the job. Instead, considering the user scenario, it makes more sense to allow user retaining some labels from the original Pod. e.g.

kubectl debug my-pod --retain-labels pod-identity,app

The above command may copy the pod-identity and app labels to the forked Pod so that the new Pod could act the same as the original Pod if there's any busy logic related to specific labels.

As of default labels for forked Pod, it could be considered as a separate problem, which could help user and other components distinguish the forked Pod from normal Pods. We could file a separate issue for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right
i'll fix it in next week

@calmkart calmkart changed the title ADD: add the pod labels configmap in fork mode ADD: add the pod labels retain config in fork mode Oct 21, 2019
@calmkart
Copy link
Contributor Author

fixed, PTAL
@aylei ,thanks!

Copy link
Owner

@aylei aylei left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@aylei aylei merged commit 63e1ecc into aylei:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants