Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.

Remove namespace from pod id local resource ID as it's redundant. #42

Merged
bmoyles0117 merged 1 commit into from
Dec 21, 2017
Merged

Remove namespace from pod id local resource ID as it's redundant. #42

bmoyles0117 merged 1 commit into from
Dec 21, 2017

Conversation

@bmoyles0117
Copy link
Contributor

@bmoyles0117 bmoyles0117 commented Dec 18, 2017

No description provided.

@bmoyles0117 bmoyles0117 requested review from davidbatelu, dhrupadb and igorpeshansky and removed request for davidbatelu December 18, 2017 17:10
dhrupadb
dhrupadb reviewed Dec 18, 2017
View reviewed changes
src/kubernetes.cc

const std::string k8s_pod_id = boost::algorithm::join(
std::vector<std::string>{kK8sPodResourcePrefix, namespace_name, pod_id},
std::vector<std::string>{kK8sPodResourcePrefix, pod_id},
Copy link
Contributor

@dhrupadb dhrupadb Dec 18, 2017

Choose a reason for hiding this comment

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

Does this change the query behaviour vis a vis. the other agents?

Copy link
Contributor Author

@bmoyles0117 bmoyles0117 Dec 18, 2017

Choose a reason for hiding this comment

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

In theory, yes, in practice, no, we've been querying by pod name, not ID. I don't believe anyone else has depended on this path in the past.

Copy link
Contributor

@dhrupadb dhrupadb Dec 18, 2017

Choose a reason for hiding this comment

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

SGTM. We're in "alpha" anyway so if we break people that's not disastrous. As long as our stuff works for now.

Copy link

@qingling128 qingling128 Dec 18, 2017

Choose a reason for hiding this comment

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

Won't this break logging in K8s because the current local_resource_id setup expects kK8sPodResourcePrefix + namespace_name + pod_id?

Copy link
Contributor Author

@bmoyles0117 bmoyles0117 Dec 21, 2017

Choose a reason for hiding this comment

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

I don't think we were ever using the pod ID, always the namespace, pod, and container names. If we were using it, I'm more than happy to make the right updates if you can show me an example.

Copy link
Contributor Author

@bmoyles0117 bmoyles0117 Dec 21, 2017

Choose a reason for hiding this comment

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

Actually, for the agents we're just using the k8s_container resource, I actually don't think anything was reporting against k8s_pod yet.

Copy link

@qingling128 qingling128 Dec 21, 2017

Choose a reason for hiding this comment

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

Ah, right. SGTM.

dhrupadb
dhrupadb approved these changes Dec 18, 2017
View reviewed changes
Copy link
Contributor

@dhrupadb dhrupadb left a comment

Choose a reason for hiding this comment

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

LGTM 👌

src/kubernetes.cc

const std::string k8s_pod_id = boost::algorithm::join(
std::vector<std::string>{kK8sPodResourcePrefix, namespace_name, pod_id},
std::vector<std::string>{kK8sPodResourcePrefix, pod_id},
Copy link
Contributor

@dhrupadb dhrupadb Dec 18, 2017

Choose a reason for hiding this comment

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

SGTM. We're in "alpha" anyway so if we break people that's not disastrous. As long as our stuff works for now.

@igorpeshansky igorpeshansky force-pushed the igorp-kubernetes-watch branch 8 times, most recently from a7ce50a to 2297cf8 Compare December 20, 2017 23:26
@igorpeshansky igorpeshansky changed the base branch from igorp-kubernetes-watch to master December 21, 2017 00:07
@igorpeshansky
Copy link
Contributor

igorpeshansky commented Dec 21, 2017

@bmoyles0117, we should rebase this off master.

@bmoyles0117 bmoyles0117 force-pushed the bmoyles0117-kubernetes-pod-id-local-resource branch from f32808d to be6ba55 Compare December 21, 2017 00:43
@bmoyles0117
Copy link
Contributor Author

bmoyles0117 commented Dec 21, 2017

Rebased, but not sure if I did it right. PTAL.

igorpeshansky
igorpeshansky suggested changes Dec 21, 2017
View reviewed changes
Copy link
Contributor

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

The rebase definitely looks wrong. This probably doesn't even build.
The changes should only be a few lines, right? You can undo the rebase (just git reset --hard to the branch head before the rebase — git reflog is your friend) and try again. If you can't get rebase to work properly, you could also rename your branch locally, create a new branch with the current name off master, and git cherry-pick the one relevant commit. @qingling128 can help you.

@bmoyles0117 bmoyles0117 force-pushed the bmoyles0117-kubernetes-pod-id-local-resource branch 2 times, most recently from 05c55be to f32808d Compare December 21, 2017 05:46
@bmoyles0117 bmoyles0117 force-pushed the bmoyles0117-kubernetes-pod-id-local-resource branch from f32808d to c9d1816 Compare December 21, 2017 05:50
@bmoyles0117 bmoyles0117 reopened this Dec 21, 2017
@bmoyles0117 bmoyles0117 force-pushed the bmoyles0117-kubernetes-pod-id-local-resource branch 2 times, most recently from aa50704 to 7288008 Compare December 21, 2017 06:08
@bmoyles0117
Copy link
Contributor Author

bmoyles0117 commented Dec 21, 2017 •
edited
Loading

I think I got closer, but I don't understand why the conflict is popping up. Any clues?

Just going to ask for Ling's help in the morning, I'm making a huge mess.

@bmoyles0117 bmoyles0117 force-pushed the bmoyles0117-kubernetes-pod-id-local-resource branch from 7288008 to f32808d Compare December 21, 2017 06:18
@igorpeshansky
Copy link
Contributor

igorpeshansky commented Dec 21, 2017

None of the other commits on your branch are in master — they were squashed. The code is there, but git doesn't know that yet.
In the rebase, you should drop all of the commits except f32808d, which would then be reapplied on top of the code in master.

@bmoyles0117 bmoyles0117 force-pushed the bmoyles0117-kubernetes-pod-id-local-resource branch from f32808d to 8314f9e Compare December 21, 2017 15:18
@bmoyles0117
Copy link
Contributor Author

bmoyles0117 commented Dec 21, 2017

Ok, I got it back to the state I had it before, but the kubernetes.cc conflict still puzzles me.

@igorpeshansky
Copy link
Contributor

igorpeshansky commented Dec 21, 2017

Now you can just git rebase master (no -i) and resolve the conflict. I suspect you just need to git pull the latest master.

@bmoyles0117 bmoyles0117 force-pushed the bmoyles0117-kubernetes-pod-id-local-resource branch from 8314f9e to 5681abb Compare December 21, 2017 15:53
@bmoyles0117
Copy link
Contributor Author

bmoyles0117 commented Dec 21, 2017

Wow, that's all it was? Alright, all up to date, PTAL.

@igorpeshansky
Copy link
Contributor

igorpeshansky commented Dec 21, 2017

(Hint for the future: you can also rebase against origin/master — just remember to do a git fetch first).

igorpeshansky
igorpeshansky approved these changes Dec 21, 2017
View reviewed changes
Copy link
Contributor

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

qingling128
qingling128 approved these changes Dec 21, 2017
View reviewed changes
Copy link

@qingling128 qingling128 left a comment

Choose a reason for hiding this comment

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

LGTM.

src/kubernetes.cc

const std::string k8s_pod_id = boost::algorithm::join(
std::vector<std::string>{kK8sPodResourcePrefix, namespace_name, pod_id},
std::vector<std::string>{kK8sPodResourcePrefix, pod_id},
Copy link

@qingling128 qingling128 Dec 21, 2017

Choose a reason for hiding this comment

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

Ah, right. SGTM.

@bmoyles0117 bmoyles0117 merged commit 27fb587 into master Dec 21, 2017
@bmoyles0117 bmoyles0117 deleted the bmoyles0117-kubernetes-pod-id-local-resource branch December 21, 2017 20:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@dhrupadb dhrupadb dhrupadb approved these changes

@qingling128 qingling128 qingling128 approved these changes

@igorpeshansky igorpeshansky igorpeshansky approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bmoyles0117 @igorpeshansky @dhrupadb @qingling128