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

CONSOLE-3717: Console CRDs: Rename files to use the default run-level for updates #1541

Merged

Conversation

wking
Copy link
Member

@wking wking commented Aug 2, 2023

It's unclear what the motivation was for their existing runlevels, with the API filenames landing without much context:

$ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
862dd2fe4 Add console-operator CRD and generate from types
$ git --no-pager log -1 --format='%h %B' --stat=80 862dd2fe4
862dd2fe4 Add console-operator CRD and generate from types
    
 operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
 1 file changed, 195 insertions(+)
$ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
a4be43c2b Added ability to configure Helm chart repository accessible within cluster
$ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c2b
a4be43c2b Added ability to configure Helm chart repository accessible within cluster
    
* Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
  from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
* The corresponding enchancement: openshift/enhancements/pull/175
    
 Makefile                                           |   3 +-
 hack/update-deepcopy.sh                            |   2 +-
 hack/verify-crds.sh                                |   1 +
 .../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
 helm/v1alpha1/doc.go                               |   8 ++
 helm/v1alpha1/register.go                          |  38 ++++++
 helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
 helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
 8 files changed, 448 insertions(+), 2 deletions(-)
$ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
8fe639fe2 helm: add a new namespaced CRD for helm
$ git --no-pager log -1 --format='%h %B' --stat=80 8fe639fe2
8fe639fe2 helm: add a new namespaced CRD for helm
    
As part of the process to support namespace-scoped Helm repositories for
non-admin users, this PR adds a new namespace-scoped CRD definition
named `projecthelmchartrepositories.helm.openshift.io`.
    
Closes: https://issues.redhat.com/browse/HELM-258
Signed-off-by: Allen Bai <abai@redhat.com>
    
 .../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
 helm/v1beta1/register.go                           |   2 +
 ...ypes_helm.go => types_helm_chart_repository.go} |   0
 .../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
 helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
 5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in with the other console resources in the default runlevel 50 (docs, implementation).

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't matched the 0000_<runlevel>_<dash-separated-component>_<manifest_filename> template, and by sorting lexically between other level-70 manifests:

$ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
$ ls manifests | grep -1 0000_70_console-operator.crd.yaml
0000_70_cluster-network-operator_05_clusteroperator.yaml
0000_70_console-operator.crd.yaml
0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one large task-node. With this change removing the unparsable attempt at a runlevel, the network and DNS manifests will be able to apply on updates via two parallel task-nodes (screenshots)

I'm prefixing them with 00_ now to get them near the front of the other level-50 console manifests. For example, manifests/01-helm.yaml is declaring a HelmChartRepository resource, so we want it to sort after the HelmChartRepository CustomResourceDefinition within the console task-node.

But I'm prefixing ConsolePlugin with 90_ to sort it towards the back, after 07-operator.yaml, because the operator is what serves the /crdconvert webhook defined in that CRD, #1532.

Generated with:

$ rename 's/0000_[0-9][0-9]_/00_/' console/*/*.yaml helm/*/*.yaml operator/*/*console-operator*.yaml
$ sed -i 's/0000_[0-9][0-9]_/00_/' console/*/*testsuite.yaml helm/*/*.testsuite.yaml
$ sed -i 's/0000_[0-9][0-9]_\(.*console-operator\)/00_\1/' operator/*/*testsuite.yaml
$ rename 's/00_/90_/' console/*/*consoleplugin*.yaml
$ sed -i 's/00_/90_/' console/*/*consoleplugin*testsuite.yaml
$ git add -A console helm operator

Once this lands, I'll reroll openshift/console-operator#782 to vendor-bump openshift/api to pull in these changes (it currently just renames them in the Dockerfile COPYs).

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 2, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 2, 2023

@wking: This pull request references CONSOLE-3717 which is a valid jira issue.

In response to this:

It's unclear what the motivation was for their existing runlevels, with the API filenames landing without much context:

$ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
862dd2fe4 Add console-operator CRD and generate from types
$ git --no-pager log -1 --format='%h %B' --stat=80 862dd2fe4
862dd2fe4 Add console-operator CRD and generate from types
   
operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
1 file changed, 195 insertions(+)
$ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
a4be43c2b Added ability to configure Helm chart repository accessible within cluster
$ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c2b
a4be43c2b Added ability to configure Helm chart repository accessible within cluster
   
* Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
 from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
* The corresponding enchancement: openshift/enhancements/pull/175
   
Makefile                                           |   3 +-
hack/update-deepcopy.sh                            |   2 +-
hack/verify-crds.sh                                |   1 +
.../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
helm/v1alpha1/doc.go                               |   8 ++
helm/v1alpha1/register.go                          |  38 ++++++
helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
8 files changed, 448 insertions(+), 2 deletions(-)
$ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
8fe639fe2 helm: add a new namespaced CRD for helm
$ git --no-pager log -1 --format='%h %B' --stat=80 8fe639fe2
8fe639fe2 helm: add a new namespaced CRD for helm
   
As part of the process to support namespace-scoped Helm repositories for
non-admin users, this PR adds a new namespace-scoped CRD definition
named `projecthelmchartrepositories.helm.openshift.io`.
   
Closes: https://issues.redhat.com/browse/HELM-258
Signed-off-by: Allen Bai <abai@redhat.com>
   
.../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
helm/v1beta1/register.go                           |   2 +
...ypes_helm.go => types_helm_chart_repository.go} |   0
.../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in with the other console resources in the default runlevel 50 (docs, implementation).

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't matched the 0000_<runlevel>_<dash-separated-component>_<manifest_filename> template, and by sorting lexically between other level-70 manifests:

$ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
$ ls manifests | grep -1 0000_70_console-operator.crd.yaml
0000_70_cluster-network-operator_05_clusteroperator.yaml
0000_70_console-operator.crd.yaml
0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one large task-node. With this change removing the unparsable attempt at a runlevel, the network and DNS manifests will be able to apply on updates via two parallel task-nodes (screenshots)

I'm prefixing them with 00_ now to get them near the front of the other level-50 console manifests. For example, manifests/01-helm.yaml is declaring a HelmChartRepository resource, so we want it to sort after the HelmChartRepository CustomResourceDefinition within the console task-node.

Generated with:

$ rename 's/0000_[0-9][0-9]_/00_/' console/*/*.yaml
$ rename 's/0000_[0-9][0-9]_/00_/' helm/*/*.yaml
$ rename 's/0000_[0-9][0-9]_/00_/' operator/*/*console-operator*.yaml
$ git add -A console helm operator

Once this lands, I'll reroll openshift/console-operator#782 to vendor-bump openshift/api to pull in these changes (it currently just renames them in the Dockerfile COPYs).

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
Copy link
Contributor

openshift-ci bot commented Aug 2, 2023

Hello @wking! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 2, 2023
@wking wking force-pushed the console-crd-default-runlevel branch from cdb2521 to a290c49 Compare August 2, 2023 16:04
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 2, 2023

@wking: This pull request references CONSOLE-3717 which is a valid jira issue.

In response to this:

It's unclear what the motivation was for their existing runlevels, with the API filenames landing without much context:

$ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
862dd2fe4 Add console-operator CRD and generate from types
$ git --no-pager log -1 --format='%h %B' --stat=80 862dd2fe4
862dd2fe4 Add console-operator CRD and generate from types
   
operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
1 file changed, 195 insertions(+)
$ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
a4be43c2b Added ability to configure Helm chart repository accessible within cluster
$ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c2b
a4be43c2b Added ability to configure Helm chart repository accessible within cluster
   
* Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
 from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
* The corresponding enchancement: openshift/enhancements/pull/175
   
Makefile                                           |   3 +-
hack/update-deepcopy.sh                            |   2 +-
hack/verify-crds.sh                                |   1 +
.../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
helm/v1alpha1/doc.go                               |   8 ++
helm/v1alpha1/register.go                          |  38 ++++++
helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
8 files changed, 448 insertions(+), 2 deletions(-)
$ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
8fe639fe2 helm: add a new namespaced CRD for helm
$ git --no-pager log -1 --format='%h %B' --stat=80 8fe639fe2
8fe639fe2 helm: add a new namespaced CRD for helm
   
As part of the process to support namespace-scoped Helm repositories for
non-admin users, this PR adds a new namespace-scoped CRD definition
named `projecthelmchartrepositories.helm.openshift.io`.
   
Closes: https://issues.redhat.com/browse/HELM-258
Signed-off-by: Allen Bai <abai@redhat.com>
   
.../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
helm/v1beta1/register.go                           |   2 +
...ypes_helm.go => types_helm_chart_repository.go} |   0
.../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in with the other console resources in the default runlevel 50 (docs, implementation).

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't matched the 0000_<runlevel>_<dash-separated-component>_<manifest_filename> template, and by sorting lexically between other level-70 manifests:

$ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
$ ls manifests | grep -1 0000_70_console-operator.crd.yaml
0000_70_cluster-network-operator_05_clusteroperator.yaml
0000_70_console-operator.crd.yaml
0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one large task-node. With this change removing the unparsable attempt at a runlevel, the network and DNS manifests will be able to apply on updates via two parallel task-nodes (screenshots)

I'm prefixing them with 00_ now to get them near the front of the other level-50 console manifests. For example, manifests/01-helm.yaml is declaring a HelmChartRepository resource, so we want it to sort after the HelmChartRepository CustomResourceDefinition within the console task-node.

Generated with:

$ rename 's/0000_[0-9][0-9]_/00_/' console/*/*.yaml helm/*/*.yaml operator/*/*console-operator*.yaml
$ sed -i 's/0000_[0-9][0-9]_/00_/' console/*/*testsuite.yaml helm/*/*.testsuite.yaml
$ sed -i 's/0000_[0-9][0-9]_\(.*console-operator\)/00_\1/' operator/*/*testsuite.yaml
$ git add -A console helm operator

Once this lands, I'll reroll openshift/console-operator#782 to vendor-bump openshift/api to pull in these changes (it currently just renames them in the Dockerfile COPYs).

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 openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 2, 2023
It's unclear what the motivation was for their existing runlevels,
with the API filenames landing without much context:

  $ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
  862dd2f Add console-operator CRD and generate from types
  $ git --no-pager log -1 --format='%h %B' --stat=80 862dd2f
  862dd2f Add console-operator CRD and generate from types

   operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
   1 file changed, 195 insertions(+)
  $ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
  a4be43c Added ability to configure Helm chart repository accessible within cluster
  $ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c
  a4be43c Added ability to configure Helm chart repository accessible within cluster

  * Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
    from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
  * The corresponding enchancement: openshift/enhancements/pull/175

   Makefile                                           |   3 +-
   hack/update-deepcopy.sh                            |   2 +-
   hack/verify-crds.sh                                |   1 +
   .../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
   helm/v1alpha1/doc.go                               |   8 ++
   helm/v1alpha1/register.go                          |  38 ++++++
   helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
   helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
   8 files changed, 448 insertions(+), 2 deletions(-)
  $ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
  8fe639f helm: add a new namespaced CRD for helm
  $ git --no-pager log -1 --format='%h %B' --stat=80 8fe639f
  8fe639f helm: add a new namespaced CRD for helm

  As part of the process to support namespace-scoped Helm repositories for
  non-admin users, this PR adds a new namespace-scoped CRD definition
  named `projecthelmchartrepositories.helm.openshift.io`.

  Closes: https://issues.redhat.com/browse/HELM-258
  Signed-off-by: Allen Bai <abai@redhat.com>

   .../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
   helm/v1beta1/register.go                           |   2 +
   ...ypes_helm.go => types_helm_chart_repository.go} |   0
   .../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
   helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
   5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in
with the other console resources in the default runlevel 50 [1,2].

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't
matched the
0000_<runlevel>_<dash-separated-component>_<manifest_filename>
template [1], and by sorting lexically between other level-70
manifests:

  $ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
  $ ls manifests | grep -1 0000_70_console-operator.crd.yaml
  0000_70_cluster-network-operator_05_clusteroperator.yaml
  0000_70_console-operator.crd.yaml
  0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one
large task-node.  With this change removing the unparsable attempt at
a runlevel, the network and DNS manifests will be able to apply on
updates via two parallel task-nodes.

I'm prefixing most of them with '00_' now to get them near the front
of the other level-50 console manifests.  For example,
manifests/01-helm.yaml is declaring a HelmChartRepository resource
[3], so we want it to sort after the HelmChartRepository
CustomResourceDefinition within the console task-node.

But I'm prefixing ConsolePlugin with '90_' to sort it towards the
back, after 07-operator.yaml [4], because the operator is what serves
the /crdconvert webhook defined in that CRD [5].

Generated with:

  $ rename 's/0000_[0-9][0-9]_/00_/' console/*/*.yaml helm/*/*.yaml operator/*/*console-operator*.yaml
  $ sed -i 's/0000_[0-9][0-9]_/00_/' console/*/*testsuite.yaml helm/*/*.testsuite.yaml
  $ sed -i 's/0000_[0-9][0-9]_\(.*console-operator\)/00_\1/' operator/*/*testsuite.yaml
  $ rename 's/00_/90_/' console/*/*consoleplugin*.yaml
  $ sed -i 's/00_/90_/' console/*/*consoleplugin*testsuite.yaml
  $ git add -A console helm operator

[1]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[2]: https://github.com/openshift/oc/blob/13225e00caf1ad2d3603e1d1cc8651833f2effcb/pkg/cli/admin/release/new.go#L1412-L1416
[3]: https://github.com/openshift/console-operator/blob/cb08a319271b2f4676d1faae96a5ab10440facb0/manifests/01-helm.yaml
[4]: https://github.com/openshift/console-operator/blob/cb08a319271b2f4676d1faae96a5ab10440facb0/manifests/07-operator.yaml
[5]: https://issues.redhat.com//browse/OCPBUGS-15834
@wking wking force-pushed the console-crd-default-runlevel branch from a290c49 to 9e3e820 Compare August 2, 2023 16:11
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 2, 2023

@wking: This pull request references CONSOLE-3717 which is a valid jira issue.

In response to this:

It's unclear what the motivation was for their existing runlevels, with the API filenames landing without much context:

$ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
862dd2fe4 Add console-operator CRD and generate from types
$ git --no-pager log -1 --format='%h %B' --stat=80 862dd2fe4
862dd2fe4 Add console-operator CRD and generate from types
   
operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
1 file changed, 195 insertions(+)
$ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
a4be43c2b Added ability to configure Helm chart repository accessible within cluster
$ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c2b
a4be43c2b Added ability to configure Helm chart repository accessible within cluster
   
* Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
 from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
* The corresponding enchancement: openshift/enhancements/pull/175
   
Makefile                                           |   3 +-
hack/update-deepcopy.sh                            |   2 +-
hack/verify-crds.sh                                |   1 +
.../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
helm/v1alpha1/doc.go                               |   8 ++
helm/v1alpha1/register.go                          |  38 ++++++
helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
8 files changed, 448 insertions(+), 2 deletions(-)
$ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
8fe639fe2 helm: add a new namespaced CRD for helm
$ git --no-pager log -1 --format='%h %B' --stat=80 8fe639fe2
8fe639fe2 helm: add a new namespaced CRD for helm
   
As part of the process to support namespace-scoped Helm repositories for
non-admin users, this PR adds a new namespace-scoped CRD definition
named `projecthelmchartrepositories.helm.openshift.io`.
   
Closes: https://issues.redhat.com/browse/HELM-258
Signed-off-by: Allen Bai <abai@redhat.com>
   
.../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
helm/v1beta1/register.go                           |   2 +
...ypes_helm.go => types_helm_chart_repository.go} |   0
.../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in with the other console resources in the default runlevel 50 (docs, implementation).

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't matched the 0000_<runlevel>_<dash-separated-component>_<manifest_filename> template, and by sorting lexically between other level-70 manifests:

$ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
$ ls manifests | grep -1 0000_70_console-operator.crd.yaml
0000_70_cluster-network-operator_05_clusteroperator.yaml
0000_70_console-operator.crd.yaml
0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one large task-node. With this change removing the unparsable attempt at a runlevel, the network and DNS manifests will be able to apply on updates via two parallel task-nodes (screenshots)

I'm prefixing them with 00_ now to get them near the front of the other level-50 console manifests. For example, manifests/01-helm.yaml is declaring a HelmChartRepository resource, so we want it to sort after the HelmChartRepository CustomResourceDefinition within the console task-node.

But I'm prefixing ConsolePlugin with 90_ to sort it towards the back, after 07-operator.yaml, because the operator is [what serves the /crdconvert webhook defined in that CRD][5], #1532.

Generated with:

$ rename 's/0000_[0-9][0-9]_/00_/' console/*/*.yaml helm/*/*.yaml operator/*/*console-operator*.yaml
$ sed -i 's/0000_[0-9][0-9]_/00_/' console/*/*testsuite.yaml helm/*/*.testsuite.yaml
$ sed -i 's/0000_[0-9][0-9]_\(.*console-operator\)/00_\1/' operator/*/*testsuite.yaml
$ rename 's/00_/90_/' console/*/*consoleplugin*.yaml
$ sed -i 's/00_/90_/' console/*/*consoleplugin*testsuite.yaml
$ git add -A console helm operator

Once this lands, I'll reroll openshift/console-operator#782 to vendor-bump openshift/api to pull in these changes (it currently just renames them in the Dockerfile COPYs).

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
Copy link

openshift-ci-robot commented Aug 2, 2023

@wking: This pull request references CONSOLE-3717 which is a valid jira issue.

In response to this:

It's unclear what the motivation was for their existing runlevels, with the API filenames landing without much context:

$ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
862dd2fe4 Add console-operator CRD and generate from types
$ git --no-pager log -1 --format='%h %B' --stat=80 862dd2fe4
862dd2fe4 Add console-operator CRD and generate from types
   
operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
1 file changed, 195 insertions(+)
$ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
a4be43c2b Added ability to configure Helm chart repository accessible within cluster
$ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c2b
a4be43c2b Added ability to configure Helm chart repository accessible within cluster
   
* Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
 from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
* The corresponding enchancement: openshift/enhancements/pull/175
   
Makefile                                           |   3 +-
hack/update-deepcopy.sh                            |   2 +-
hack/verify-crds.sh                                |   1 +
.../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
helm/v1alpha1/doc.go                               |   8 ++
helm/v1alpha1/register.go                          |  38 ++++++
helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
8 files changed, 448 insertions(+), 2 deletions(-)
$ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
8fe639fe2 helm: add a new namespaced CRD for helm
$ git --no-pager log -1 --format='%h %B' --stat=80 8fe639fe2
8fe639fe2 helm: add a new namespaced CRD for helm
   
As part of the process to support namespace-scoped Helm repositories for
non-admin users, this PR adds a new namespace-scoped CRD definition
named `projecthelmchartrepositories.helm.openshift.io`.
   
Closes: https://issues.redhat.com/browse/HELM-258
Signed-off-by: Allen Bai <abai@redhat.com>
   
.../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
helm/v1beta1/register.go                           |   2 +
...ypes_helm.go => types_helm_chart_repository.go} |   0
.../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in with the other console resources in the default runlevel 50 (docs, implementation).

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't matched the 0000_<runlevel>_<dash-separated-component>_<manifest_filename> template, and by sorting lexically between other level-70 manifests:

$ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
$ ls manifests | grep -1 0000_70_console-operator.crd.yaml
0000_70_cluster-network-operator_05_clusteroperator.yaml
0000_70_console-operator.crd.yaml
0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one large task-node. With this change removing the unparsable attempt at a runlevel, the network and DNS manifests will be able to apply on updates via two parallel task-nodes (screenshots)

I'm prefixing them with 00_ now to get them near the front of the other level-50 console manifests. For example, manifests/01-helm.yaml is declaring a HelmChartRepository resource, so we want it to sort after the HelmChartRepository CustomResourceDefinition within the console task-node.

But I'm prefixing ConsolePlugin with 90_ to sort it towards the back, after 07-operator.yaml, because the operator is what serves the /crdconvert webhook defined in that CRD, #1532.

Generated with:

$ rename 's/0000_[0-9][0-9]_/00_/' console/*/*.yaml helm/*/*.yaml operator/*/*console-operator*.yaml
$ sed -i 's/0000_[0-9][0-9]_/00_/' console/*/*testsuite.yaml helm/*/*.testsuite.yaml
$ sed -i 's/0000_[0-9][0-9]_\(.*console-operator\)/00_\1/' operator/*/*testsuite.yaml
$ rename 's/00_/90_/' console/*/*consoleplugin*.yaml
$ sed -i 's/00_/90_/' console/*/*consoleplugin*testsuite.yaml
$ git add -A console helm operator

Once this lands, I'll reroll openshift/console-operator#782 to vendor-bump openshift/api to pull in these changes (it currently just renames them in the Dockerfile COPYs).

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.

@wking
Copy link
Member Author

wking commented Aug 2, 2023

/assign jhadvig

@jhadvig
Copy link
Member

jhadvig commented Aug 3, 2023

Thanks @wking 🙌
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2023
@bparees
Copy link
Contributor

bparees commented Aug 3, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, jhadvig, wking

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 3, 2023

@wking: 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-merge-robot openshift-merge-robot merged commit 08ec5d2 into openshift:master Aug 3, 2023
7 checks passed
@wking wking deleted the console-crd-default-runlevel branch August 3, 2023 18:11
wking added a commit to wking/openshift-api that referenced this pull request Aug 3, 2023
I'd meant to do these in 9e3e820 (Console CRDs: Rename files to use
the default run-level for updates, 2023-08-02, openshift#1541), but flubbed my
regexp.  Fixed now, with:

  $ rename 's/0000_[0-9][0-9][_-]/00_/' helm/*/*.yaml
  $ sed -i 's/0000_[0-9][0-9][_-]/00_/' helm/*/*.testsuite.yaml
wking added a commit to wking/console-operator that referenced this pull request Aug 3, 2023
Pull in openshift/api@9e3e820a70e4b8 (Console CRDs: Rename files to
use the default run-level for updates, 2023-08-02,
openshift/api#1541).

Generated with:

  $ rm -rf vendor/github.com/openshift/api  # possibly unnecessary?
  $ go get -u github.com/openshift/api
  $ go mod tidy
  $ go mod vendor
  $ git add -A go.* vendor

using:

  $ go version
  go version go1.19.5 linux/amd64
wking added a commit to wking/console-operator that referenced this pull request Aug 3, 2023
…m openshift/api

openshift/api@9e3e820a70e4b8 (Console CRDs: Rename files to use the
default run-level for updates, 2023-08-02, openshift/api#1541) renamed
these, and needing opinions in two locations that must match makes
maintenance tedious.  With this commit, I'm asserting that
openshift/api is where CRD filename opinions happen, and that this
repo just passes those filenames along.  And I'm asserting that the
CRDs we're responsible for are "all of console/ and helm/" and "any
CRDs in operator/ that include 'console-operator' in their filename".
wking added a commit to wking/openshift-api that referenced this pull request Aug 3, 2023
I'd meant to do these in 9e3e820 (Console CRDs: Rename files to use
the default run-level for updates, 2023-08-02, openshift#1541), but flubbed my
regexp.  Fixed now, with:

  $ rename 's/0000_[0-9][0-9][_-]/00_/' helm/*/*.yaml
  $ sed -i 's/0000_[0-9][0-9][_-]/00_/' helm/*/*.testsuite.yaml
wking added a commit to wking/openshift-api that referenced this pull request Aug 3, 2023
Like 9e3e820 (Console CRDs: Rename files to use the default
run-level for updates, 2023-08-02, openshift#1541) and 9aa8b74
(helm/v1beta1: Rename files to use the default run-level for updates,
2023-08-03, openshift#1544), but now for the CustomResourceDefinition deployed
by the samples operator [1].  As in the previous changes, the outgoing
filename failed to match the
0000_<runlevel>_<dash-separated-component>_<manifest_filename>
template [2], and there is no reason that the CRD needs bumping at
run-level 10 during updates.  I'm using a 00_ prefix to sort before
the other samples manifests [3], so there's no relative change in
update order between those resources; this commit just shifts update
CRD reconciliation vs. other, non-samples resources managed by the
cluster-version operator.

Generated with:

  $ mv samples/v1/0000_10_samplesconfig.crd.yaml samples/v1/00_samplesconfig.crd.yaml
  $ sed -i 's/0000_10_/00_/' samples/v1/stable.config.testsuite.yaml
  $ git add -A samples

[1]: https://github.com/openshift/cluster-samples-operator/blob/684b8c7f1cf6aaf9fb4f2bcffe91cde7eae07347/Dockerfile#L10
[2]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[3]: https://github.com/openshift/cluster-samples-operator/tree/684b8c7f1cf6aaf9fb4f2bcffe91cde7eae07347/manifests
wking added a commit to wking/openshift-api that referenced this pull request Aug 3, 2023
Like 9e3e820 (Console CRDs: Rename files to use the default
run-level for updates, 2023-08-02, openshift#1541) and 9aa8b74
(helm/v1beta1: Rename files to use the default run-level for updates,
2023-08-03, openshift#1544), but now for the CustomResourceDefinition deployed
by the samples operator [1].  As in the previous changes, the outgoing
filename failed to match the
0000_<runlevel>_<dash-separated-component>_<manifest_filename>
template [2], and there is no reason that the CRD needs bumping at
run-level 10 during updates.  I'm using a 00_ prefix to sort before
the other samples manifests [3], so there's no relative change in
update order between those resources; this commit just shifts update
CRD reconciliation vs. other, non-samples resources managed by the
cluster-version operator.

Checking history, there doesn't seem to be any motivation for the
original 0000_10_ prefix for this manifest:

  $ git --no-pager log -1 3139ff9
  commit 3139ff9 (origin/pr/513)
  Author: gabemontero <gmontero@redhat.com>
  Date:   Mon Nov 11 14:02:54 2019 -0500

      generate CRD yaml

Generated with:

  $ mv samples/v1/0000_10_samplesconfig.crd.yaml samples/v1/00_samplesconfig.crd.yaml
  $ sed -i 's/0000_10_/00_/' samples/v1/stable.config.testsuite.yaml
  $ git add -A samples

[1]: https://github.com/openshift/cluster-samples-operator/blob/684b8c7f1cf6aaf9fb4f2bcffe91cde7eae07347/Dockerfile#L10
[2]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[3]: https://github.com/openshift/cluster-samples-operator/tree/684b8c7f1cf6aaf9fb4f2bcffe91cde7eae07347/manifests
wking added a commit to wking/console-operator that referenced this pull request Sep 6, 2023
Removing the explicit 0000_10_ runlevel will allow these resources to
go in with the other console resources in the default runlevel 50
[1,2].  I'm prefixing ConsolePlugin with '90_' to sort it towards the
back, after 07-operator.yaml [3], because the operator is what serves
the /crdconvert webhook defined in that CRD [4].

This is a part of what shifted in openshift/api@9e3e820a70 (Console
CRDs: Rename files to use the default run-level for updates,
2023-08-02, openshift/api#1541), but pulled back to 4.12 to address
the 4.11-to-4.12 update race [4].  We don't need to patch 4.13 or
later this way, because for 4.12-to-4.13 and later updates, the
console deployment is already serving the conversion webhook.

[1]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[2]: https://github.com/openshift/oc/blob/13225e00caf1ad2d3603e1d1cc8651833f2effcb/pkg/cli/admin/release/new.go#L1412-L1416
[3]: https://github.com/openshift/console-operator/blob/cb08a319271b2f4676d1faae96a5ab10440facb0/manifests/07-operator.yaml
[4]: https://issues.redhat.com/browse/OCPBUGS-15834
wking added a commit to wking/console-operator that referenced this pull request Sep 6, 2023
Removing the explicit 0000_10_ runlevel will allow these resources to
go in with the other console resources in the default runlevel 50
[1,2].  I'm prefixing ConsolePlugin with '90_' to sort it towards the
back, after 07-operator.yaml [3], because the operator is what serves
the /crdconvert webhook defined in that CRD [4].

This is a part of what shifted in openshift/api@9e3e820a70 (Console
CRDs: Rename files to use the default run-level for updates,
2023-08-02, openshift/api#1541), but pulled back to 4.12 to address
the 4.11-to-4.12 update race [4].  We don't need to patch 4.13 or
later this way, because for 4.12-to-4.13 and later updates, the
console deployment is already serving the conversion webhook.  And
because this is a one-off just for 4.12.z, I'm using the RUN hack
instead of updating the API and vendor-bumping like we're doing in [5]
for the development branch.

[1]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[2]: https://github.com/openshift/oc/blob/13225e00caf1ad2d3603e1d1cc8651833f2effcb/pkg/cli/admin/release/new.go#L1412-L1416
[3]: https://github.com/openshift/console-operator/blob/cb08a319271b2f4676d1faae96a5ab10440facb0/manifests/07-operator.yaml
[4]: https://issues.redhat.com/browse/OCPBUGS-15834
[5]: openshift#782
wking added a commit to wking/console-operator that referenced this pull request Sep 6, 2023
Removing the explicit 0000_10_ runlevel will allow these resources to
go in with the other console resources in the default runlevel 50
[1,2].  I'm prefixing ConsolePlugin with '90_' to sort it towards the
back, after 07-operator.yaml [3], because the operator is what serves
the /crdconvert webhook defined in that CRD [4].

This is a part of what shifted in openshift/api@9e3e820a70 (Console
CRDs: Rename files to use the default run-level for updates,
2023-08-02, openshift/api#1541), but pulled back to 4.12 to address
the 4.11-to-4.12 update race [4].  We don't need to patch 4.13 or
later this way, because for 4.12-to-4.13 and later updates, the
console deployment is already serving the conversion webhook.  And
because this is a one-off just for 4.12.z, I'm using the expanded-COPY
hack instead of updating the API and vendor-bumping like we're doing
in [5] for the development branch.

[1]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[2]: https://github.com/openshift/oc/blob/13225e00caf1ad2d3603e1d1cc8651833f2effcb/pkg/cli/admin/release/new.go#L1412-L1416
[3]: https://github.com/openshift/console-operator/blob/cb08a319271b2f4676d1faae96a5ab10440facb0/manifests/07-operator.yaml
[4]: https://issues.redhat.com/browse/OCPBUGS-15834
[5]: openshift#782
wking added a commit to wking/console-operator that referenced this pull request Sep 25, 2023
Removing the explicit 0000_10_ runlevel will allow these resources to
go in with the other console resources in the default runlevel 50
[1,2].  I'm prefixing ConsolePlugin with '08-a' sort it after
07-operator.yaml [3], because the operator is what serves the
/crdconvert webhook defined in that CRD [4], but to sort it before
08-clusteroperator.yaml, to avoid [5]:

  level=error msg=Cluster operator console Available is False with RouteHealth_FailedLoadCA: RouteHealthAvailable: failed to read CA to check route health: configmaps "trusted-ca-bundle" not found

although I'm not entirely clear on why that trusted-ca-bundle issue
would be related to the positioning of the ConsolePlugin CRD.  Still,
even without a clear relationship, console operator health might
depend on having the CRD in place, so positioning between the
Deployment and the ClusterOperator seems safest.

This is a part of what shifted in openshift/api@9e3e820a70 (Console
CRDs: Rename files to use the default run-level for updates,
2023-08-02, openshift/api#1541), but pulled back to 4.12 to address
the 4.11-to-4.12 update race [4].  We don't need to patch 4.13 or
later this way, because for 4.12-to-4.13 and later updates, the
console deployment is already serving the conversion webhook.  And
because this is a one-off just for 4.12.z, I'm using the expanded-COPY
hack instead of updating the API and vendor-bumping like we're doing
in [6] for the development branch.

[1]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[2]: https://github.com/openshift/oc/blob/13225e00caf1ad2d3603e1d1cc8651833f2effcb/pkg/cli/admin/release/new.go#L1412-L1416
[3]: https://github.com/openshift/console-operator/blob/cb08a319271b2f4676d1faae96a5ab10440facb0/manifests/07-operator.yaml
[4]: https://issues.redhat.com/browse/OCPBUGS-15834
[5]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_console-operator/791/pull-ci-openshift-console-operator-release-4.12-e2e-gcp-ovn/1699372255974264832#1:build-log.txt%3A6648
[6]: openshift#782
wking added a commit to wking/console-operator that referenced this pull request Sep 27, 2023
Pull in openshift/api@9e3e820a70e4b8 (Console CRDs: Rename files to
use the default run-level for updates, 2023-08-02, openshift/api#1541)
and openshift/api@9aa8b74279cf32 (helm/v1beta1: Rename files to use
the default run-level for updates, 2023-08-03, openshift/api#1544).

Generated with:

  $ rm -rf vendor/github.com/openshift/api  # possibly unnecessary?
  $ go get -u github.com/openshift/api
  $ go mod tidy
  $ go mod vendor
  $ git add -A go.* vendor

using:

  $ go version
  go version go1.19.5 linux/amd64
wking added a commit to wking/console-operator that referenced this pull request Sep 27, 2023
…m openshift/api

openshift/api@9e3e820a70e4b8 (Console CRDs: Rename files to use the
default run-level for updates, 2023-08-02, openshift/api#1541) renamed
these, and needing opinions in two locations that must match makes
maintenance tedious.  With this commit, I'm asserting that
openshift/api is where CRD filename opinions happen, and that this
repo just passes those filenames along.  And I'm asserting that the
CRDs we're responsible for are "all of console/ and helm/" and "any
CRDs in operator/ that include 'console-operator' in their filename".
wking added a commit to wking/console-operator that referenced this pull request Oct 3, 2023
Pull in openshift/api@9e3e820a70e4b8 (Console CRDs: Rename files to
use the default run-level for updates, 2023-08-02, openshift/api#1541)
and openshift/api@9aa8b74279cf32 (helm/v1beta1: Rename files to use
the default run-level for updates, 2023-08-03, openshift/api#1544).

Generated with:

  $ rm -rf vendor/github.com/openshift/api  # possibly unnecessary?
  $ go get -u github.com/openshift/api
  $ go mod tidy
  $ go mod vendor
  $ git add -A go.* vendor

using:

  $ go version
  go version go1.19.5 linux/amd64
wking added a commit to wking/console-operator that referenced this pull request Oct 3, 2023
…m openshift/api

openshift/api@9e3e820a70e4b8 (Console CRDs: Rename files to use the
default run-level for updates, 2023-08-02, openshift/api#1541) renamed
these, and needing opinions in two locations that must match makes
maintenance tedious.  With this commit, I'm asserting that
openshift/api is where CRD filename opinions happen, and that this
repo just passes those filenames along.  And I'm asserting that the
CRDs we're responsible for are "all of console/ and helm/" and "any
CRDs in operator/ that include 'console-operator' in their filename".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants