Handle `.status.conditions` on `Service`s using in accordance with KEP-1623 by addreas · Pull Request #27399 · cilium/cilium

addreas · 2023-08-09T19:42:22Z

Currently the lbipam controller creates multiple .status.conditions per type of condition on the Services it updates. This conflicts with what's expected, as the ServiceStatus.Conditions is defined as a // +listType=map and // +listMapKey=type.

While the Kubernetes API server seems to not notice, tooling that utilizes strategic merge patches on the in-cluster resources will encounter errors such as error: failed to create manager for existing fields: failed to convert new object (network-cluster/nginx-ingress-controller; /v1, Kind=Service) to smd typed: .status.conditions: duplicate entries for key [type="io.cilium/lb-ipam-request-satisfied"].

Not quite sure if copy-pasting the required code for github.com/cilium/cilium/pkg/k8s/slim/k8s/apimachinery/pkg/api/meta from the apimachinery master branch and modifying the header is the correct way of doing things. Is that supposed to follow some specific version or be tracked somehow?

Haven't dug into building an image locally for testing, but at least go test ./operator/pkg/lbipam passes.

Fixes: #24945

ghost

docs structure looks good

ghost · 2023-08-10T08:48:43Z

/test

joestringer · 2023-08-25T18:54:11Z

Thanks for the PR! CI was passing before. Next step is getting @cilium/sig-k8s review, then once all reviewers are addressed and green we can do a rebase + re-run CI to confirm there's no breakage on tip of main branch. It'll also be worth evaluating whether we should backport this fix to v1.14 assuming it satisfies the backport criteria.

christarazi

Thanks for the PR!

Not quite sure if copy-pasting the required code for github.com/cilium/cilium/pkg/k8s/slim/k8s/apimachinery/pkg/api/meta from the apimachinery master branch and modifying the header is the correct way of doing things. Is that supposed to follow some specific version or be tracked somehow?

Please follow https://github.com/cilium/cilium/blob/main/pkg/k8s/slim/README.md and update it if the files you need in this PR aren't inside this script.

addreas · 2023-08-28T10:34:17Z

Updated the slim README with another curl line for the meta conditions utils. Changed the package path to line up more closely with the others, though I'm not sure I can actually see the pattern there.

…ties This changes the behaviour such that any one type of condition will only appear once in the .status.conditions list. Fixes: cilium#24945 Signed-off-by: Andreas Mårtensson <andreas@addem.se>

Signed-off-by: Andreas Mårtensson <andreas@addem.se>

christarazi · 2023-08-28T17:04:14Z

LGTM. CCing @aanm to double check the process for slim-related changes.

aanm

slim-related changes LGTM

joamaki · 2023-08-29T14:47:28Z

/test

Edit: ci-ginkgo suite failed on one case due to some cancellation of a step, re-running

As the previous commit mentions, this does not occur on main, v1.16, and v1.15 due to cilium#29036. However, in v1.14 and v1.13, we need to take special care because the NameManager and SelectorCache lock can be taken while the Endpoint lock is held during Endpoint deletion. Here are the relevant stacktraces regarding the deadlock: ``` 1: sync.Mutex.Lock [75 minutes] [Created by http.(*Server).Serve in goroutine 1699 @ server.go:3086] sync sema.go:77 runtime_SemacquireMutex(*uint32(0x5), false, 43690) sync mutex.go:171 (*Mutex).lockSlow(*Mutex(cilium#1733)) sync mutex.go:90 (*Mutex).Lock(...) sync rwmutex.go:147 (*RWMutex).Lock(*RWMutex(0xb0)) fqdn name_manager.go:70 (*NameManager).Lock(0xffffffffffffffff) policy selectorcache.go:964 (*SelectorCache).RemoveSelectors(cilium#1088, {cilium#28569, 0xb, 1}, {cilium#643, cilium#32582}) policy l4.go:810 (*L4Filter).removeSelectors(cilium#32582, cilium#29992) policy l4.go:817 (*L4Filter).detach(cilium#719, cilium#29993) policy l4.go:988 L4PolicyMap.Detach(...) policy l4.go:1179 (*L4Policy).Detach(cilium#20318, cilium#1383) policy resolve.go:103 (*selectorPolicy).Detach(...) policy distillery.go:81 (*PolicyCache).delete(cilium#1354, cilium#19354) policy distillery.go:138 (*PolicyCache).LocalEndpointIdentityRemoved(cilium#523, cilium#1163) identitymanager manager.go:167 (*IdentityManager).remove(cilium#706, cilium#19354) identitymanager manager.go:147 (*IdentityManager).Remove(cilium#706, cilium#19354) identitymanager manager.go:52 Remove(...) endpoint endpoint.go:1146 (*Endpoint).leaveLocked(cilium#1883, cilium#12221, {0x30, 0}) endpoint endpoint.go:2192 (*Endpoint).Delete(cilium#1883, {0x80, 0xaa}) endpointmanager manager.go:380 (*EndpointManager).removeEndpoint(cilium#1161, 0, {0xff, 0xff}) endpointmanager manager.go:394 (*EndpointManager).RemoveEndpoint(...) cmd endpoint.go:684 (*Daemon).deleteEndpointQuiet(...) cmd endpoint.go:666 (*Daemon).deleteEndpoint(cilium#1155, cilium#1883) cmd endpoint.go:713 (*Daemon).DeleteEndpoint(cilium#1155, {cilium#27399, cilium#8108}) cmd endpoint.go:770 (*deleteEndpointID).Handle(cilium#21193, {cilium#2452, {cilium#27399, 0x4d}}) endpoint delete_endpoint_id.go:66 (*DeleteEndpointID).ServeHTTP(cilium#5934, {cilium#666, cilium#2242}, cilium#2452) middleware operation.go:28 (*Context).RoutesHandler.NewOperationExecutor.func1({cilium#666, cilium#2242}, cilium#2452) http server.go:2136 HandlerFunc.ServeHTTP(ReadCloser(cilium#130), func{cilium#2242, 0x3}) middleware router.go:78 NewRouter.func1({cilium#666, cilium#2242}, cilium#2451) http server.go:2136 HandlerFunc.ServeHTTP(ReadCloser(cilium#718), func{cilium#2242, #59}) middleware redoc.go:72 Redoc.func1({cilium#666, cilium#2242}, cilium#1251) http server.go:2136 HandlerFunc.ServeHTTP(ReadCloser(cilium#4920), func{cilium#2242, #45}) middleware spec.go:46 Spec.func1({cilium#666, cilium#2242}, cilium#4921) http server.go:2136 HandlerFunc.ServeHTTP(ReadCloser(cilium#10532), func{cilium#2242, cilium#23015}) metrics middleware.go:64 (*APIEventTSHelper).ServeHTTP(cilium#1459, {cilium#668, cilium#10533}, cilium#2451) api apipanic.go:42 (*APIPanicHandler).ServeHTTP(cilium#722, {cilium#668, cilium#10533}, cilium#4922) http server.go:2938 serverHandler.ServeHTTP(*Server(cilium#8105), cilium#668, cilium#10533, 0x6) http server.go:2009 (*conn).serve(*conn(cilium#16005), Context{cilium#673, cilium#1554}) 8: sync.Mutex.Lock [74 minutes] [Created by http.(*Server).Serve in goroutine 1699 @ server.go:3086] sync sema.go:77 runtime_SemacquireMutex(*, 0x47, cilium#1154) sync mutex.go:171 (*Mutex).lockSlow(cilium#706) sync mutex.go:90 (*Mutex).Lock(...) sync rwmutex.go:147 (*RWMutex).Lock(*) identitymanager manager.go:99 (*IdentityManager).RemoveOldAddNew(cilium#706, 0, cilium#1154) identitymanager manager.go:123 RemoveOldAddNew(...) endpoint policy.go:852 (*Endpoint).SetIdentity(*, cilium#1154, 0) endpoint endpoint.go:1932 (*Endpoint).identityLabelsChanged(*, {cilium#674, *}, 1) endpoint endpoint.go:1780 (*Endpoint).runIdentityResolver(*, {cilium#674, *}, 1, 1) endpoint endpoint.go:1720 (*Endpoint).UpdateLabels(*, {cilium#674, *}, *, *, 8) cmd endpoint.go:477 (*Daemon).createEndpoint(cilium#1155, {cilium#673, *}, {cilium#683, cilium#1155}, *) cmd endpoint.go:542 (*putEndpointID).Handle(cilium#21192, {*, *, {*, 0xe}}) endpoint put_endpoint_id.go:58 (*PutEndpointID).ServeHTTP(cilium#3961, {cilium#666, *}, *) middleware operation.go:28 (*Context).RoutesHandler.NewOperationExecutor.func1({cilium#666, *}, *) http server.go:2136 HandlerFunc.ServeHTTP(*, {cilium#666, *}, *) middleware router.go:78 NewRouter.func1({cilium#666, *}, *) http server.go:2136 HandlerFunc.ServeHTTP(*, {cilium#666, *}, *) middleware redoc.go:72 Redoc.func1({cilium#666, *}, cilium#1251) http server.go:2136 HandlerFunc.ServeHTTP(*, {cilium#666, *}, #45) middleware spec.go:46 Spec.func1({cilium#666, *}, *) http server.go:2136 HandlerFunc.ServeHTTP(*, {cilium#666, *}, *) metrics middleware.go:64 (*APIEventTSHelper).ServeHTTP(cilium#1459, {cilium#668, *}, *) api apipanic.go:42 (*APIPanicHandler).ServeHTTP(#49, {cilium#668, *}, *) http server.go:2938 serverHandler.ServeHTTP({cilium#653}, {cilium#668, *}, 6) http server.go:2009 (*conn).serve(*, {cilium#673, cilium#1554}) 5: sync.Mutex.Lock [75 minutes] [Created by eventqueue.(*EventQueue).Run in goroutine 1482 @ eventqueue.go:229] sync sema.go:77 runtime_SemacquireMutex(cilium#142, 0xe8, *) sync mutex.go:171 (*Mutex).lockSlow(cilium#1733) sync mutex.go:90 (*Mutex).Lock(...) sync rwmutex.go:147 (*RWMutex).Lock(0x68) fqdn name_manager.go:70 (*NameManager).Lock(*) policy selectorcache.go:798 (*SelectorCache).AddFQDNSelector(cilium#1088, {cilium#643, *}, {{*, 0x4d}, {0, 0}}) policy l4.go:628 (*L4Filter).cacheFQDNSelector(...) policy l4.go:623 (*L4Filter).cacheFQDNSelectors(*, {*, 4, cilium#193}, cilium#536) policy l4.go:725 createL4Filter({cilium#680, *}, {*, 1, 1}, 0, {cilium#660, *}, {{*, 4}, ...}, ...) policy l4.go:879 createL4EgressFilter(...) policy rule.go:717 mergeEgressPortProto({cilium#680, *}, #44, {*, 0xa, 0}, *, {cilium#660, *}, {{*, ...}, ...}, ...) policy rule.go:672 mergeEgress.func1({cilium#660, *}) api l4.go:284 PortRules.Iterate({*, 1, cilium#546}, *) policy rule.go:624 mergeEgress({cilium#680, *}, *, {*, 1, 1}, 0, {cilium#661, *}, {cilium#662, ...}, ...) policy rule.go:753 (*rule).resolveEgressPolicy(*, {cilium#680, *}, *, *, *, {0, 0, 0}, {0, ...}) policy rules.go:103 ruleSlice.resolveL4EgressPolicy({*, *, *}, {cilium#680, *}, *) policy repository.go:718 (*Repository).resolvePolicyLocked(cilium#1089, *) policy distillery.go:119 (*PolicyCache).updateSelectorPolicy(cilium#1354, *) policy distillery.go:153 (*PolicyCache).UpdatePolicy(...) endpoint policy.go:262 (*Endpoint).regeneratePolicy(*) endpoint bpf.go:744 (*Endpoint).runPreCompilationSteps(*, *, *) endpoint bpf.go:589 (*Endpoint).regenerateBPF(*, *) endpoint policy.go:457 (*Endpoint).regenerate(*, *) endpoint events.go:53 (*EndpointRegenerationEvent).Handle(*, *) eventqueue eventqueue.go:245 (*EventQueue).run.func1() sync once.go:74 (*Once).doSlow(*, *) sync once.go:65 (*Once).Do(...) eventqueue eventqueue.go:233 (*EventQueue).run(*) 1: select [75 minutes] [Created by eventqueue.(*EventQueue).Run in goroutine 1482 @ eventqueue.go:229] semaphore semaphore.go:60 (*Weighted).Acquire(cilium#1092, {cilium#671, cilium#722}, cilium#766) lock semaphored_mutex.go:30 (*SemaphoredMutex).Lock(...) ipcache ipcache.go:140 (*IPCache).Lock(...) ipcache cidr.go:56 (*IPCache).AllocateCIDRs(cilium#1316, {0, 0, cilium#2037}, {0, 0, 0}, 0) ipcache cidr.go:103 (*IPCache).AllocateCIDRsForIPs(0, {0, cilium#697, 0}, 0xffffffffffffffff) cmd identity.go:114 cachingIdentityAllocator.AllocateCIDRsForIPs(...) policy selectorcache.go:509 (*SelectorCache).allocateIdentityMappings(cilium#1088, {{0, 0}, {cilium#5036, 0x2b}}, #45) policy selectorcache.go:843 (*SelectorCache).AddFQDNSelector(cilium#1088, {cilium#643, cilium#27811}, {{0, 0}, {cilium#5036, 0x2b}}) policy l4.go:628 (*L4Filter).cacheFQDNSelector(...) policy l4.go:623 (*L4Filter).cacheFQDNSelectors(cilium#27811, {cilium#42936, 0x287, cilium#193}, cilium#536) policy l4.go:725 createL4Filter({cilium#680, cilium#22826}, {cilium#22828, 1, 1}, 0, {cilium#660, cilium#24510}, {{cilium#4487, 3}, ...}, ...) policy l4.go:879 createL4EgressFilter(...) policy rule.go:717 mergeEgressPortProto({cilium#680, cilium#22826}, #44, {cilium#22828, 0xa, #78536}, #79633, {cilium#660, cilium#24510}, {{cilium#4487, ...}, ...}, ...) policy rule.go:672 mergeEgress.func1({cilium#660, cilium#24510}) api l4.go:284 PortRules.Iterate({cilium#24510, 1, cilium#546}, cilium#11741) policy rule.go:624 mergeEgress({cilium#680, cilium#22826}, cilium#18687, {cilium#22828, 1, 1}, 0, {cilium#661, cilium#5624}, {cilium#662, ...}, ...) policy rule.go:753 (*rule).resolveEgressPolicy(cilium#24575, {cilium#680, cilium#22826}, cilium#18687, cilium#29345, cilium#4782, {0, 0, 0}, {0, ...}) policy rules.go:103 ruleSlice.resolveL4EgressPolicy({cilium#10690, 0xb, 5}, {cilium#680, cilium#22826}, cilium#18687) policy repository.go:718 (*Repository).resolvePolicyLocked(cilium#1089, cilium#18461) policy distillery.go:119 (*PolicyCache).updateSelectorPolicy(cilium#1354, cilium#18461) policy distillery.go:153 (*PolicyCache).UpdatePolicy(...) endpoint policy.go:262 (*Endpoint).regeneratePolicy(cilium#1748) endpoint bpf.go:744 (*Endpoint).runPreCompilationSteps(cilium#1748, cilium#27542, cilium#4781) endpoint bpf.go:589 (*Endpoint).regenerateBPF(cilium#1748, cilium#27542) endpoint policy.go:457 (*Endpoint).regenerate(cilium#1748, cilium#27542) endpoint events.go:53 (*EndpointRegenerationEvent).Handle(cilium#18609, cilium#703) eventqueue eventqueue.go:245 (*EventQueue).run.func1() sync once.go:74 (*Once).doSlow(*Once(cilium#45414), func(cilium#69)) sync once.go:65 (*Once).Do(...) eventqueue eventqueue.go:233 (*EventQueue).run(cilium#6023) ``` Generated from pp tool: https://github.com/maruel/panicparse Signed-off-by: Chris Tarazi <chris@isovalent.com>

addreas requested review from a team as code owners August 9, 2023 19:42

addreas requested a review from joamaki August 9, 2023 19:42

maintainer-s-little-helper Bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 9, 2023

github-actions Bot added the kind/community-contribution This was a contribution made by a community member. label Aug 9, 2023

addreas force-pushed the lb-ipam-status-conditions branch from 60c39e4 to 366c66f Compare August 9, 2023 19:49

addreas requested a review from a team as a code owner August 9, 2023 19:49

addreas requested a review from a user August 9, 2023 19:49

ghost approved these changes Aug 10, 2023

View reviewed changes

ghost added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Aug 10, 2023

maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 10, 2023

joamaki requested a review from dylandreimerink August 17, 2023 07:42

dylandreimerink approved these changes Aug 18, 2023

View reviewed changes

Comment thread operator/pkg/lbipam/lbipam.go Outdated

christarazi requested changes Aug 25, 2023

View reviewed changes

addreas force-pushed the lb-ipam-status-conditions branch from 366c66f to 59e8197 Compare August 28, 2023 10:16

addreas added 2 commits August 28, 2023 12:34

Handle status conditions using k8s.io/apimachinery/pkg/api/meta utili…

78b31f6

…ties This changes the behaviour such that any one type of condition will only appear once in the .status.conditions list. Fixes: cilium#24945 Signed-off-by: Andreas Mårtensson <andreas@addem.se>

Use condition type names that follow KEP-1623

5a1058a

Signed-off-by: Andreas Mårtensson <andreas@addem.se>

addreas force-pushed the lb-ipam-status-conditions branch from 59e8197 to 5a1058a Compare August 28, 2023 10:34

christarazi approved these changes Aug 28, 2023

View reviewed changes

christarazi requested a review from aanm August 28, 2023 17:04

aanm approved these changes Aug 28, 2023

View reviewed changes

joamaki approved these changes Aug 29, 2023

View reviewed changes

christarazi added the area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label Aug 29, 2023

maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 29, 2023

borkmann merged commit 42bd6ef into cilium:main Aug 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `.status.conditions` on `Service`s using in accordance with KEP-1623#27399

Handle `.status.conditions` on `Service`s using in accordance with KEP-1623#27399
borkmann merged 2 commits intocilium:mainfrom
addreas:lb-ipam-status-conditions

addreas commented Aug 9, 2023

Uh oh!

ghost left a comment

Uh oh!

ghost commented Aug 10, 2023

Uh oh!

Uh oh!

joestringer commented Aug 25, 2023

Uh oh!

christarazi left a comment •

edited

Loading

Uh oh!

addreas commented Aug 28, 2023

Uh oh!

christarazi commented Aug 28, 2023

Uh oh!

aanm left a comment

Uh oh!

joamaki commented Aug 29, 2023 •

edited by christarazi

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

addreas commented Aug 9, 2023

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 10, 2023

Uh oh!

Uh oh!

joestringer commented Aug 25, 2023

Uh oh!

christarazi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addreas commented Aug 28, 2023

Uh oh!

christarazi commented Aug 28, 2023

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

joamaki commented Aug 29, 2023 • edited by christarazi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

christarazi left a comment •

edited

Loading

joamaki commented Aug 29, 2023 •

edited by christarazi

Loading