Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build(operator-sdk): upgrade operator sdk version to 1.22.2 #430

Merged
merged 82 commits into from
Aug 5, 2022

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Jul 22, 2022

Fixes #287
Fixes #431
Related to #433

Upgraded operator-sdk to v1.22.2. (Actual) Changes for each version:

1.6.1:

  • Update tls configuration for metrics endpoint (though still disabled).
  • Add patches to delete cert mount volume (still commented out but added for completeness).
  • Add build-catalog target to build an index image.

1.7.1:

  • Add pipefail and non-zero status checks for SHELL in Makefile.
  • Move lease to its own proxy-rule rule.
  • Bump controller-runtime to v0.8.3 and k8s deps to v0.20.0.

1.8.0: No migration

1.9.0: No migration

1.10.0: No migration

1.11.0:

  • Add protocol for containerPort field in manifest (i.e. setting to TCP).

1.12.0: No migration

1.13.0: No migration

1.14.0:

  • Bump controller-gen to 0.7.0.
  • Bump controller-runtime to v0.10.0 and k8s deps to v0.22.1.
  • Fix unit tests for FakeClient release 0.9.0
  • Use the new envtest tool (controller-runtime to v0.10.0 removes setup-envtest.sh)
  • Fixed kustomize target in Makefile since go install does not work with Kustomize. Issue is filed here.
  • Use comment markers to mark deprecated API instead of patches since we upgrades past 1.10.*.

1.15.0: No migration

1.16.0:

  • Add annotation kubectl.kubernetes.io/default-container to specify default manager container.
  • Add PHONY for all targets
  • Add ignore-not-found flag for undeploy/uninstall targets.

1.17.0:

  • Bump controller-gen to 0.8.0.
  • Bump k8s deps to 0.23.0 and controller-runtime to 0.11.0.
  • Add resource request/limit to kube-rbac-proxy container.
  • Reduce debug log level of kube-rbac-proxy container from 10 to 0.
  • Fix new struct names after release refactoring (i.e. Handler -> ProbeHandler)
  • Fix unit tests after bumping to controller-runtime 0.11.0.

1.18.0:

  • Remove replace directive for gogo/protobuf since client-go new pulls 1.3.2.
  • Add support for bundle image digest instead of tags.

1.19.0:

  • Manager deployment entry in CSV now has label field added.

1.20.0: No migration

1.21.0:

  • Fix suite test to use global config.
  • Bump controller-runtime to 0.11.2 and k8s deps to 0.23.5.
  • Bump kube-rbac-proxy image to 0.11.0.

1.22.0:

  • Bump go version to 1.18.
  • Bump controller-gen to 0.9.0.
  • Bump controller-runtime to 0.12.1 and k8s deps to 0.24.0.

1.20.1: No migration

1.20.2: No migration

Others:

  • Use go install instead of go get (deprecated) to download and build binaries.
  • Bump ginkgo to v1.16.5 (having deprecated warnings - currently add env var to silence) and gomega to v1.18.1.
  • Since 1.22.0, go version requirement is bump to 1.18.
  • Fix spec.selector should be updated only on creation.
  • Update ci.yaml to use go 1.18.* and operator-sdk 1.22.2.

@tthvo
Copy link
Member Author

tthvo commented Jul 22, 2022

hi @ebaron #356 mentions deprecated apis can be specified as markers. Would I start on fixing these too now that we have migrated to at least v.0.13.0 now?

v0.14.0 breaks tests as they upgrades controller-runtime to 0.10.0 so I will look into that.

@ebaron
Copy link
Member

ebaron commented Jul 22, 2022

hi @ebaron #356 mentions deprecated apis can be specified as markers. Would I start on fixing these too now that we have migrated to at least v.0.13.0 now?

That's a good idea, but we're planning to remove those APIs before the next release. So not a big deal.

@tthvo
Copy link
Member Author

tthvo commented Jul 25, 2022

Hi @ebaron, in our unit tests, when Cryostat CR is being deleted, we call reconcileDeletedCryostat:

func (t *cryostatTestInput) reconcileDeletedCryostat() {

which, I think, adds a delete-timestamp and calls reconcile again to resolve finalizers.

Then, in test spec, we call check if finalizers are absent.

func (t *cryostatTestInput) expectCryostatFinalizerAbsent() {

But in this case, would cryostat CR already be deleted since its finalizers are resolved so the finalizer checks failed to get cryostat CR as here?

should remove the finalizer [It]
          /home/thvo/workspace/cryostat-operator/internal/controllers/cryostat_controller_test.go:969

          Unexpected error:
              <*errors.StatusError | 0xc000178aa0>: {
                  ErrStatus: {
                      TypeMeta: {Kind: "", APIVersion: ""},
                      ListMeta: {
                          SelfLink: "",
                          ResourceVersion: "",
                          Continue: "",
                          RemainingItemCount: nil,
                      },
                      Status: "Failure",
                      Message: "cryostats.operator.cryostat.io \"cryostat\" not found",
                      Reason: "NotFound",
                      Details: {
                          Name: "cryostat",
                          Group: "operator.cryostat.io",
                          Kind: "cryostats",
                          UID: "",
                          Causes: nil,
                          RetryAfterSeconds: 0,
                      },
                      Code: 404,
                  },
              }
              cryostats.operator.cryostat.io "cryostat" not found
          occurred

@tthvo
Copy link
Member Author

tthvo commented Jul 25, 2022

Release 0.9.0 for controller-runtime said:

Fakeclient: Handle Finalizers (kubernetes-sigs/controller-runtime#869)
moderate impact: Finalizers in the fakeclient now behave similiar to the kube api, i.E. an object with a finalizer will not actually be deleted upon Delete and Updating an object with a DeletionTimestamp to remove the finalizer will result in deletion

@ebaron
Copy link
Member

ebaron commented Jul 25, 2022

Release 0.9.0 for controller-runtime said:

Fakeclient: Handle Finalizers (kubernetes-sigs/controller-runtime#869)
moderate impact: Finalizers in the fakeclient now behave similiar to the kube api, i.E. an object with a finalizer will not actually be deleted upon Delete and Updating an object with a DeletionTimestamp to remove the finalizer will result in deletion

Ah, that explains the behaviour you saw then. In our existing tests, we would use the DeletionTimestamp to simulate the Cryostat CR being deleted, but nothing actually followed through with the deletion. I suppose now the tests should be updated to ensure the object is deleted instead of testing whether the finalizer is absent.

@tthvo
Copy link
Member Author

tthvo commented Jul 26, 2022

hi @ebaron, It looks like we update template.spec but not templace.metadata. Not sure if this is intentional?

deploy.Spec.Template.Spec = specCopy.Template.Spec

Fail test:

• Failure [0.005 seconds]
CryostatController
/home/thvo/workspace/cryostat-operator/internal/controllers/cryostat_controller_test.go:84
  reconciling a request in OpenShift
  /home/thvo/workspace/cryostat-operator/internal/controllers/cryostat_controller_test.go:121
    with an existing main Deployment
    /home/thvo/workspace/cryostat-operator/internal/controllers/cryostat_controller_test.go:272
      should update the Deployment [It]
      /home/thvo/workspace/cryostat-operator/internal/controllers/cryostat_controller_test.go:280

      Expected
          <string>: 
      to equal
          <string>: cryostat

      /home/thvo/workspace/cryostat-operator/internal/controllers/cryostat_controller_test.go:1939

@tthvo
Copy link
Member Author

tthvo commented Jul 27, 2022

It seems this only happens after bumping to controller-runtime 0.11.0. In case we expect the $.spec.template.metadata to not be updated, I add this to Deployment.Spec.Template in OtherDeployment method since I guess metadata was missing and not updated in reconciler for new deployment so the check failed.

Template: corev1.PodTemplateSpec{

ObjectMeta: metav1.ObjectMeta{
	Name:      "cryostat",
	Namespace: "default",
	Labels: map[string]string{
		"app":       "cryostat",
		"kind":      "cryostat",
		"component": "cryostat",
	},
}

But I wonder if we need to update their labels/annotations in $.spec.template.metadata in our reconciler similar to $.metadata as here? Any suggestions @ebaron?

mergeLabelsAndAnnotations(&deploy.ObjectMeta, metaCopy)

@tthvo tthvo requested review from ebaron and andrewazores July 27, 2022 18:58
@tthvo tthvo marked this pull request as ready for review July 27, 2022 18:58
@tthvo
Copy link
Member Author

tthvo commented Jul 27, 2022

With new upgrades, a cache directory is created when doing bundle deployment, not sure if we need to commit that so I add it into .gitignore.

@andrewazores
Copy link
Member

I did make generate manifests manager oci-build bundle and there are some changes to generated files:

$ git status
HEAD detached at thvo/upgrade-operator-sdk
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   bundle/manifests/operator.cryostat.io_cryostats.yaml
	modified:   bundle/manifests/operator.cryostat.io_flightrecorders.yaml
	modified:   bundle/manifests/operator.cryostat.io_recordings.yaml
	modified:   config/crd/bases/operator.cryostat.io_cryostats.yaml
	modified:   config/crd/bases/operator.cryostat.io_flightrecorders.yaml
	modified:   config/crd/bases/operator.cryostat.io_recordings.yaml
	modified:   config/rbac/role.yaml

no changes added to commit (use "git add" and/or "git commit -a")

git diff looks fine, so I think these are probably changes that should just be checked in and committed.

@tthvo
Copy link
Member Author

tthvo commented Jul 27, 2022

I did make generate manifests manager oci-build bundle and there are some changes to generated files:

$ git status
HEAD detached at thvo/upgrade-operator-sdk
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   bundle/manifests/operator.cryostat.io_cryostats.yaml
	modified:   bundle/manifests/operator.cryostat.io_flightrecorders.yaml
	modified:   bundle/manifests/operator.cryostat.io_recordings.yaml
	modified:   config/crd/bases/operator.cryostat.io_cryostats.yaml
	modified:   config/crd/bases/operator.cryostat.io_flightrecorders.yaml
	modified:   config/crd/bases/operator.cryostat.io_recordings.yaml
	modified:   config/rbac/role.yaml

no changes added to commit (use "git add" and/or "git commit -a")

git diff looks fine, so I think these are probably changes that should just be checked in and committed.

Oh hmm that's weird. There appears no changes on my end. I am using operator-sdk 1.22.2 and go 1.18. Any idea what happens?

@andrewazores
Copy link
Member

$ go version
go version go1.18.4 linux/amd64
$ operator-sdk version
operator-sdk version: "v1.22.2", commit: "da3346113a8a75e11225f586482934000504a60f", kubernetes version: "1.24.1", go version: "go1.18.4", GOOS: "linux", GOARCH: "amd64"

I'm in the middle of building and testing this PR as a bundle so I have some other changes that got made locally, modifying image tags and such. I'll post the diff of what I found earlier when this is out of the way.

@tthvo
Copy link
Member Author

tthvo commented Jul 27, 2022

Ohh would it be controller-gen out of date because I added a check in Makefile if controller-gen (same for other binaries) exists then no downloading executed. Right now, controller-gen is at 0.9.0?

@andrewazores
Copy link
Member

Good hunch, but

$ ./bin/controller-gen --version
Version: v0.9.0
$ ./bin/kustomize version
{Version:kustomize/v3.8.7 GitCommit:ad092cc7a91c07fdf63a2e4b7f13fa588a39af4f BuildDate:2020-11-11T23:14:14Z GoOs:linux GoArch:amd64}

These were freshly retrieved since this is my first time doing an operator build on my new laptop.

@tthvo
Copy link
Member Author

tthvo commented Jul 27, 2022

Oh myy actually it is my setup that is out of date! Still on 0.7.0 when testing lower version! I will update and check in these changes now!

@andrewazores
Copy link
Member

Cool, just manually testing out deploying things using this new build looks good. I didn't go too in-depth but I was able to install the operator via bundle, create a Cryostat CR with a configuration for a report sidecar, and that got deployed as usual. Then I edited the CR to remove the report sidecar and the deployment was adjusted accordingly.

@andrewazores
Copy link
Member

Oh myy actually it is my setup that is out of date! Still on 0.7.0 when testing lower version! I will update and check in these changes now!

Makes sense:

diff --git a/bundle/manifests/operator.cryostat.io_cryostats.yaml b/bundle/manifests/operator.cryostat.io_cryostats.yaml
index d2fa0f6..c55b204 100644
--- a/bundle/manifests/operator.cryostat.io_cryostats.yaml
+++ b/bundle/manifests/operator.cryostat.io_cryostats.yaml
@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.7.0
+    controller-gen.kubebuilder.io/version: v0.9.0

@andrewazores
Copy link
Member

Nice, after your latest commit all the generated files appear to be up-to-date for me too.

@ebaron
Copy link
Member

ebaron commented Jul 27, 2022

It seems this only happens after bumping to controller-runtime 0.11.0. In case we expect the $.spec.template.metadata to not be updated, I add this to Deployment.Spec.Template in OtherDeployment method since I guess metadata was missing and not updated in reconciler for new deployment so the check failed.

Template: corev1.PodTemplateSpec{

ObjectMeta: metav1.ObjectMeta{
	Name:      "cryostat",
	Namespace: "default",
	Labels: map[string]string{
		"app":       "cryostat",
		"kind":      "cryostat",
		"component": "cryostat",
	},
}

But I wonder if we need to update their labels/annotations in $.spec.template.metadata in our reconciler similar to $.metadata as here? Any suggestions @ebaron?

mergeLabelsAndAnnotations(&deploy.ObjectMeta, metaCopy)

I think this makes sense to do. In the future I would like the Deployment and Pod Template's labels and annotations to managed from the Cryostat CRD, but this seems like a good solution for now.

I'm still a bit confused what has changed in controller-runtime to cause this test failure.

@tthvo
Copy link
Member Author

tthvo commented Jul 27, 2022

Sure I will fix add the label and annotation merge for spec.template.metatadata now.

I'm still a bit confused what has changed in controller-runtime to cause this test failure.

The changelog could not really help :((

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.6.1:

* Update tls configuration for metrics endpoint (though still disabled).

I think we might want this (commented out) for completeness: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.6.1/#manifestsv2-add-a-kustomize-patch-to-remove-the-cert-manager-volumevolumemount-from-your-csv

@tthvo
Copy link
Member Author

tthvo commented Jul 27, 2022

Oh yes sure I might have been confused with our manually adding cert-manager. Will update now!

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Makefile addition would be useful as well: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.6.1/#gov2-gov3-ansiblev1-helmv1-add-opm-and-catalog-build-makefile-targets

We can skip the catalog-push target since we don't have any other *-push targets in our Makefile. We'd also want to change --container-tool docker to --container-tool podman. We also don't seem to have an IMAGE_TAG_BASE in our Makefile, but you can substitute it with $(IMAGE_NAMESPACE)/$(OPERATOR_NAME).

@ebaron
Copy link
Member

ebaron commented Jul 27, 2022

This Makefile addition would be useful as well: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.6.1/#gov2-gov3-ansiblev1-helmv1-add-opm-and-catalog-build-makefile-targets

We can skip the catalog-push target since we don't have any other *-push targets in our Makefile. We'd also want to change --container-tool docker to --container-tool podman. We also don't seem to have an IMAGE_TAG_BASE in our Makefile, but you can substitute it with $(IMAGE_NAMESPACE)/$(OPERATOR_NAME).

Ah, IMAGE_TAG_BASE is introduced in the next point: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.6.1/#gov2-gov3-ansiblev1-helmv1-changed-bundle_img-and-added-image_tag_base-makefile-variables

In our case, feel free to define: IMAGE_TAG_BASE ?= $(IMAGE_NAMESPACE)/$(OPERATOR_NAME) and replace instances of $(IMAGE_NAMESPACE)/$(OPERATOR_NAME) with ${IMAGE_TAG_BASE).

Makefile Outdated Show resolved Hide resolved
@tthvo tthvo force-pushed the upgrade-operator-sdk branch from ae31505 to ded5684 Compare August 3, 2022 21:41
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
config/manifests/remove_cert_manager_volume_patch.yaml Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
internal/controllers/cryostat_controller.go Outdated Show resolved Hide resolved
@ebaron
Copy link
Member

ebaron commented Aug 5, 2022

@tthvo I have to apologize. It seems the Makefile tool download targets are not working as I expected. Since they all have a prerequisite of $(LOCALBIN), the modification time of $(LOCALBIN) is changed whenever a new tool is downloaded. This means that unless the same tool is being downloaded twice in succession, $(LOCALBIN) will always appear newer than the already downloaded tool, and Make will download it again. This can cause a problem with the Kustomize tool in particular, which is an open bug in the Operator SDK: operator-framework/operator-sdk#5875

The fix coming upstream from Kubebuilder is very similar to what you had before, only using test -s instead: kubernetes-sigs/kubebuilder@a9bf7b9.

Could you add test -s ... to the Make recipes for controller-gen, kustomize, addlicense, envtest, and opm, as is done in the above Kubebuilder commit? Sorry again for getting this wrong.

@ebaron
Copy link
Member

ebaron commented Aug 5, 2022

The fix coming upstream from Kubebuilder is very similar to what you had before, only using test -s instead: kubernetes-sigs/kubebuilder@a9bf7b9.

Could you add test -s ... to the Make recipes for controller-gen, kustomize, addlicense, envtest, and opm, as is done in the above Kubebuilder commit? Sorry again for getting this wrong.

Actually this Kubebuilder fix looks odd. It seems like instead of using test -s $(LOCALBIN)/controller-gen they should use test -s $(CONTROLLER_GEN). This is because CONTROLLER_GEN and related variables can be overriden by the user.

@tthvo
Copy link
Member Author

tthvo commented Aug 5, 2022

@tthvo I have to apologize. It seems the Makefile tool download targets are not working as I expected. Since they all have a prerequisite of $(LOCALBIN), the modification time of $(LOCALBIN) is changed whenever a new tool is downloaded. This means that unless the same tool is being downloaded twice in succession, $(LOCALBIN) will always appear newer than the already downloaded tool, and Make will download it again. This can cause a problem with the Kustomize tool in particular, which is an open bug in the Operator SDK: operator-framework/operator-sdk#5875

The fix coming upstream from Kubebuilder is very similar to what you had before, only using test -s instead: kubernetes-sigs/kubebuilder@a9bf7b9.

Could you add test -s ... to the Make recipes for controller-gen, kustomize, addlicense, envtest, and opm, as is done in the above Kubebuilder commit? Sorry again for getting this wrong.

Ohh no problem at all! I just saw the exact same behavior when running it too and about to ask! I would just add it back with test then.

@tthvo
Copy link
Member Author

tthvo commented Aug 5, 2022

The fix coming upstream from Kubebuilder is very similar to what you had before, only using test -s instead: kubernetes-sigs/kubebuilder@a9bf7b9.
Could you add test -s ... to the Make recipes for controller-gen, kustomize, addlicense, envtest, and opm, as is done in the above Kubebuilder commit? Sorry again for getting this wrong.

Actually this Kubebuilder fix looks odd. It seems like instead of using test -s $(LOCALBIN)/controller-gen they should use test -s $(CONTROLLER_GEN). This is because CONTROLLER_GEN and related variables can be overriden by the user.

Right! I guess we can just use CONTROLLER_GEN then.

@ebaron
Copy link
Member

ebaron commented Aug 5, 2022

Right! I guess we can just use CONTROLLER_GEN then.

I think so, unless I'm missing something. Same for the other binaries.

@tthvo
Copy link
Member Author

tthvo commented Aug 5, 2022

I am thinking whether only LOCALBIN variable could be overrided by user instead of the var for the binaries themselves since we set GOBIN anyway? I guess in this case, for example, overriding CONTROLLER_GEN does not really affect where the binary is installed?

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! @tthvo thanks for taking on this substantial and overdue change!

@ebaron ebaron merged commit 3588058 into cryostatio:main Aug 5, 2022
@tthvo
Copy link
Member Author

tthvo commented Aug 5, 2022

Thanks a lot @ebaron for helping me in this! Its great to learn a lot more from this issue!

@ebaron
Copy link
Member

ebaron commented Aug 5, 2022

Thanks a lot @ebaron for helping me in this! Its great to learn a lot more from this issue!

I was glad to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

go-get-tool should install to $1 instead of $PROJECT_DIR/bin Upgrade Operator SDK to at least 1.8.2
3 participants