Add feature to manage CRDs #1558

tmjd · 2021-10-08T22:41:31Z

Description

The changes also include fetching CRDs from the libcalico-go repos.

This PR is expected to be picked to the v1.23 branch when it is created for release with Calico v3.21

For PR author

Tests for change.
If changing pkg/apis/, run make gen-files
If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

Milestone set according to targeted release.
Appropriate labels:
- kind/bug if this is a bugfix.
- kind/enhancement if this is a a new feature.
- enterprise if this PR applies to Calico Enterprise only.

Brian-McM · 2021-10-13T17:46:32Z

Makefile

+    $(eval branch := $(2))
+    $(eval dir := $(3))
+	@echo "Fetching $(dir) CRDs from $(project) branch $(branch)"
+	rm -rf pkg/crds/$(dir)


I wonder if we should have a crd-clean target to remove all the CRD info and just assume it's been called before this?

Brian-McM · 2021-10-13T17:48:10Z

Makefile

+	echo "config/crd" >> .git/info/sparse-checkout; \
+	git checkout -q tags/$(branch) &>/dev/null ||  git checkout -q origin/$(branch) --;' || echo "Failed to fetch $(dir) CRDs"
+	@cp .crds/$(dir)/config/crd/* pkg/crds/$(dir)/ && echo "Copied $(dir) CRDs"
+	git add pkg/crds/$(dir)


I don't think we should do a git add here, this should be to the user to add and commit.

Why not do the add? I like having this here because it prevents someone from running the gen-versions and then missing that a new CRD was added. I understand it will be obvious if there are changes to existing CRDs but if someone has Untracked files, it could easily be missed that there are new CRDs to add.

I don't agree that part of the dev workflow, i.e. what files are added and commit and what aren't, should be put in here. It may be a temporary change they're adding, there could be many reasons why they don't want add / commit this.

If we want to ensure the CRDs are up to date it should be part of the CI, a specific check for this.

Ok so you're suggesting part of CI should be running the gen-files and then verifying nothing is dirty to ensure a user doesn't miss adding a CRD? That sounds reasonable.

Actually a new file won't count as dirty will it. So I guess we can't count on the current CI we have that does

make gen-versions make dirty-check

We can change that, I definitely think the dirty-check should check if there's new files as seen be git status.

Makefile

main.go

pkg/crds/crds.go

pkg/controller/installation/core_controller.go

Brian-McM · 2021-10-13T18:22:49Z

pkg/crds/crds.go

+				crd.Name = fmt.Sprintf("%s.%s", crd.Spec.Names.Plural, crd.Spec.Group)
+				calicoCRDs = append(calicoCRDs, crd)
+			}
+			crdyamls = getOperatorCRDSource(variant)


This seems to be a somewhat duplicate block of code that might be worth pulling out into a separate function.

pkg/crds/crds.go

pkg/controller/installation/core_controller.go

Brian-McM · 2021-10-13T18:35:50Z

pkg/controller/utils/component.go

+	case *apiextenv1.CustomResourceDefinition:
+		c := current.(*apiextenv1.CustomResourceDefinition)
+		d := desired.(*apiextenv1.CustomResourceDefinition)
+		d.SetResourceVersion(c.GetResourceVersion())


Is this correct? Would we want the resource version to be incremented, or fail if the resource current resource version is ahead of the desired (this protects against race conditions and reverting changes).

This is something I didn't understand why it was needed in my changes. When I loaded and parsed the CRDs from the yamls, they seemed to have a ResourceVersion set that meant the code above would not replace the ResourceVersion because there was already one set. I added this to override with the current ResourceVersion so that attempting to update would not fail.
I'm starting to wonder if the issue is that when we call Create/Update if the call updates the metadata on the object it is passed, and because the CRDs are pointers, if everything is using the same underlying data causing us to store the ResourceVersion. I wonder if I changed GetCRDs to create a new slice using DeepCopy of the original if this wouldn't be needed.

that meant the code above would not replace the ResourceVersion because there was already one set

Why do we want to replace the resource version though? If you were to edit a resource, the best way (I thought) would be to pull the resource with the resource version, edit it, then write it back. That way if something else did this simultaneously then this write would fail because the resource version would be different. I might be missing something though....

We want our desired resource to have the "current" ResourceVersion so when we do the update if something else has edited the same resource our update will fail.
The whole point of this mergeState is to merge our 'desired' resource with the 'current' one that was retrieved, this function is doing what you are suggesting we need to do.
I'm not quite sure why we don't just overwrite the desired ResourceVersion with the current ResourceVersion. That should be something we can always do.

I'll admit maybe I'm just confused and what I'm about to say is just wrong but I still don't see how this is doing what I suggested. I thought the desired resource is the one the controller passes in, and the current is the one that was just grabbed from kubernetes.

A dramatic example is what if the controller grabbed the resource from kubernetes, took 20 minutes to process stuff in the resource, and in that time 100 updates happened to that resource. Once the call to update the resource is made, a call to kubernetes will be made to get the current resource (that had 100 updates to it) and we will override the resource version of the desired resource (which is 100 updates behind), and the update will succeed and we will have undone 100 updates.

Brian-McM · 2021-10-18T16:26:58Z

pkg/controller/clusterconnection/shim_test.go

 	"github.com/tigera/operator/pkg/controller/status"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )

 func NewReconcilerWithShims(cli client.Client, schema *runtime.Scheme, status status.StatusManager, provider operatorv1.Provider) reconcile.Reconciler {
+	opts := options.AddOptions{
+		ShutdownContext: context.TODO(),


I think this should be something other than TODO

I'll switch it to Background().

Brian-McM · 2021-10-18T16:30:05Z

test/mainline_test.go

@@ -147,20 +148,21 @@ var _ = Describe("Mainline component function tests", func() {
 				return nil
 			}, 60*time.Second).Should(BeNil())

+			fmt.Printf("Status is %+v", instance.Status)


Leftover debugging?

- Move projectcalico CRDs to pkg/crds - Move operator CRDs to pkg/crds

- ability to print CRDs - Simple test that generating CRDs works - Add flag to enable/disable managing CRDs

- Also add retries to tigeraStatus updates - In tests wait for Operator to stop

- also use larger image so more disk is available - disable pre-loaded image cache that isn't helpful

marvin-tigera added this to the v1.23.0 milestone Oct 8, 2021

marvin-tigera added docs-pr-required release-note-required labels Oct 8, 2021

tmjd changed the title ~~Add feature to manage CRDs~~ [WIP] Add feature to manage CRDs Oct 8, 2021

tmjd changed the title ~~[WIP] Add feature to manage CRDs~~ Add feature to manage CRDs Oct 11, 2021

tmjd force-pushed the fetch-crds branch 2 times, most recently from 64814f6 to ab48395 Compare October 13, 2021 18:33

Brian-McM requested changes Oct 13, 2021

View reviewed changes

tmjd force-pushed the fetch-crds branch 4 times, most recently from d4933a1 to e58b1c8 Compare October 15, 2021 14:09

Brian-McM approved these changes Oct 15, 2021

View reviewed changes

tmjd force-pushed the fetch-crds branch from 28f2d07 to 3ba34ba Compare October 16, 2021 01:03

Brian-McM reviewed Oct 18, 2021

View reviewed changes

tmjd force-pushed the fetch-crds branch from da3b0a1 to a939509 Compare October 18, 2021 19:20

tmjd added 5 commits October 18, 2021 16:29

Fetch CRD updates when doing gen-versions

141bfd5

- Move projectcalico CRDs to pkg/crds - Move operator CRDs to pkg/crds

Add management of CRDs feature

87ff187

- ability to print CRDs - Simple test that generating CRDs works - Add flag to enable/disable managing CRDs

Add stops to go routines so tests are successful

bdeedb6

- Also add retries to tigeraStatus updates - In tests wait for Operator to stop

Fix Info logs from Status

d4bef17

Free up some CI disk space

5781104

- also use larger image so more disk is available - disable pre-loaded image cache that isn't helpful

tmjd force-pushed the fetch-crds branch from a939509 to 5781104 Compare October 18, 2021 21:31

tmjd merged commit bfcfb69 into tigera:master Oct 18, 2021

tmjd deleted the fetch-crds branch October 18, 2021 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature to manage CRDs #1558

Add feature to manage CRDs #1558

tmjd commented Oct 8, 2021 •

edited

Brian-McM Oct 13, 2021

Brian-McM Oct 13, 2021

tmjd Oct 13, 2021

Brian-McM Oct 13, 2021 •

edited

tmjd Oct 13, 2021

tmjd Oct 13, 2021

Brian-McM Oct 13, 2021

Brian-McM Oct 13, 2021

Brian-McM Oct 13, 2021

tmjd Oct 13, 2021

Brian-McM Oct 13, 2021

tmjd Oct 13, 2021

Brian-McM Oct 14, 2021

Brian-McM Oct 18, 2021

tmjd Oct 18, 2021

Brian-McM Oct 18, 2021

Add feature to manage CRDs #1558

Add feature to manage CRDs #1558

Conversation

tmjd commented Oct 8, 2021 • edited

Description

For PR author

For PR reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brian-McM Oct 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmjd commented Oct 8, 2021 •

edited

Brian-McM Oct 13, 2021 •

edited