STOR-1462: CRD manifests should be generated automatically

[dobsonj]

https://issues.redhat.com/browse/STOR-1462

I did quite a lot of cleanup work in this PR outside the scope of the original ticket. It's best to review the commits individually. I left them separate to make reviews easier, and I can split some of these changes out if needed.

The intent overall is to make it easier to contribute to LSO and remove artifacts that are not needed for building, developing, or testing LSO.

Some overall themes:

• Building bundles has long been error prone, requiring manual steps. Now you can just make bundle REGISTRY=quay.io/username and that's it. See HACKING.md for more details.
• Makefile was full of cruft, and there were duplicate manifests, unused manifests, and outdated examples. I removed anything that was not obviously useful.
• The remaining manifests in config/crd and config/rbac are generated now by make update. The generated CRD's are included in the bundle. The generated RBAC rules are incomplete, so the manifests in config/rbac are there for reference only right now. We should do some follow-up work on this in another PR.
• make verify ensures we don't forget to run make update -- I'll add a verify presubmit job after this PR merges. I fixed up all verify errors so it runs cleanly.
• I moved all go code to either pkg/ cmd/ or test/ and updated all package references. This is just to make the code organization easier to reason through and be consistent with other operators.

cc @openshift/storage

[openshift-ci-robot]

@dobsonj: This pull request references STOR-1462 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

https://issues.redhat.com/browse/STOR-1462

cc @openshift/storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dobsonj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dobsonj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dobsonj]

image-registry.openshift-image-registry.svc:5000/ci-op-z4j69dnp/pipeline: received unexpected HTTP status: 500 Internal Server Error
/retest-required

[mpatlasov]

Last commit ("replace hack/sync_bundle, fix make bundle, remove dead dockerfiles") would collide with Roman's #410. @RomanBednar , you re-wokred sync_bundle long time ago (in June?). Jonathan's bash script ("create-bundle") looks more readable than go version (for my eyes). Do you have any preferences? (maybe your go prog does something important that bash script misses?)

[mpatlasov]

Side note for reviewers: the first commit ("STOR-1462: CRD manifests should be generated automatically") modifies four yaml-s in config/crd/bases/. These changes didn't come from human, these files were auto-generated by newer version of controller-gen and its results differ from the older version of controller-gen. (somehow it took long while for me to discover that)

[openshift-ci-robot]

@dobsonj: This pull request references STOR-1462 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

https://issues.redhat.com/browse/STOR-1462

I did quite a lot of cleanup work in this PR outside the scope of the original ticket. It's best to review the commits individually. I left them separate to make reviews easier, and I can split some of these changes out if needed.

• STOR-1462: CRD manifests should be generated automatically
• clean up Makefile, add verify scripts
• remove unused kustomize crd patches
• remove dead manifests under config dir
• replace hack/sync_bundle, fix make bundle, remove dead dockerfiles
• remove unused rbac manifests from config dir
• remove outdated configmap examples
• add go vet to verify target and fix go vet errors
• move go code under pkg/ cmd/ or test/
• update package locations after code move

The intent overall is to make it easier to contribute to LSO and remove artifacts that are not needed for building, developing, or testing LSO.

Some overall themes:

• Building bundles has long been error prone, requiring manual steps. Now you can just make bundle REGISTRY=quay.io/username and that's it. See HACKING.md for more details.
• Makefile was full of cruft, and there were duplicate manifests, unused manifests, and outdated examples. I removed anything that was not obviously useful.
• The remaining manifests in config/crd and config/rbac are generated now by make update. The generated CRD's are included in the bundle. The generated RBAC rules are incomplete, so the manifests in config/rbac are there for reference only right now. We should do some follow-up work on this in another PR.
• make verify ensures we don't forget to run make update -- I'll add a verify presubmit job after this PR merges. I fixed up all verify errors so it runs cleanly.
• I moved all go code to either pkg/ cmd/ or test/ and updated all package references. This is just to the code organization easier to reason through and be similar to other operators.

cc @openshift/storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@dobsonj: This pull request references STOR-1462 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

https://issues.redhat.com/browse/STOR-1462

I did quite a lot of cleanup work in this PR outside the scope of the original ticket. It's best to review the commits individually. I left them separate to make reviews easier, and I can split some of these changes out if needed.

• STOR-1462: CRD manifests should be generated automatically
• clean up Makefile, add verify scripts
• remove unused kustomize crd patches
• remove dead manifests under config dir
• replace hack/sync_bundle, fix make bundle, remove dead dockerfiles
• remove unused rbac manifests from config dir
• remove outdated configmap examples
• add go vet to verify target and fix go vet errors
• move go code under pkg/ cmd/ or test/
• update package locations after code move

The intent overall is to make it easier to contribute to LSO and remove artifacts that are not needed for building, developing, or testing LSO.

Some overall themes:

• Building bundles has long been error prone, requiring manual steps. Now you can just make bundle REGISTRY=quay.io/username and that's it. See HACKING.md for more details.
• Makefile was full of cruft, and there were duplicate manifests, unused manifests, and outdated examples. I removed anything that was not obviously useful.
• The remaining manifests in config/crd and config/rbac are generated now by make update. The generated CRD's are included in the bundle. The generated RBAC rules are incomplete, so the manifests in config/rbac are there for reference only right now. We should do some follow-up work on this in another PR.
• make verify ensures we don't forget to run make update -- I'll add a verify presubmit job after this PR merges. I fixed up all verify errors so it runs cleanly.
• I moved all go code to either pkg/ cmd/ or test/ and updated all package references. This is just to the code organization easier to reason through and be similar to other operators.

cc @openshift/storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@dobsonj: This pull request references STOR-1462 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

https://issues.redhat.com/browse/STOR-1462

I did quite a lot of cleanup work in this PR outside the scope of the original ticket. It's best to review the commits individually. I left them separate to make reviews easier, and I can split some of these changes out if needed.

• CRD manifests should be generated automatically
• clean up Makefile, add verify scripts
• remove unused kustomize crd patches
• remove dead manifests under config dir
• replace hack/sync_bundle, fix make bundle, remove dead dockerfiles
• remove unused rbac manifests from config dir
• remove outdated configmap examples
• add go vet to verify target and fix go vet errors
• move go code under pkg/ cmd/ or test/
• update package locations after code move

The intent overall is to make it easier to contribute to LSO and remove artifacts that are not needed for building, developing, or testing LSO.

Some overall themes:

• Building bundles has long been error prone, requiring manual steps. Now you can just make bundle REGISTRY=quay.io/username and that's it. See HACKING.md for more details.
• Makefile was full of cruft, and there were duplicate manifests, unused manifests, and outdated examples. I removed anything that was not obviously useful.
• The remaining manifests in config/crd and config/rbac are generated now by make update. The generated CRD's are included in the bundle. The generated RBAC rules are incomplete, so the manifests in config/rbac are there for reference only right now. We should do some follow-up work on this in another PR.
• make verify ensures we don't forget to run make update -- I'll add a verify presubmit job after this PR merges. I fixed up all verify errors so it runs cleanly.
• I moved all go code to either pkg/ cmd/ or test/ and updated all package references. This is just to the code organization easier to reason through and be similar to other operators.

cc @openshift/storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@dobsonj: This pull request references STOR-1462 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

https://issues.redhat.com/browse/STOR-1462

I did quite a lot of cleanup work in this PR outside the scope of the original ticket. It's best to review the commits individually. I left them separate to make reviews easier, and I can split some of these changes out if needed.

The intent overall is to make it easier to contribute to LSO and remove artifacts that are not needed for building, developing, or testing LSO.

Some overall themes:

• Building bundles has long been error prone, requiring manual steps. Now you can just make bundle REGISTRY=quay.io/username and that's it. See HACKING.md for more details.
• Makefile was full of cruft, and there were duplicate manifests, unused manifests, and outdated examples. I removed anything that was not obviously useful.
• The remaining manifests in config/crd and config/rbac are generated now by make update. The generated CRD's are included in the bundle. The generated RBAC rules are incomplete, so the manifests in config/rbac are there for reference only right now. We should do some follow-up work on this in another PR.
• make verify ensures we don't forget to run make update -- I'll add a verify presubmit job after this PR merges. I fixed up all verify errors so it runs cleanly.
• I moved all go code to either pkg/ cmd/ or test/ and updated all package references. This is just to the code organization easier to reason through and be similar to other operators.

cc @openshift/storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dobsonj]

I just pushed several new commits and updated the PR description, but I think I'm done updating this now (aside from addressing review comments).

I'm partial to the create-bundle bash script because:

• It's a lightly modified version of what we have for EFS, filestore, and secrets-store. i.e. it's consistent with all our other OLM operators.
• It introduces very little code and no external dependencies aside from opm and docker / podman. The less code we have to maintain, the better.

[openshift-ci-robot]

@dobsonj: This pull request references STOR-1462 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

https://issues.redhat.com/browse/STOR-1462

I did quite a lot of cleanup work in this PR outside the scope of the original ticket. It's best to review the commits individually. I left them separate to make reviews easier, and I can split some of these changes out if needed.

The intent overall is to make it easier to contribute to LSO and remove artifacts that are not needed for building, developing, or testing LSO.

Some overall themes:

• Building bundles has long been error prone, requiring manual steps. Now you can just make bundle REGISTRY=quay.io/username and that's it. See HACKING.md for more details.
• Makefile was full of cruft, and there were duplicate manifests, unused manifests, and outdated examples. I removed anything that was not obviously useful.
• The remaining manifests in config/crd and config/rbac are generated now by make update. The generated CRD's are included in the bundle. The generated RBAC rules are incomplete, so the manifests in config/rbac are there for reference only right now. We should do some follow-up work on this in another PR.
• make verify ensures we don't forget to run make update -- I'll add a verify presubmit job after this PR merges. I fixed up all verify errors so it runs cleanly.
• I moved all go code to either pkg/ cmd/ or test/ and updated all package references. This is just to the code organization easier to reason through and be similar to other operators.

cc @openshift/storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@dobsonj: This pull request references STOR-1462 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

https://issues.redhat.com/browse/STOR-1462

I did quite a lot of cleanup work in this PR outside the scope of the original ticket. It's best to review the commits individually. I left them separate to make reviews easier, and I can split some of these changes out if needed.

The intent overall is to make it easier to contribute to LSO and remove artifacts that are not needed for building, developing, or testing LSO.

Some overall themes:

• Building bundles has long been error prone, requiring manual steps. Now you can just make bundle REGISTRY=quay.io/username and that's it. See HACKING.md for more details.
• Makefile was full of cruft, and there were duplicate manifests, unused manifests, and outdated examples. I removed anything that was not obviously useful.
• The remaining manifests in config/crd and config/rbac are generated now by make update. The generated CRD's are included in the bundle. The generated RBAC rules are incomplete, so the manifests in config/rbac are there for reference only right now. We should do some follow-up work on this in another PR.
• make verify ensures we don't forget to run make update -- I'll add a verify presubmit job after this PR merges. I fixed up all verify errors so it runs cleanly.
• I moved all go code to either pkg/ cmd/ or test/ and updated all package references. This is just to make the code organization easier to reason through and be consistent with other operators.

cc @openshift/storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mpatlasov]

ci/prow/e2e-operator-extended is perma-failing:

: Run multi-stage test e2e-operator-extended - e2e-operator-extended-openshift-extended-test container test expand_less | 11m17s
-- | --
{  e8x Sep 30 00:12:56.122 W persistentvolume/local-pv-eef239ed Error cleaning PV "local-pv-eef239ed": Unknown storage class name lvsc-j0g3se8x (2 times) Sep 30 00:13:09.406 W persistentvolume/local-pv-eef239ed Error cleaning PV "local-pv-eef239ed": Unknown storage class name lvsc-j0g3se8x (3 times) Sep 30 00:13:09.410 I persistentvolume/local-pv-f0173d8c Starting cleanup of Block PV "local-pv-f0173d8c", this may take a while Sep 30 00:13:17.105 W persistentvolume/local-pv-f0173d8c Error cleaning PV "local-pv-f0173d8c": Unknown storage class name lvsc-t7kmmne9 Sep 30 00:13:17.111 W persistentvolume/local-pv-f0173d8c Error cleaning PV "local-pv-f0173d8c": Unknown storage class name lvsc-t7kmmne9 (2 times) Sep 30 00:13:17.121 W persistentvolume/local-pv-f0173d8c Error cleaning PV "local-pv-f0173d8c": Unknown storage class name lvsc-t7kmmne9 (3 times) Sep 30 00:13:17.153 W persistentvolume/local-pv-f0173d8c Error cleaning PV "local-pv-f0173d8c": Unknown storage class name lvsc-t7kmmne9 (4 times) Sep 30 00:13:17.158 W persistentvolume/local-pv-f0173d8c Error cleaning PV "local-pv-f0173d8c": Unknown storage class name lvsc-t7kmmne9 (5 times) Sep 30 00:14:07.643 I persistentvolume/local-pv-eef239ed Starting cleanup of Block PV "local-pv-eef239ed", this may take a while Sep 30 00:14:17.671 I persistentvolume/local-pv-f0173d8c Starting cleanup of Block PV "local-pv-f0173d8c", this may take a while  Writing JUnit report to /logs/artifacts/junit/junit_e2e_20230930-001453.xml  8 pass, 0 skip (10m47s) + set +x try to handle result /logs/artifacts/junit/junit_e2e_20230930-001453.xml /logs/artifacts/junit/junit_e2e_2023-09-30-00-14-53.xml done to handle result success force success exit {"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"k8s.io/test-infra/prow/entrypoint/run.go:84","func":"k8s.io/test-infra/prow/entrypoint.Options.internalRun","level":"error","msg":"Error executing test process","severity":"error","time":"2023-09-30T00:14:53Z"} error: failed to execute wrapped command: exit status 1 }

(https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_local-storage-operator/433/pull-ci-openshift-local-storage-operator-master-e2e-operator-extended/1707897311264247808)

See also this comment: #435 (comment)

[mpatlasov]

/test e2e-operator-extended

[dobsonj]

latest push is just a rebase after #430 merged

[dobsonj]

/retest-required

[dobsonj]

/retest-required

[dobsonj]

/retest-required

[RomanBednar]

Last commit ("replace hack/sync_bundle, fix make bundle, remove dead dockerfiles") would collide with Roman's #410. @RomanBednar , you re-wokred sync_bundle long time ago (in June?). Jonathan's bash script ("create-bundle") looks more readable than go version (for my eyes). Do you have any preferences? (maybe your go prog does something important that bash script misses?)

@mpatlasov That is correct, thanks for pointing that out. Both scripts are essentially the same, the aim is to replace the error prone script that we had. There are a few differences:

• checking for opm binary - without it this script will build images (which takes a while) and can fail when it tries to run opm if not installed - this can be a bit annoying so I had a check for that
• checking container engine - this script is better that it considers both docker and podman
• this script is for creating bundle and index only, LSO images are built and pushed via makefile. I intended to have standalone script that does everything on one place - this is a matter of preference
• with this version you're going to need 5 different quay.io namespaces set to be public (local-storage-operator, local-storage-diskmaker, etc...). I assumed that most of us use just one for testing and use tag for naming instead of version - again, not too important
• sed is platform specific, so that's one advantage of not using bash, on macOS I get this when I run the script:

sed: -I or -i may not be used with stdin
make: *** [bundle] Error 1

Bash is way more readable and consistent with other repos, so we can stick with that (maybe with some small improvements).

[mpatlasov]

@dobsonj , the commit b654e701f ("remove unused kustomize crd patches") looks reasonable, but how did you find out that nobody uses that kustomize stuff?

The same question is about next commit -- 5b88fc0 ("remove dead manifests under config dir")

[mpatlasov]

@dobsonj , why are we interested in BSD at all? (3bb170c: create-bundle: add opm check and fix sed command on BSD)

[mpatlasov]

Minor nit: last commit (34e906c: address review comments, remove mustgather from make bundle) is better to squash with previous ones. (Future code readers will be puzzled why you introduced mustgather in hack/create-bundle to remove it very soon)

[dobsonj]

@dobsonj , the first commit in this PR replaces 4 yaml-s we had in config/manifests/stable/ with 4 yaml-s auto-generated by new manifests target. I attempted to compare what we had vs. what we generate now and it's really cumbersome because both indentation and order changed. However, I discovered one discrepancy:

Older config/manifests/stable/local-volume-discoveries.crd.yaml had:

          required:
              - spec

in spec.versions[0].schema.openAPIV3Schema while newer config/crd/bases/local.storage.openshift.io_localvolumediscoveries.yaml doesn't have it. Two questions about it:

1. How did you decide that that discrepancy is unimportant?
2. How did you compare those 4 yaml-s (by eyes or by some tooling)?

I compared the yamls only with my eyes and otherwise relied on testing. There are a lot of formatting differences, but I did not identify any functional differences.

@dobsonj , the commit b654e701f ("remove unused kustomize crd patches") looks reasonable, but how did you find out that nobody uses that kustomize stuff?

The same question is about next commit -- 5b88fc0 ("remove dead manifests under config dir")

The kustomize patches were mostly boilerplate manifests generated by operator-sdk, and they were never used to generate the manifests that get deployed as part of the bundle.

I grepped for each filename to find any existing references in the repo, and also looked for these objects on a cluster with LSO deployed. If they are not referenced by any code, makefile targets, or docs and they are not deployed on the cluster and they do not have a clearly defined purpose, then there isn't much reason to keep them.

@dobsonj , why are we interested in BSD at all? (3bb170c: create-bundle: add opm check and fix sed command on BSD)

We're not interested in BSD per se, but this commit was to fix the issue Roman pointed out when running create-bundle.sh on MacOS (which uses the BSD version of sed). I squashed that commit though in my latest push, so it's part of the create-bundle commit now.

Minor nit: last commit (34e906c: address review comments, remove mustgather from make bundle) is better to squash with previous ones. (Future code readers will be puzzled why you introduced mustgather in hack/create-bundle to remove it very soon)

Fixed, I squashed related commits.

@mpatlasov @RomanBednar thanks very much for the reviews, I think I addressed outstanding comments.

[RomanBednar]

/lgtm

I've retested mainly the bundle target and it works like a charm for me now, thanks!

[dobsonj]

no end-user impact or doc changes
/label px-approved
/label docs-approved

[Phaow]

Go through the commits, looks good. Also CI results looks good.
/label qe-approved

[openshift-ci-robot]

@dobsonj: This pull request references STOR-1462 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

https://issues.redhat.com/browse/STOR-1462

I did quite a lot of cleanup work in this PR outside the scope of the original ticket. It's best to review the commits individually. I left them separate to make reviews easier, and I can split some of these changes out if needed.

The intent overall is to make it easier to contribute to LSO and remove artifacts that are not needed for building, developing, or testing LSO.

Some overall themes:

• Building bundles has long been error prone, requiring manual steps. Now you can just make bundle REGISTRY=quay.io/username and that's it. See HACKING.md for more details.
• Makefile was full of cruft, and there were duplicate manifests, unused manifests, and outdated examples. I removed anything that was not obviously useful.
• The remaining manifests in config/crd and config/rbac are generated now by make update. The generated CRD's are included in the bundle. The generated RBAC rules are incomplete, so the manifests in config/rbac are there for reference only right now. We should do some follow-up work on this in another PR.
• make verify ensures we don't forget to run make update -- I'll add a verify presubmit job after this PR merges. I fixed up all verify errors so it runs cleanly.
• I moved all go code to either pkg/ cmd/ or test/ and updated all package references. This is just to make the code organization easier to reason through and be consistent with other operators.

cc @openshift/storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci]

@dobsonj: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[openshift-bot]

[ART PR BUILD NOTIFIER]

This PR has been included in build local-storage-operator-container-v4.15.0-202311130831.p0.g39fe975.assembly.stream for distgit local-storage-operator.
All builds following this will include this PR.