Skip to content

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

Merged
pchaigno merged 1 commit intocilium:masterfrom
christarazi:pr/christarazi/v1-ep-slice-missing
Nov 5, 2021
Merged

k8s/watchers: Add missing v1 EndpointSlice group on init#17778
pchaigno merged 1 commit intocilium:masterfrom
christarazi:pr/christarazi/v1-ep-slice-missing

Conversation

@christarazi
Copy link
Copy Markdown
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. area/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
Copy Markdown
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).

Comment thread pkg/k8s/watchers/watcher.go Outdated
Copy link
Copy Markdown
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
Copy Markdown
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 christarazi force-pushed the pr/christarazi/v1-ep-slice-missing branch from e907017 to da0fba1 Compare November 4, 2021 19:46
@christarazi christarazi requested a review from aanm November 4, 2021 19:50
@christarazi
Copy link
Copy Markdown
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
Copy Markdown
Member Author

/test

@pchaigno
Copy link
Copy Markdown
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

area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants