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.

Sign up for GitHub

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

k8s,node: Reuse retrieveNodeInformation to retrieve node labels #11659

Merged
merged 2 commits into from May 26, 2020
Merged

k8s,node: Reuse retrieveNodeInformation to retrieve node labels #11659

merged 2 commits into from May 26, 2020

Conversation

pchaigno
Copy link
Member

When creating the host endpoint, we need to retrieve existing node labels. Instead of making a new API call, we can reuse retrieveNodeInformation() to get those labels.

@tgraf In #11543, you mentioned it would be best to reuse retrieveNodeInformation(). Is this what you had in mind?

Would it be worth adding these labels to CiliumNode as well?

@pchaigno pchaigno added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/misc This PR makes changes that have no direct user impact. labels May 22, 2020
@pchaigno pchaigno requested a review from tgraf May 22, 2020 20:47
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 22, 2020
@coveralls
Copy link

coveralls commented May 23, 2020 •
edited

Coverage Status

Coverage increased (+0.04%) to 36.906% when pulling b202364 on pr/pchaigno/reuse-retrieveNodeInformation-to-retrieve-labels into dfe8ba1 on master.

@pchaigno pchaigno force-pushed the pr/pchaigno/reuse-retrieveNodeInformation-to-retrieve-labels branch from 3b64732 to e3ac7a3 Compare May 23, 2020 06:15
@pchaigno pchaigno marked this pull request as ready for review May 23, 2020 08:57
@pchaigno pchaigno requested review from a team as code owners May 23, 2020 08:57
@pchaigno pchaigno requested a review from a team May 23, 2020 08:57
aanm
aanm approved these changes May 25, 2020
View reviewed changes
pkg/node/node_labels.go Outdated Show resolved Hide resolved
nebril
nebril approved these changes May 25, 2020
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

one question inline, looks good otherwise

pkg/endpointmanager/manager.go Show resolved Hide resolved
@pchaigno
node: Rename node_address_xxx files to address_xxx
85e67a5 
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/reuse-retrieveNodeInformation-to-retrieve-labels branch from e3ac7a3 to 80b6348 Compare May 26, 2020 08:02
@pchaigno
k8s,node: Reuse retrieveNodeInformation to retrieve node labels
b202364 
When creating the host endpoint, we need to retrieve existing node
labels. Instead of making a new API call, we can reuse
retrieveNodeInformation() to get those labels.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/reuse-retrieveNodeInformation-to-retrieve-labels branch from 80b6348 to b202364 Compare May 26, 2020 08:06
@pchaigno
Copy link
Member Author

test-me-please

@pchaigno
Copy link
Member Author

I think this is ready to merge. I filled #11698 for the test failing on K8s-1.11-Kernel-netnext. I don't see how it could be caused by this PR and it happened in other PRs as well according to the CI dashboard and logs. All other required builds are passing.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 26, 2020
@christarazi
Copy link
Member

Great, merging.

@christarazi christarazi merged commit a12c2ee into master May 26, 2020
1.8.0 automation moved this from In progress to Merged May 26, 2020
@christarazi christarazi deleted the pr/pchaigno/reuse-retrieveNodeInformation-to-retrieve-labels branch May 26, 2020 18:24
@pchaigno pchaigno mentioned this pull request Jun 1, 2020
Closed
34 tasks
@pchaigno pchaigno mentioned this pull request Jul 17, 2020
Merged
@pchaigno pchaigno added the area/host-firewall Impacts the host firewall or the host endpoint. label Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

@nebril nebril nebril approved these changes

@christarazi christarazi christarazi approved these changes

@tgraf tgraf Awaiting requested review from tgraf

Assignees
No one assigned
Labels
area/host-firewall Impacts the host firewall or the host endpoint. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pchaigno @coveralls @christarazi @nebril @aanm