CLOUDP-82159: Upgrade to operator-sdk v1.4.0 #327

alyacb · 2021-02-11T16:16:09Z

This change re-scaffolds the operator using the latest version of operator-sdk (v1.4.0)

Code refactorings:
- pkg/apis/mongodb/v1/ -> api/v1/
- pkg/controller/ -> controllers/
CRD refactorings:
- CRDs were moved to config/, CRs to config/samples
- The new directory structure has roles in config/rbac/
- operator.yaml was renamed and placed in config/manager/manager.yaml
- kustomize is being used everywhere
Build refactorings:
- Added a Makefile. The operator can now be built and deployed locally using:
  make generate manifests docker-build docker-push deploy IMG=localhost:5000/mongodb-kubernetes-operator
  - make manifests generates CRDs
  - make generate generates deepcopy code
  - make docker-build docker-push builds the docker image and pushes it. I modified this directive to generate the Dockerfile using our existing scripts, but if you want to generate a Dockerfile other than the operator, e.g. e2e, you can now run make docker-build docker-push IMG=localhost:5000/e2e DOCKERFILE=e2e
- Changes to the operator Dockerfile template to build the code in the same way as the Makefile
- Changes to any affected scripts so they can work with the new directory format/ way of building

It was necessary to remove the wrappers around the StatefulSetConfiguration field in mongodb_types, as fields annotated with "-" json struct tags no longer are included, not even with the custom JSON marshalling code. This reintroduced CRD pollution (and accounts for most of the liens of code changed). The CRD is automatically generated, so hopefully this shouldn't be too much of a problem.

StatefulSetConfiguration appsv1.StatefulSetSpec `json:"statefulSet,omitempty"`

For a similar reason, these changes break the additionalMongodConfig field(the only failing e2e test is e2e_test_replica_set_mongod_config). However, the fix for this field is not apparent, since its type boils down to map[string]interface{}, which cannot be generated in the CRDs as interface{} is not a named type.

Note: this doesn't include the necessary documentation changes, which should be added in a separate PR.

alyacb · 2021-02-16T10:39:18Z

.evergreen.yml

@@ -19,17 +19,6 @@ functions:
      params:
        directory: mongodb-kubernetes-operator

-  setup_operator_sdk:


No longer needed, so I removed this step.

alyacb · 2021-02-16T10:44:40Z

api/v1/mongodbcommunity_types.go

@@ -69,7 +69,7 @@ type MongoDBCommunitySpec struct {
 	Users []MongoDBUser `json:"users"`

 	// +optional
-	StatefulSetConfiguration StatefulSetConfiguration `json:"statefulSet,omitempty"`
+	StatefulSetConfiguration appsv1.StatefulSetSpec `json:"statefulSet,omitempty"`


This change fixed the statefulset_arbitrary e2e tests. For some reason, even though the custom marshal/unmarshal code should be including this field when annotated with "-", it is being ignored. For that reason I reintroduced the CRD generation (which should be fine as long as we can automatically generate the CRD).

we need to make sure the end resource still works by specifying statefulSet.spec

alyacb · 2021-02-16T10:45:19Z

build/bin/entrypoint

@@ -9,4 +9,5 @@ if ! whoami >/dev/null 2>&1; then
  fi
 fi

-exec "${OPERATOR}" "$@"
+echo "Running ./${OPERATOR}"
+"./${OPERATOR}" "$@"


exec was not working, so I switched to "./"

chatton

Looks great, massive work! Had a few comments, the main issue we need to address is fixing the test failures in the additionalMongodConfig test and making sure that the spec of the MongoDBCommunity resources is still mdb.spec.statefulSet.spec.*

test/e2e/e2eutil.go

test/e2e/statefulset_arbitrary_config/statefulset_arbitrary_config_test.go

.gitignore

Makefile

scripts/dev/templates/Dockerfile.e2e

config/samples/arbitrary_statefulset_configuration/mongodb.com_v1_custom_volume_cr.yaml

config/samples/kustomization.yaml

config/samples/mongodbcommunity_v1_mongodbcommunity.yaml

chatton · 2021-02-16T12:54:33Z

api/v1/mongodbcommunity_types.go

@@ -69,7 +69,7 @@ type MongoDBCommunitySpec struct {
 	Users []MongoDBUser `json:"users"`

 	// +optional
-	StatefulSetConfiguration StatefulSetConfiguration `json:"statefulSet,omitempty"`
+	StatefulSetConfiguration appsv1.StatefulSetSpec `json:"statefulSet,omitempty"`


we need to make sure the end resource still works by specifying statefulSet.spec

chatton · 2021-02-16T12:55:31Z

controllers/replica_set_controller.go

-
-	return nil
+// SetupWithManager sets up the controller with the Manager and configures the necessary watches.
+func (r *ReplicaSetReconciler) SetupWithManager(mgr ctrl.Manager) error {


[q] is there a reason this is a method and not a function? Does this implement some interface that is required?

Not a big deal either way!

I don't think it's required, but it is the way the scaffolding intended (and matches the Reconcile method). I don't think this particular method gets called anywhere outside of main.go (e.g. by the library), but my vote would be to keep it consistent with operator-sdk's expectations so that we don't diverge too far, just in case they introduce code expecting it to follow this format.

alyacb · 2021-02-16T16:38:15Z

Pushed changes to move back to CRD version v1beta1 and address feedback. Note that whenever we are ready to move to version v1beta1, here's what needs to happen:

Replace all occurrences of v1beta1 in the code with v1
Change this line in the Makefile from:

CRD_OPTIONS ?= "crd:trivialVersions=true,preserveUnknownFields=true,crdVersions=v1beta1"

to:

CRD_OPTIONS ?= "crd:trivialVersions=true,preserveUnknownFields=false"

Add the following annotation to the StatefulSetConfiguration and AdditionalMongodConfig fields on the MongoDBCommunitySpec struct. This is the v1 replacement for preserveUnknownFields.

// +kubebuilder:validation:XPreserveUnknownFields

Run make generate manifests

config/manager/kustomization.yaml

chatton

This looks amazing! I think we're good to go, I think if @bznein / @irajdeep can take a look at this as well that would be great.

We can tackle the upgrade from v1beta1 to v1 in a future PR.

cc @rodrigovalin this will be a great PR to use as a reference when working on the CRD project.

irajdeep

LGTM! Great work. Added some minor comments.

cmd/manager/main.go

irajdeep · 2021-02-16T18:16:19Z

config/crd/kustomization.yaml

@@ -0,0 +1,10 @@
+# This kustomization.yaml is not intended to be run by itself,
+# since it depends on service name and namespace that are out of this kustomize package.


q: are we adding kustomize support with this PR as well?

Yes. The project was re-scaffolded with the new version of operator-sdk, which generated a bunch of kustomize files for us. The make deploy target in the Makefile uses kustomize to generate the yaml it applies.

controllers/replica_set_controller.go

alyacb added the wip label Feb 11, 2021

alyacb force-pushed the CLOUDP-82159_upgrade_operator_sdk_v1 branch 4 times, most recently from 2ff254a to bef215d Compare February 15, 2021 09:33

alyacb added 3 commits February 16, 2021 10:12

Import-only changes

55891f2

Non-trivial code changes

4b6c47f

Build, github, and evergreen changes

d23c43e

alyacb force-pushed the CLOUDP-82159_upgrade_operator_sdk_v1 branch from 4eedf29 to 86596b2 Compare February 16, 2021 10:38

alyacb commented Feb 16, 2021

View reviewed changes

CRD changes

5c054bc

alyacb force-pushed the CLOUDP-82159_upgrade_operator_sdk_v1 branch from 86596b2 to 5c054bc Compare February 16, 2021 10:53

alyacb changed the title ~~[WIP] CLOUDP-82159: Upgrade to operator-sdk v1.4.0~~ CLOUDP-82159: Upgrade to operator-sdk v1.4.0 Feb 16, 2021

alyacb requested a review from chatton February 16, 2021 11:14

chatton reviewed Feb 16, 2021

View reviewed changes

alyacb added 2 commits February 16, 2021 15:49

Address PR comments

b825fb3

Address PR comments II

5407554

alyacb removed the wip label Feb 16, 2021

alyacb marked this pull request as ready for review February 16, 2021 16:38

chatton reviewed Feb 16, 2021

View reviewed changes

config/manager/kustomization.yaml Show resolved Hide resolved

chatton approved these changes Feb 16, 2021

View reviewed changes

irajdeep self-requested a review February 16, 2021 17:58

irajdeep reviewed Feb 16, 2021

View reviewed changes

Address PR comments III

9983c95

irajdeep approved these changes Feb 17, 2021

View reviewed changes

alyacb merged commit e7c6368 into master Feb 17, 2021

alyacb deleted the CLOUDP-82159_upgrade_operator_sdk_v1 branch February 17, 2021 10:12

bznein mentioned this pull request Feb 17, 2021

kubectl create -f deploy/crds/mongodb.com_mongodbcommunity_crd.yaml command fails #334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLOUDP-82159: Upgrade to operator-sdk v1.4.0 #327

CLOUDP-82159: Upgrade to operator-sdk v1.4.0 #327

alyacb commented Feb 11, 2021 •

edited

Loading

alyacb Feb 16, 2021

alyacb Feb 16, 2021

chatton Feb 16, 2021

alyacb Feb 16, 2021

chatton left a comment

chatton Feb 16, 2021

chatton Feb 16, 2021

alyacb Feb 16, 2021

alyacb commented Feb 16, 2021

chatton left a comment •

edited

Loading

irajdeep left a comment

irajdeep Feb 16, 2021

alyacb Feb 16, 2021

		@@ -0,0 +1,10 @@
		# This kustomization.yaml is not intended to be run by itself,
		# since it depends on service name and namespace that are out of this kustomize package.

CLOUDP-82159: Upgrade to operator-sdk v1.4.0 #327

CLOUDP-82159: Upgrade to operator-sdk v1.4.0 #327

Conversation

alyacb commented Feb 11, 2021 • edited Loading

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

chatton left a 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

alyacb commented Feb 16, 2021

chatton left a comment • edited Loading

Choose a reason for hiding this comment

irajdeep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyacb commented Feb 11, 2021 •

edited

Loading

chatton left a comment •

edited

Loading