refactor: move CRD registration to separate cell #24219

knight42 · 2023-03-07T14:18:09Z

christarazi · 2023-03-08T06:39:09Z

Travis has a failure, but it's unclear to me if it's related to this PR:

panic: *resource.resource[*github.com/cilium/cilium/pkg/k8s/slim/k8s/api/core/v1.Service].Events() called from /home/travis/gopath/src/github.com/cilium/cilium/operator/pkg/lbipam/lbipam.go:192 has a broken event handler that did not call Done() before event was garbage collected

                                                                                                                                                                                                                                                                                        

goroutine 5 [running]:                                                                                                                                                                                                                                                                  

github.com/cilium/cilium/pkg/k8s/resource.(*resource[...]).Events.func1.1()                                                                                                                                                                                                             

	/home/travis/gopath/src/github.com/cilium/cilium/pkg/k8s/resource/resource.go:343 +0x68                                                                                                                                                                                                 

FAIL	github.com/cilium/cilium/operator/pkg/lbipam	3.698s

cc @dylandreimerink

pippolo84

Thanks for your PR!
I left some comments inline with suggestions on how to restructure the new cell.

operator/cmd/root.go

knight42 · 2023-03-09T15:33:59Z

Thanks for your PR! I left some comments inline with suggestions on how to restructure the new cell.

Thanks for your detailed guidance, I will update this PR in a few days.

maintainer-s-little-helper · 2023-03-10T16:27:51Z

Commit f7fd9410946764331bccd4d130934438f9a16d4f does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

knight42 · 2023-03-11T02:23:20Z

Travis has a failure, but it's unclear to me if it's related to this PR:

panic: *resource.resource[*github.com/cilium/cilium/pkg/k8s/slim/k8s/api/core/v1.Service].Events() called from /home/travis/gopath/src/github.com/cilium/cilium/operator/pkg/lbipam/lbipam.go:192 has a broken event handler that did not call Done() before event was garbage collected

                                                                                                                                                                                                                                                                                        

goroutine 5 [running]:                                                                                                                                                                                                                                                                  

github.com/cilium/cilium/pkg/k8s/resource.(*resource[...]).Events.func1.1()                                                                                                                                                                                                             

	/home/travis/gopath/src/github.com/cilium/cilium/pkg/k8s/resource/resource.go:343 +0x68                                                                                                                                                                                                 

FAIL	github.com/cilium/cilium/operator/pkg/lbipam	3.698s

cc @dylandreimerink

The test passed this time, seems to be a flaky one 🤔

pippolo84

Thanks for the updated PR!

The cell is ok, my main concern is about the management of the skipCRDCreation option. We can improve it moving to the cell itself.

pkg/k8s/apis/cilium.io/client/cell.go

operator/cmd/root.go

operator/cmd/flags.go

pippolo84

Changes look good, only things remaining are:

fix the operator options setting in controlplane test
update documentation in cmdref

I left some notes inline.

test/controlplane/suite/testcase.go

pkg/k8s/apis/cilium.io/client/cell.go

maintainer-s-little-helper · 2023-03-15T09:45:40Z

Commit 113c868a1c20c183b4ac29858b1e5828e6ed8d4b does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2023-03-15T09:48:46Z

Commits 113c868a1c20c183b4ac29858b1e5828e6ed8d4b, 0e794d6ac866468dc544abe1f8f7833067ec4941 do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

pippolo84 · 2023-03-17T10:20:08Z

@pippolo84 Got it, so.. could we simply rerun this test? If all tests become green, I think we could get this PR merged +1

We usually avoid rerunning in this case, it would just hide the flake.
For the PR to be marked as ready to merge, we should wait for the other reviewers feedback.

Documentation/cmdref/cilium-operator-alibabacloud_hive.md

test/controlplane/suite/testcase.go

knight42 · 2023-03-18T04:34:46Z

@christarazi I have squashed the commits, PTAL

knight42 · 2023-03-22T03:11:41Z

gently ping @christarazi

christarazi · 2023-03-22T06:09:22Z

/test

youngnick · 2023-03-22T23:34:41Z

@knight42 the outstanding CI failure is a known flake that should now be fixed, #23272. If you rebase, it should clear up, and then this should be able to be merged.

knight42 · 2023-03-23T04:29:16Z

@knight42 the outstanding CI failure is a known flake that should now be fixed, #23272. If you rebase, it should clear up, and then this should be able to be merged.

Hi I have rebased and there is no CI error this time 🎉

pippolo84 · 2023-03-29T12:18:37Z

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Tests NodePort inside cluster (kube-proxy) with IPSec and externalTrafficPolicy=Local

Failure Output

FAIL: Request from k8s1 to service http://[fd04::11]:30227 failed

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

ciliumbot · 2023-03-29T14:44:13Z

Build finished.

christarazi · 2023-03-29T19:00:11Z

@pippolo84 We should track the CI failures before we mark ready to merge.

pippolo84 · 2023-03-30T09:42:24Z

/mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19

👍 created #24648

pippolo84 · 2023-03-30T09:43:38Z

/test-1.25-4.19

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Tests NodePort inside cluster (kube-proxy) with IPSec and externalTrafficPolicy=Local

Failure Output

FAIL: Request from k8s1 to service http://[fd04::11]:32161 failed

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

Edit: Hit #24648 again

pippolo84 · 2023-03-30T12:33:41Z

/test-1.26-net-next

christarazi · 2023-03-30T18:37:19Z

/test-1.25-4.19

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Tests NodePort inside cluster (kube-proxy) with IPSec and externalTrafficPolicy=Local

Failure Output

FAIL: Request from k8s1 to service http://[fd04::11]:30470 failed

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

Fixes: cilium#24068 1. Move CRD registration to a separate cell inside the client package, and add a `--skip-crd-creation` flag. 2. Remove the `SkipCRDOption` from the `operatorOption.OperatorConfig` struct, alongside any reference to it in `operator/cmd/fags.go`, `operator/option/config.go` and `pkg/option/config.go` 3. Update cmdref Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

christarazi · 2023-03-30T22:21:05Z

/test

Edit: 5.4 hit #24573

christarazi · 2023-03-31T00:53:57Z

/test-1.24-5.4

knight42 requested a review from a team as a code owner March 7, 2023 14:18

knight42 requested a review from christarazi March 7, 2023 14:18

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 Mar 7, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 7, 2023

pippolo84 self-requested a review March 8, 2023 09:25

pippolo84 requested changes Mar 8, 2023

View reviewed changes

operator/cmd/root.go Outdated Show resolved Hide resolved

operator/cmd/root.go Outdated Show resolved Hide resolved

operator/cmd/root.go Outdated Show resolved Hide resolved

knight42 requested a review from a team as a code owner March 10, 2023 16:27

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 10, 2023

knight42 force-pushed the refactor/modularize-crd-registration branch from f7fd941 to 5f03659 Compare March 10, 2023 16:30

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 10, 2023

pippolo84 requested changes Mar 13, 2023

View reviewed changes

pkg/k8s/apis/cilium.io/client/cell.go Outdated Show resolved Hide resolved

operator/cmd/root.go Outdated Show resolved Hide resolved

operator/cmd/root.go Outdated Show resolved Hide resolved

knight42 force-pushed the refactor/modularize-crd-registration branch from 5f03659 to c7b6f0b Compare March 13, 2023 14:38

knight42 requested review from a team as code owners March 13, 2023 14:38

knight42 requested a review from rolinh March 13, 2023 14:38

knight42 commented Mar 13, 2023

View reviewed changes

operator/cmd/flags.go Show resolved Hide resolved

pippolo84 added the release-note/misc This PR makes changes that have no direct user impact. label Mar 14, 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 Mar 14, 2023

pippolo84 requested changes Mar 14, 2023

View reviewed changes

test/controlplane/suite/testcase.go Outdated Show resolved Hide resolved

pippolo84 reviewed Mar 14, 2023

View reviewed changes

pkg/k8s/apis/cilium.io/client/cell.go Show resolved Hide resolved

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 15, 2023

knight42 force-pushed the refactor/modularize-crd-registration branch from 0e794d6 to 6714b8d Compare March 15, 2023 09:50

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 15, 2023

christarazi requested changes Mar 17, 2023

View reviewed changes

Documentation/cmdref/cilium-operator-alibabacloud_hive.md Show resolved Hide resolved

test/controlplane/suite/testcase.go Show resolved Hide resolved

christarazi added area/operator Impacts the cilium-operator component area/modularization labels Mar 17, 2023

knight42 force-pushed the refactor/modularize-crd-registration branch from 6714b8d to 396ccd6 Compare March 18, 2023 04:33

christarazi force-pushed the refactor/modularize-crd-registration branch from 396ccd6 to 767bd6a Compare March 22, 2023 06:09

christarazi approved these changes Mar 22, 2023

View reviewed changes

knight42 force-pushed the refactor/modularize-crd-registration branch from 767bd6a to 060c348 Compare March 23, 2023 03:15

rolinh approved these changes Mar 29, 2023

View reviewed changes

pippolo84 added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 29, 2023

christarazi removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 29, 2023

christarazi force-pushed the refactor/modularize-crd-registration branch from 060c348 to b918d35 Compare March 30, 2023 22:20

christarazi merged commit ba2362a into cilium:master Mar 31, 2023
42 checks passed

knight42 deleted the refactor/modularize-crd-registration branch March 31, 2023 06:20

pippolo84 mentioned this pull request Jul 4, 2023

CI: ci-e2e: check-log-errors level=error msg="Failed to watch *v2.CiliumClusterwideNetworkPolicy": the server could not find the requested resource #26591

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move CRD registration to separate cell #24219

refactor: move CRD registration to separate cell #24219

knight42 commented Mar 7, 2023 •

edited

christarazi commented Mar 8, 2023

pippolo84 left a comment

knight42 commented Mar 9, 2023

maintainer-s-little-helper bot commented Mar 10, 2023

knight42 commented Mar 11, 2023

pippolo84 left a comment

pippolo84 left a comment

maintainer-s-little-helper bot commented Mar 15, 2023

maintainer-s-little-helper bot commented Mar 15, 2023

pippolo84 commented Mar 17, 2023

knight42 commented Mar 18, 2023

knight42 commented Mar 22, 2023

christarazi commented Mar 22, 2023

youngnick commented Mar 22, 2023

knight42 commented Mar 23, 2023

pippolo84 commented Mar 29, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

ciliumbot commented Mar 29, 2023

christarazi commented Mar 29, 2023

pippolo84 commented Mar 30, 2023 •

edited by maintainer-s-little-helper bot

pippolo84 commented Mar 30, 2023 •

edited by christarazi

Test Name

Failure Output

pippolo84 commented Mar 30, 2023

christarazi commented Mar 30, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

christarazi commented Mar 30, 2023 •

edited

christarazi commented Mar 31, 2023

refactor: move CRD registration to separate cell #24219

refactor: move CRD registration to separate cell #24219

Conversation

knight42 commented Mar 7, 2023 • edited

christarazi commented Mar 8, 2023

pippolo84 left a comment

Choose a reason for hiding this comment

knight42 commented Mar 9, 2023

maintainer-s-little-helper bot commented Mar 10, 2023

knight42 commented Mar 11, 2023

pippolo84 left a comment

Choose a reason for hiding this comment

pippolo84 left a comment

Choose a reason for hiding this comment

maintainer-s-little-helper bot commented Mar 15, 2023

maintainer-s-little-helper bot commented Mar 15, 2023

pippolo84 commented Mar 17, 2023

knight42 commented Mar 18, 2023

knight42 commented Mar 22, 2023

christarazi commented Mar 22, 2023

youngnick commented Mar 22, 2023

knight42 commented Mar 23, 2023

pippolo84 commented Mar 29, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

ciliumbot commented Mar 29, 2023

christarazi commented Mar 29, 2023

pippolo84 commented Mar 30, 2023 • edited by maintainer-s-little-helper bot

pippolo84 commented Mar 30, 2023 • edited by christarazi

Test Name

Failure Output

pippolo84 commented Mar 30, 2023

christarazi commented Mar 30, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

christarazi commented Mar 30, 2023 • edited

christarazi commented Mar 31, 2023

knight42 commented Mar 7, 2023 •

edited

pippolo84 commented Mar 29, 2023 •

edited by maintainer-s-little-helper bot

pippolo84 commented Mar 30, 2023 •

edited by maintainer-s-little-helper bot

pippolo84 commented Mar 30, 2023 •

edited by christarazi

christarazi commented Mar 30, 2023 •

edited by maintainer-s-little-helper bot

christarazi commented Mar 30, 2023 •

edited