Refactor CRD generation in Makefile #24615

christarazi · 2023-03-29T00:20:52Z

Clarify function for all Cilium CRDs
Makefile: Extract paths for CRD into variable
Makefile: Extract CRD groups from generate-k8s-api
Makefile: Refactor CRD generation to avoid copy-paste pattern
Makefile: Clean up old CRD state, ensure blank slate

Makefile

christarazi · 2023-03-30T00:31:12Z

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathCustomCalls Basic test with byte-counter Loads byte-counter and gets consistent values, with per-endpoint routes

Failure Output

FAIL: Timed out after 240.000s.

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

joestringer

Broadly looks good to me, how do we test that this doesn't break CRD generation for some of the files? (I'm sure you've done a fine job, but Makefile syntax can be finnicky and surprising at the best of times)

christarazi · 2023-03-30T01:04:41Z

@joestringer Good question. I guess we have the GH Action to run make manifests generate-k8s-api and ensure that we don't have a mismatch, but the flip side is to check whether we didn't break legitimate runs to introduce changes to the existing CRDs and checking that we get the same output from this branch vs master.

I applied the following diff to both this branch and master, and ran the make targets:

diff --git a/pkg/policy/api/rule.go b/pkg/policy/api/rule.go
index 6dfeb63244..a0d5484da6 100644
--- a/pkg/policy/api/rule.go
+++ b/pkg/policy/api/rule.go
@@ -100,6 +100,11 @@ type Rule struct {
 	//
 	// +kubebuilder:validation:Optional
 	Description string `json:"description,omitempty"`
+
+	// Foo is a test.
+	//
+	// +kubebuilder:validation:Required
+	Foo string `json:"foo,omitempty"`
 }
 
 // MarshalJSON returns the JSON encoding of Rule r. We need to overwrite it to

The resulting diff that I get back is identical on both branches, so this should be good to go.

Does that resolve your concerns?

mhofstetter · 2023-03-30T07:09:34Z

regarding generator check: check-k8s-code-gen.sh gets executed in the GH action lint-go.

might be worth to delete the artifacts generated by make manifests before re-generating and checking (delete pkg/k8s/apis/cilium.io/client/crds ?). the same is already performed for deepequal & deepcopy artifacts. This way we also detect stale generated artifacts.

ℹ️ btw: by doing this we have to ensure, that make manifests creates the directories pkg/k8s/apis/cilium.io/client/crds/v2 & pkg/k8s/apis/cilium.io/client/crds/v2alpha1 - otherwise copying the YAMLs from the tmp directory fails due to missing target directory

Documentation/contributing/development/introducing_new_crds.rst

Makefile

christarazi · 2023-03-30T23:20:27Z

Updated to address Marco's suggestions. There should be a new commit handling the cleaning up of the old artifact state and amending an existing commit to update the docs with the new instructions.

mhofstetter · 2023-03-31T04:26:33Z

Makefile

-	mv ${TMPDIR}/cilium.io_ciliumnodeconfigs.yaml ./pkg/k8s/apis/cilium.io/client/crds/v2alpha1/ciliumnodeconfigs.yaml
+
+	# Clean up old CRD state and start with a blank state.
+	for path in $(CRDS_CILIUM_PATHS); do rm -rf $${path} && mkdir $${path}; done


even though i meant to add it here it's kind of nice to have it in the make target directly! helps to detect stale YAML (of potentially deleted / moved (version) CRDs) even sooner during development. 👍

qmonnet

Thanks!

qmonnet · 2023-03-31T11:02:46Z

Makefile

+	# Clean up old CRD state and start with a blank state.
+	for path in $(CRDS_CILIUM_PATHS); do rm -rf $${path} && mkdir $${path}; done


Some nitpick suggestions but they don't really matter, I can't find a case where even mkdir -p would be necessary. Just “good practices” I guess.

Suggested change

# Clean up old CRD state and start with a blank state.

for path in $(CRDS_CILIUM_PATHS); do rm -rf $${path} && mkdir $${path}; done

# Clean up old CRD state and start with a blank slate.

for path in $(CRDS_CILIUM_PATHS); do $(RM) -rf -- $${path} && mkdir -p $${path}; done

This clarifies that this function will enumerate all Cilium CRDs, whereas previously it was unclear which CRDs because it said "all CRDs". Signed-off-by: Chris Tarazi <chris@isovalent.com>

This cleans up the invocation of controller-gen so that any new paths don't have to directly change the command line. Signed-off-by: Chris Tarazi <chris@isovalent.com>

This will allow the content of the generate-k8s-api to stay the same when needing to add another CRD group. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Now, all a developer is expected to do is to add the name of the CRD they want to generate into the corresponding variable (v2, v2alpha1, etc), and the automation will handle it from there. Signed-off-by: Chris Tarazi <chris@isovalent.com>

With this, we can also detect stale generated artifacts. This is now consistent with how the generate-k8s-api target works as well. Suggested-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi · 2023-03-31T22:44:59Z

/test

Edit: Hit #24573

christarazi · 2023-04-01T02:21:45Z

Marking ready to merge as we have approving reviews and CI is passing except for known flakes

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

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

christarazi changed the title ~~pr/christarazi/refactor crd creation~~ Refactor CRD generation in Makefile Mar 29, 2023

christarazi marked this pull request as ready for review March 29, 2023 23:30

christarazi requested review from a team as code owners March 29, 2023 23:30

christarazi requested review from tklauser, youngnick and qmonnet March 29, 2023 23:30

christarazi commented Mar 29, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

christarazi force-pushed the pr/christarazi/refactor-crd-creation branch from 22855ab to b2e90a7 Compare March 30, 2023 00:30

joestringer approved these changes Mar 30, 2023

View reviewed changes

youngnick approved these changes Mar 30, 2023

View reviewed changes

mhofstetter reviewed Mar 30, 2023

View reviewed changes

Documentation/contributing/development/introducing_new_crds.rst Outdated Show resolved Hide resolved

mhofstetter reviewed Mar 30, 2023

View reviewed changes

Makefile Show resolved Hide resolved

christarazi force-pushed the pr/christarazi/refactor-crd-creation branch from b2e90a7 to 07ade12 Compare March 30, 2023 23:19

christarazi requested a review from mhofstetter March 30, 2023 23:20

mhofstetter reviewed Mar 31, 2023

View reviewed changes

mhofstetter approved these changes Mar 31, 2023

View reviewed changes

qmonnet approved these changes Mar 31, 2023

View reviewed changes

tklauser approved these changes Mar 31, 2023

View reviewed changes

christarazi added 2 commits March 31, 2023 15:34

Clarify function for all Cilium CRDs

8df918b

This clarifies that this function will enumerate all Cilium CRDs, whereas previously it was unclear which CRDs because it said "all CRDs". Signed-off-by: Chris Tarazi <chris@isovalent.com>

Makefile: Extract paths for CRD into variable

f942b2b

This cleans up the invocation of controller-gen so that any new paths don't have to directly change the command line. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi force-pushed the pr/christarazi/refactor-crd-creation branch from 07ade12 to 3cb2156 Compare March 31, 2023 22:42

christarazi added 3 commits March 31, 2023 15:44

Makefile: Extract CRD groups from generate-k8s-api

732ddf2

This will allow the content of the generate-k8s-api to stay the same when needing to add another CRD group. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi force-pushed the pr/christarazi/refactor-crd-creation branch from 3cb2156 to 3d98466 Compare March 31, 2023 22:44

christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 1, 2023

julianwiedmann merged commit eb7645b into master Apr 3, 2023
42 checks passed

julianwiedmann deleted the pr/christarazi/refactor-crd-creation branch April 3, 2023 06:36

christarazi added backport/author The backport will be carried out by the author of the PR. affects/v1.13 This issue affects v1.13 branch labels Apr 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CRD generation in Makefile #24615

Refactor CRD generation in Makefile #24615

christarazi commented Mar 29, 2023 •

edited

christarazi commented Mar 30, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

joestringer left a comment

christarazi commented Mar 30, 2023 •

edited

mhofstetter commented Mar 30, 2023 •

edited

christarazi commented Mar 30, 2023

mhofstetter Mar 31, 2023

qmonnet left a comment

qmonnet Mar 31, 2023

christarazi commented Mar 31, 2023 •

edited

christarazi commented Apr 1, 2023

		# Clean up old CRD state and start with a blank state.
		for path in $(CRDS_CILIUM_PATHS); do rm -rf $${path} && mkdir $${path}; done

Refactor CRD generation in Makefile #24615

Refactor CRD generation in Makefile #24615

Conversation

christarazi commented Mar 29, 2023 • edited

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

Test Name

Failure Output

joestringer left a comment

Choose a reason for hiding this comment

christarazi commented Mar 30, 2023 • edited

mhofstetter commented Mar 30, 2023 • edited

christarazi commented Mar 30, 2023

mhofstetter Mar 31, 2023

Choose a reason for hiding this comment

qmonnet left a comment

Choose a reason for hiding this comment

qmonnet Mar 31, 2023

Choose a reason for hiding this comment

christarazi commented Mar 31, 2023 • edited

christarazi commented Apr 1, 2023

christarazi commented Mar 29, 2023 •

edited

christarazi commented Mar 30, 2023 •

edited by maintainer-s-little-helper bot

christarazi commented Mar 30, 2023 •

edited

mhofstetter commented Mar 30, 2023 •

edited

christarazi commented Mar 31, 2023 •

edited