Bump kubebuilder version from `v2` to `v3` #40

rustycl0ck · 2021-09-10T07:03:33Z

What this PR does / Why we need it

Upgrade kubebuilder version from v2 to v3 using the migration guide - https://book.kubebuilder.io/migration/migration_guide_v2tov3.html

Which issue(s) this PR fixes

Fixes #39, #33

rustycl0ck · 2021-09-10T07:13:51Z

The Makefile has a lot changes. I have kept the default generated makefile as it is. If there were any custom created make targets which should be ported over, please leave a comment.

Signed-off-by: rustyclock <rustyclock@protonmail.com>

rustycl0ck · 2021-09-10T09:09:23Z

cmd/spanner-autoscaler/main.go

-	healthzName     = "healthz"
-	readyzName      = "readyz"
+	exitCode         = 1
+	leaderElectionID = "spanner-autoscaler-leader-election"


This was created by default with the hash of repo name: leaderElectionID = "54b82eb3.mercari.com". I reverted it to the old one.

tjun · 2021-09-10T10:44:24Z

.dockerignore

-bin/
-config/
-.git/
-hack/
-.gitignore
-LICENSE
-Makefile
-PROJECT
-README.md


We do not need to remove them.

I just kept the default file in this case because I think it looked cleaner to have an allow-list instead have a block-list (block-list has to be updated whenever a new kind of file is added to the project). Let me know if you prefer to have both and I can add them back.

The following configures seem to be correct.

$ cat .dockerignore # More info: https://docs.docker.com/engine/reference/builder/#dockerignore-file # Ignore all files ** # Except go related files !pkg !cmd !**/*.mod !**/*.sum

You can check the following command.

$ docker image build -t build-context -f - . <<EOF FROM busybox COPY . /build-context WORKDIR /build-context CMD find . EOF $ docker container run --rm build-context . ./go.sum ./cmd ./cmd/spanner-autoscaler ./cmd/spanner-autoscaler/main.go ./pkg ./pkg/pointer ./pkg/pointer/pointer.go ./pkg/pointer/pointer_test.go ./pkg/api ./pkg/api/v1alpha1 ./pkg/api/v1alpha1/zz_generated.deepcopy.go ./pkg/api/v1alpha1/spannerautoscaler_types.go ./pkg/api/v1alpha1/groupversion_info.go ./pkg/metrics ./pkg/metrics/metrics.go ./pkg/metrics/fake.go ./pkg/controllers ./pkg/controllers/spannerautoscaler_controller.go ./pkg/controllers/spannerautoscaler_controller_test.go ./pkg/spanner ./pkg/spanner/spanner.go ./pkg/spanner/fake.go ./pkg/logging ./pkg/logging/context.go ./pkg/syncer ./pkg/syncer/fake ./pkg/syncer/fake/fake.go ./pkg/syncer/syncer_test.go ./pkg/syncer/syncer.go ./go.mod

!**/*.go doesn't work, so I choose !pkg and !cmd.

Excellent catch!! Thank you 🙇‍♂️ , I was not aware that this is an open bug in docker. I have now fixed it as per your recommendation since ! does not work with wild cards.

References:

Exception patterns in .dockerignore do not support wildcard directories moby/moby#30018

Scaffolded .dockerignore does not do what it claims kubernetes-sigs/kubebuilder#2073

Makefile

tjun · 2021-09-10T11:04:55Z

config/default/kustomization.yaml


 # Value of this field is prepended to the
 # names of all resources, e.g. a deployment named
 # "wordpress" becomes "alices-wordpress".
 # Note that it should also match with the prefix (text before '-') of the namespace
 # field above.
-# namePrefix: spanner-autoscaler-
+namePrefix: spanner-autoscaler-


Is this change required?

This was enabled by default. I think it is good because this makes it easier to identify the resources created for this controller specifically. If multiple operators/controllers are deployed in the same namespace, then this really helps in distinguishing resources quickly.

tjun · 2021-09-10T11:04:59Z

config/default/kustomization.yaml

@@ -1,12 +1,12 @@
 # Adds namespace to all resources.
-namespace: spanner-autoscaler
+namespace: spanner-autoscaler-system


Is this change required?

This was auto generated. I have now reverted it to the original one.

Signed-off-by: rustyclock <rustyclock@protonmail.com>

apstndb · 2021-09-13T07:07:00Z

config/manager/manager.yaml

        resources:
          limits:
            cpu: 100m
            memory: 30Mi
          requests:
            cpu: 100m
            memory: 20Mi
-      terminationGracePeriodSeconds: 20
+      serviceAccountName: controller-manager


It may require to update Workload Identity and some RBAC configurations written in README.md.

Thanks, fixed it! I think having a helm-chart would greatly simplify the deployment process here. Currently it requires too many files to be manually created and certain lines have to be searched and modified. I will see if kpt can help with that or helm.

Signed-off-by: rustyclock <rustyclock@protonmail.com>

tkuchiki · 2021-09-21T06:21:54Z

.dockerignore

-bin/
-config/
-.git/
-hack/
-.gitignore
-LICENSE
-Makefile
-PROJECT
-README.md


The following configures seem to be correct.

$ cat .dockerignore # More info: https://docs.docker.com/engine/reference/builder/#dockerignore-file # Ignore all files ** # Except go related files !pkg !cmd !**/*.mod !**/*.sum

You can check the following command.

$ docker image build -t build-context -f - . <<EOF FROM busybox COPY . /build-context WORKDIR /build-context CMD find . EOF $ docker container run --rm build-context . ./go.sum ./cmd ./cmd/spanner-autoscaler ./cmd/spanner-autoscaler/main.go ./pkg ./pkg/pointer ./pkg/pointer/pointer.go ./pkg/pointer/pointer_test.go ./pkg/api ./pkg/api/v1alpha1 ./pkg/api/v1alpha1/zz_generated.deepcopy.go ./pkg/api/v1alpha1/spannerautoscaler_types.go ./pkg/api/v1alpha1/groupversion_info.go ./pkg/metrics ./pkg/metrics/metrics.go ./pkg/metrics/fake.go ./pkg/controllers ./pkg/controllers/spannerautoscaler_controller.go ./pkg/controllers/spannerautoscaler_controller_test.go ./pkg/spanner ./pkg/spanner/spanner.go ./pkg/spanner/fake.go ./pkg/logging ./pkg/logging/context.go ./pkg/syncer ./pkg/syncer/fake ./pkg/syncer/fake/fake.go ./pkg/syncer/syncer_test.go ./pkg/syncer/syncer.go ./go.mod

!**/*.go doesn't work, so I choose !pkg and !cmd.

Dockerfile

Signed-off-by: rustyclock <rustyclock@protonmail.com>

tkuchiki · 2021-10-13T08:16:14Z

Makefile

-.PHONY: lint/golangci-lint
-lint/golangci-lint: tools/golangci-lint .golangci.yml  ## Run golangci-lint.
-	golangci-lint run
-
-# Build the docker image
-docker-build:
-	docker image build . -t ${IMG}
-
-# Push the docker image
-docker-push:
-	docker image push ${IMG}
-
-kubebuilder: ${KUBEBUILDER_ASSETS}/kubebuilder
-${KUBEBUILDER_ASSETS}/kubebuilder:
-	@{ \
-		set -e ;\
-		TMP_DIR=./tmp/kubebuilder ;\
-		mkdir -p $$TMP_DIR ;\
-		curl -sSL https://github.com/kubernetes-sigs/kubebuilder/releases/download/v${KUBEBUILDER_VERSION}/kubebuilder_${KUBEBUILDER_VERSION}_${GOOS}_${GOARCH}.tar.gz | tar -xz -C $$TMP_DIR ;\
-		mv $$TMP_DIR/kubebuilder_${KUBEBUILDER_VERSION}_${GOOS}_${GOARCH} ${KUBEBUILDER_ASSETS_DIR} ;\
-		rm -rf $$TMP_DIR ;\
-	}
-
-.PHONY: run
-run: $(SKAFFOLD)
-	# NOTE: since skaffold uses kind executable from PATH directly, so this overrides PATH to use project local executable.
-	@PATH=$${PWD}/bin:$${PATH} $(SKAFFOLD) dev --filename=./skaffold/skaffold.yaml
-
-.PHONY: manifests
-manifests: $(CONTROLLER_GEN)
-	@$(CONTROLLER_GEN) crd:trivialVersions=true rbac:roleName=manager-role webhook paths="./pkg/api/..." output:crd:artifacts:config=config/crd/bases
-
-.PHONY: deepcopy
-deepcopy: $(CONTROLLER_GEN)
-	 @$(CONTROLLER_GEN) object:headerFile=./hack/boilerplate.go.txt paths="./..."
-
-controlller-gen: $(CONTROLLER_GEN)
-$(CONTROLLER_GEN): go.sum
-	@go build -o $(CONTROLLER_GEN) sigs.k8s.io/controller-tools/cmd/controller-gen
-
-kind: $(KIND)
-$(KIND): go.sum
-	@go build -o ./bin/kind sigs.k8s.io/kind
-
-kubectl: $(KUBECTL)
-$(KUBECTL): .kubectl-version
-	@curl -Lso $(KUBECTL) https://storage.googleapis.com/kubernetes-release/release/$(shell cat .kubectl-version)/bin/$(GOOS)/$(GOARCH)/kubectl
-	@chmod +x $(KUBECTL)
-
-skaffold: $(SKAFFOLD)
-$(SKAFFOLD): .skaffold-version
-	@curl -Lso $(SKAFFOLD) https://storage.googleapis.com/skaffold/releases/$(shell cat .skaffold-version)/skaffold-$(GOOS)-$(GOARCH)
-	@chmod +x $(SKAFFOLD)
-
-kpt: $(KPT)
-$(KPT): go.sum
-	@go build -o $(KPT) github.com/GoogleContainerTools/kpt
-
-.PHONY: kind-cluster
-kind-cluster: $(KIND) $(KUBECTL)
-	@$(KIND) delete cluster --name spanner-autoscaler
-	@$(KIND) create cluster --name spanner-autoscaler --image kindest/node:v${KIND_NODE_VERSION}
-	@$(KUBECTL) apply -f ./config/crd/bases/spanner.mercari.com_spannerautoscalers.yaml
-	@$(KUBECTL) apply -f ./kind/namespace.yaml
-	@$(KUBECTL) apply -f ./kind/rbac
-
-.PHONY: kpt/crd
-kpt/crd: manifests kpt
-	@kustomize build config/crd > ./kpt/crd/spannerautoscalers.spanner.mercari.com.yaml


Is there any reason why you removed these make tasks?

Ref
This is the current status of the Makefile:

The current kpt package was anyways broken (Upgrade and fix broken kpt package files #41), so I didn't bother on porting it over yet. I will fix it along with kpt upgrade in Kpt package upgrade #43.

As for others:

kubectl: I think we don't need to install it on the developer's system because this is a basic tool for interaction with k8s and is expected to be present already.

kind: Same as above. I think any k8s developer would already have access to some kind of development k8s environment (be it kind or any other k8s setup of their choice).

kubebuilder: Same as above. Anyone building or modifying operators, would already have it on their system I think. We shouldn't burden ourselves with maintaining more code/components than what is/are necessary.

skaffold: I'm not sure about this one. Is this really used by any of the current code owners for development? If so, then I can port it.

Let me know what you think and which (or all) targets should be ported over.

Thanks!

kubectl

Yes, it may not be necessary.

kind

I agree that installation is not necessary. However, I think it would be more developer-friendly to not leave out make tasks that make it easier to build a local environment using kind.

kubebuilder

Yes, it may not be necessary.

skaffold

I have never used it. Does anyone use it? @mercari/spanner-autoscaler

The current kpt package was anyways broken (Upgrade and fix broken kpt package files #41), so I didn't bother on porting it over yet. I will fix it along with kpt upgrade in Kpt package upgrade #43.

OK!

I have added the kind target now. Please have a look. Thanks!

tkuchiki · 2021-10-15T03:38:17Z

cmd/spanner-autoscaler/main.go

+	metricsAddr          = flag.String("metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
+	probeAddr            = flag.String("health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")


If you don't have a strong motivation, it might be better to use the same flags and ports.

These were the default values generated by kubebuilder v3. These port numbers are also configured in the k8s manifests automatically, so instead of changing them here and then everywhere else, I just kept the default ones. Same goes for the flags as well.
We can revert to the old ones, but I think it is better to move closer to the default kubebuilder configuration for easier maintenance and migration in future.

Ref: https://book.kubebuilder.io/migration/manually_migration_guide_v2_v3.html#rename-the-manager-flags

I understood, thanks!
Also, this change might not be a problem because people will probably use the manifest file generated by kustomize.

tkuchiki · 2021-10-15T08:29:30Z

config/default/manager_auth_proxy_patch.yaml

 apiVersion: apps/v1
 kind: Deployment
 metadata:
-  name: spanner-autoscaler-manager
+  name: controller-manager


It might be friendly to write the migration steps in the README(or another documentation) if you want to change the name.

And the same for other resources.

* Add targets for creating or resetting a `kind` cluster for development * Fix the RBAC resources which are created by `kind-cluster` target for development Signed-off-by: rustyclock <rustyclock@protonmail.com>

Signed-off-by: rustyclock <rustyclock@protonmail.com>

github-actions · 2021-10-29T03:41:04Z

The new version v0.2.0 has been released 🎉

Changes: v0.1.5...v0.2.0

rustycl0ck requested a review from a team as a code owner September 10, 2021 07:03

github-actions bot added the size/XXL label Sep 10, 2021

rustycl0ck force-pushed the master branch from 620b730 to 9e95aab Compare September 10, 2021 07:19

rustycl0ck marked this pull request as draft September 10, 2021 08:08

Bump kubebuilder version from v2 to v3

3680ffe

Signed-off-by: rustyclock <rustyclock@protonmail.com>

rustycl0ck force-pushed the master branch from 9e95aab to 3680ffe Compare September 10, 2021 09:00

rustycl0ck commented Sep 10, 2021

View reviewed changes

rustycl0ck marked this pull request as ready for review September 10, 2021 09:09

tjun reviewed Sep 10, 2021

View reviewed changes

Fixes for code review

abc08ba

Signed-off-by: rustyclock <rustyclock@protonmail.com>

apstndb reviewed Sep 13, 2021

View reviewed changes

Fix service account name in README

80d4bd9

Signed-off-by: rustyclock <rustyclock@protonmail.com>

rustycl0ck requested a review from tjun September 14, 2021 01:00

rustycl0ck mentioned this pull request Sep 17, 2021

Kpt package upgrade #43

Closed

tkuchiki reviewed Sep 21, 2021

View reviewed changes

Fix .dockerignore exclusion syntax

6da13d4

Signed-off-by: rustyclock <rustyclock@protonmail.com>

rustycl0ck requested a review from tkuchiki October 6, 2021 05:23

tkuchiki reviewed Oct 13, 2021

View reviewed changes

tkuchiki reviewed Oct 15, 2021

View reviewed changes

rustycl0ck added 2 commits October 21, 2021 12:27

Add kind targets to Makefile

cf93c7c

* Add targets for creating or resetting a `kind` cluster for development * Fix the RBAC resources which are created by `kind-cluster` target for development Signed-off-by: rustyclock <rustyclock@protonmail.com>

Add migration warning for old verison

fc6f132

Signed-off-by: rustyclock <rustyclock@protonmail.com>

rustycl0ck force-pushed the master branch from d501dff to fc6f132 Compare October 21, 2021 03:27

rustycl0ck added the release/minor Indicates minor update to action-release-label label Oct 28, 2021

tkuchiki approved these changes Oct 29, 2021

View reviewed changes

tkuchiki merged commit 0b4381e into mercari:master Oct 29, 2021

rustycl0ck mentioned this pull request Nov 16, 2021

Upgrade CustomResourceDefinition to apiextensions.k8s.io/v1 #33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump kubebuilder version from `v2` to `v3` #40

Bump kubebuilder version from `v2` to `v3` #40

rustycl0ck commented Sep 10, 2021 •

edited

rustycl0ck commented Sep 10, 2021

rustycl0ck Sep 10, 2021

tjun Sep 10, 2021

rustycl0ck Sep 10, 2021 •

edited

tkuchiki Sep 21, 2021 •

edited

rustycl0ck Sep 22, 2021

tjun Sep 10, 2021

rustycl0ck Sep 10, 2021

tjun Sep 10, 2021

rustycl0ck Sep 10, 2021

apstndb Sep 13, 2021 •

edited

rustycl0ck Sep 13, 2021

tkuchiki Sep 21, 2021 •

edited

tkuchiki Oct 13, 2021

rustycl0ck Oct 13, 2021

tkuchiki Oct 15, 2021

rustycl0ck Oct 21, 2021

tkuchiki Oct 15, 2021

rustycl0ck Oct 15, 2021

tkuchiki Oct 15, 2021

tkuchiki Oct 15, 2021

rustycl0ck Oct 21, 2021

github-actions bot commented Oct 29, 2021

		metricsAddr = flag.String("metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
		probeAddr = flag.String("health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")

Bump kubebuilder version from v2 to v3 #40

Bump kubebuilder version from v2 to v3 #40

Conversation

rustycl0ck commented Sep 10, 2021 • edited

What this PR does / Why we need it

Which issue(s) this PR fixes

rustycl0ck commented Sep 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustycl0ck Sep 10, 2021 • edited

Choose a reason for hiding this comment

tkuchiki Sep 21, 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

apstndb Sep 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkuchiki Sep 21, 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

github-actions bot commented Oct 29, 2021

Bump kubebuilder version from `v2` to `v3` #40

Bump kubebuilder version from `v2` to `v3` #40

rustycl0ck commented Sep 10, 2021 •

edited

rustycl0ck Sep 10, 2021 •

edited

tkuchiki Sep 21, 2021 •

edited

apstndb Sep 13, 2021 •

edited

tkuchiki Sep 21, 2021 •

edited