Skip to content

k8s: keep CiliumNode labels synced with Node object#18609

Merged
qmonnet merged 2 commits intomasterfrom
pr/jibi/sync-node-labels-to-ciliumnode-obj
Mar 29, 2022
Merged

k8s: keep CiliumNode labels synced with Node object#18609
qmonnet merged 2 commits intomasterfrom
pr/jibi/sync-node-labels-to-ciliumnode-obj

Conversation

@jibi
Copy link
Copy Markdown
Member

@jibi jibi commented Jan 25, 2022

See individual commits

@jibi jibi added area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/agent Cilium agent related. labels Jan 25, 2022
@jibi jibi requested review from a team, aanm and ldelossa January 25, 2022 13:09
@jibi jibi mentioned this pull request Jan 25, 2022
Open
@jibi jibi force-pushed the pr/jibi/sync-node-labels-to-ciliumnode-obj branch 2 times, most recently from 77f1c8d to 089ad2e Compare January 25, 2022 14:03
@maintainer-s-little-helper maintainer-s-little-helper Bot mentioned this pull request Jan 25, 2022
Closed
aanm
aanm approved these changes Jan 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@aanm aanm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just 2 small nits. LGTM, doesn't require my 2nd review. However, it would be great if we could have some e2e tests that covers this.

Comment thread daemon/cmd/daemon.go Outdated
Comment thread pkg/k8s/watchers/cilium_node.go Outdated
@jibi jibi marked this pull request as draft January 26, 2022 14:55
@jibi jibi force-pushed the pr/jibi/sync-node-labels-to-ciliumnode-obj branch 2 times, most recently from ea8306e to 9e110a1 Compare February 1, 2022 15:51
@jibi jibi marked this pull request as ready for review February 1, 2022 15:51
@jibi jibi requested a review from a team as a code owner February 1, 2022 15:51
@jibi jibi requested a review from nebril February 1, 2022 15:51
@jibi jibi requested a review from aanm February 1, 2022 15:52
jibi
jibi commented Feb 1, 2022
View reviewed changes
Comment thread daemon/cmd/daemon.go Outdated
@jibi jibi force-pushed the pr/jibi/sync-node-labels-to-ciliumnode-obj branch from 9e110a1 to 1972cf0 Compare February 4, 2022 13:16
aanm
aanm requested changes Feb 7, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

Discussed offline, there are some necessary changes to handle the kvstore mode node update

@jibi jibi force-pushed the pr/jibi/sync-node-labels-to-ciliumnode-obj branch 2 times, most recently from f238790 to 3395ac3 Compare March 2, 2022 13:01
@jibi jibi marked this pull request as draft March 2, 2022 13:07
@jibi jibi force-pushed the pr/jibi/sync-node-labels-to-ciliumnode-obj branch from 3395ac3 to 7b81077 Compare March 2, 2022 16:42
@jibi jibi force-pushed the pr/jibi/sync-node-labels-to-ciliumnode-obj branch 3 times, most recently from 9b46e4c to 8f21dde Compare March 4, 2022 18:02
@jibi
Copy link
Copy Markdown
Member Author

jibi commented Mar 4, 2022

/test-netnext

@aanm
Copy link
Copy Markdown
Member

aanm commented Mar 5, 2022

/test-1.23-net-next

1 similar comment
@aanm
Copy link
Copy Markdown
Member

aanm commented Mar 5, 2022

/test-1.23-net-next

@jibi jibi force-pushed the pr/jibi/sync-node-labels-to-ciliumnode-obj branch 2 times, most recently from 46e69d1 to 33c9ed6 Compare March 27, 2022 09:12
@jibi jibi closed this Mar 27, 2022
@jibi jibi reopened this Mar 27, 2022
@jibi
Copy link
Copy Markdown
Member Author

jibi commented Mar 27, 2022

/test

@jibi jibi force-pushed the pr/jibi/sync-node-labels-to-ciliumnode-obj branch from 33c9ed6 to 44649da Compare March 27, 2022 10:20
qmonnet
qmonnet approved these changes Mar 28, 2022
View reviewed changes
Comment thread test/k8s/Node.go Outdated
@jibi
Copy link
Copy Markdown
Member Author

jibi commented Mar 28, 2022

/test

aanm
aanm requested changes Mar 28, 2022
View reviewed changes
Comment thread pkg/k8s/watchers/node.go Outdated
Comment on lines 54 to 57
Copy link
Copy Markdown
Member

@aanm aanm Mar 28, 2022

Choose a reason for hiding this comment

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

If the title says we won't do anything for addresses then we can drop this?

Copy link
Copy Markdown
Member Author

@jibi jibi Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Correct, this is not needed now, I'll drop it 👍
(but I'll still keep this function as I'll probably need the annotations check for the KV store case)

jibi added 2 commits March 28, 2022 23:17
@jibi
k8s: keep CiliumNode labels synced with Node object
8e1c465 
This commit ensures that any update to the labels of a k8s Node object
is reflected in the corresponding CiliumNode one.

Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>
@jibi
watchers: don't ignore CiliumNode events if labels get updated
7a1c7ea 
Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>
@jibi
Copy link
Copy Markdown
Member Author

jibi commented Mar 28, 2022

/test

@qmonnet qmonnet mentioned this pull request Mar 30, 2022
Merged
18 tasks
@christarazi christarazi mentioned this pull request Apr 1, 2022
Merged
@joestringer joestringer mentioned this pull request Apr 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ldelossa ldelossa ldelossa left review comments

@qmonnet qmonnet qmonnet approved these changes

@gandro gandro gandro approved these changes

@nebril nebril nebril approved these changes

@christarazi christarazi christarazi approved these changes

@aanm aanm aanm approved these changes

Assignees

No one assigned

Labels

area/agent Cilium agent related. area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jibi @ldelossa @aanm @gandro @nebril @christarazi @qmonnet @joestringer @pchaigno