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

k8s/watchers: Add missing v1 EndpointSlice group on init #17778

Merged

Conversation

christarazi
Copy link
Member

The K8sWatcher's InitK8sSubsystem() calls resourceGroups() which returns
a list of resources that should be waited on via WaitForCacheSync(). The
v1 version of EndpointSlice was missing from this list. This commit adds
it.

Signed-off-by: Chris Tarazi chris@isovalent.com

@christarazi christarazi requested a review from a team November 4, 2021 00:18
@christarazi christarazi added 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. labels Nov 4, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 4, 2021
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

nice catch, looks like this was introduced by one of my refactoring PRs during v1.11 development cycle. v1.10 looks fine in this regard.

Curious also how you noticed this, since CI doesn't appear to catch this condition? (at least not in a reliable way; for all I know this was causing a flake but I don't know).

pkg/k8s/watchers/watcher.go Outdated Show resolved Hide resolved
Copy link
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

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

good on 🍏

@christarazi
Copy link
Member Author

@joestringer My guess for why we don't see it in CI is because we probably don't disable the original Endpoints resource, so even if the v1.EndpointSlice group is missing from the wait, then we still have to wait on the former. I imagine that if Cilium ran in a cluster with the former completely disabled, then we would have seen some flakes at the startup.

I found it through regular code inspection while working on #17714.

@christarazi
Copy link
Member Author

christarazi commented Nov 4, 2021

/test

Job 'Cilium-PR-K8s-1.20-kernel-5.4' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Iptables Skip conntrack for pod traffic

Failure Output

FAIL: Missing '-j CT --notrack' iptables rule

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.20-kernel-5.4 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.21-kernel-4.19' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Iptables Skip conntrack for pod traffic

Failure Output

FAIL: Missing '-j CT --notrack' iptables rule

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.19 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Iptables Skip conntrack for pod traffic

Failure Output

FAIL: Missing '-j CT --notrack' iptables rule

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.16-net-next so I can create a new GitHub issue to track it.

The K8sWatcher's InitK8sSubsystem() calls resourceGroups() which returns
a list of resources that should be waited on via WaitForCacheSync(). The
v1 version of EndpointSlice was missing from this list. This commit adds
it.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/v1-ep-slice-missing branch from da0fba1 to 527a598 Compare November 5, 2021 00:13
@christarazi
Copy link
Member Author

/test

@pchaigno
Copy link
Member

pchaigno commented Nov 5, 2021

Reviews are in. Multicluster has the usual failure and k8s-1.21-kernel-4.19 hit known flake #13552. Marking ready to merge and merging.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 5, 2021
@pchaigno pchaigno merged commit 832246b into cilium:master Nov 5, 2021
@christarazi christarazi deleted the pr/christarazi/v1-ep-slice-missing branch November 5, 2021 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants