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.

Sign up for GitHub

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

Jump to bottom

Add Horizon to OpenStackControlPlane #226

Merged
merged 1 commit into from
Apr 18, 2023
Merged

Add Horizon to OpenStackControlPlane #226

merged 1 commit into from
Apr 18, 2023

Conversation

bshephar
Copy link
Contributor

This change adds the necessary pieces to deploy the Horizon operator.

@bshephar bshephar force-pushed the add-horizon branch 3 times, most recently from 43027ae to b56e269 Compare March 10, 2023 23:27
@bshephar bshephar marked this pull request as draft March 10, 2023 23:27
@bshephar bshephar force-pushed the add-horizon branch 2 times, most recently from f1cd09e to 84cb8b6 Compare March 10, 2023 23:32
@bshephar bshephar marked this pull request as ready for review March 10, 2023 23:34
@openshift-ci openshift-ci bot requested a review from olliewalsh March 10, 2023 23:34
@bshephar bshephar force-pushed the add-horizon branch 5 times, most recently from da9b2f5 to db67a67 Compare March 11, 2023 03:44
@bshephar
Copy link
Contributor Author

I believe the operator-build-deploy job failure will be resolved by this patch:
openstack-k8s-operators/horizon-operator#58

@bshephar
Copy link
Contributor Author

/retest

@bshephar bshephar force-pushed the add-horizon branch 3 times, most recently from d78bab8 to 0b2f2dc Compare March 13, 2023 02:36
@bshephar
Copy link
Contributor Author

/test openstack-operator-build-deploy

@gibizer
Copy link
Contributor

gibizer commented Mar 13, 2023

/test golangci
golangci-lint runs in pre-commit as well and it passed there. (yes we eventually remove the duplication of jobs)

gibizer
gibizer reviewed Mar 13, 2023
View reviewed changes
Copy link
Contributor

@gibizer gibizer left a comment

Choose a reason for hiding this comment

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

When I tried to deploy this I saw that the horizon controller_manager restarts with

W0313 08:40:08.044118       1 reflector.go:424] pkg/mod/k8s.io/client-go@v0.26.2/tools/cache/reflector.go:169: failed to list *v1beta1.Memcached: memcacheds.memcached.openstack.org is forbidden: User "system:serviceaccount:openstack:horizon-operator-controller-manager" cannot list resource "memcacheds" in API group "memcached.openstack.org" at the cluster scope

Do you see that if you try to deploy horizon alone or is it specific to this integration patch that causing the missing permission?

@gibizer
Copy link
Contributor

gibizer commented Mar 13, 2023

When I tried to deploy this I saw that the horizon controller_manager restarts with

W0313 08:40:08.044118       1 reflector.go:424] pkg/mod/k8s.io/client-go@v0.26.2/tools/cache/reflector.go:169: failed to list *v1beta1.Memcached: memcacheds.memcached.openstack.org is forbidden: User "system:serviceaccount:openstack:horizon-operator-controller-manager" cannot list resource "memcacheds" in API group "memcached.openstack.org" at the cluster scope

Do you see that if you try to deploy horizon alone or is it specific to this integration patch that causing the missing permission?

I think it is due to openstack-k8s-operators/horizon-operator#62

@bshephar
Copy link
Contributor Author

openstack-k8s-operators/horizon-operator#62

Yeah, looks like it:
openstack-k8s-operators/horizon-operator#63

@bshephar
Copy link
Contributor Author

bshephar commented Mar 14, 2023 •
edited
Loading

The only possible thing I can see for the golangci error is that it's related to a bug that was fixed in 1.46.2 and we're running these tests with 1.46.0:

INFO[2023-03-13T08:24:19Z] Tagging ci/golangci-lint:v1.46.0 into pipeline:golangci-lint.

A similar issue is reported here:
golangci/golangci-lint#2851

Which was apparently fixed by:
1uf3/execinquery@bef5bea

With the fix being listed in 1.46.2:
https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md#v1462

When we execute make golangci locally, it is pulling the latest version of golangci-lint:

+ http_download_curl /tmp/tmp.awuaaPCkPc/golangci-lint-1.51.2-linux-amd64.tar.gz https://github.com/golangci/golangci-lint/releases/download/v1.51.2/golangci-lint-1.51.2-linux-amd64.tar.gz
+ local_file=/tmp/tmp.awuaaPCkPc/golangci-lint-1.51.2-linux-amd64.tar.gz

So, my assumption is that the issue being reported in the golangci job is related to an issue with the golangci-lint version in-use. I would be interested to know if it's at all possible to bump this version?

@gibizer
Copy link
Contributor

gibizer commented Mar 14, 2023

golangci-lint runs in pre-commit as well and it passed there. (yes we eventually remove the duplication of jobs)

I was mistaken, here in openstack-operator we don't run golangci-lint in pre-commit :/

@gibizer
Copy link
Contributor

gibizer commented Mar 14, 2023

So, my assumption is that the issue being reported in the golangci job is related to an issue with the golangci-lint version in-use. I would be interested to know if it's at all possible to bump this version?

The job uses a golangci-lint specific container:

https://github.com/openshift/release/blob/ef2dca28318a6f0bd6257813165ffcdefb062e9d/ci-operator/config/openstack-k8s-operators/openstack-operator/openstack-k8s-operators-openstack-operator-master.yaml#L6-L9

but I'm not sure where that is coming from

@bshephar bshephar force-pushed the add-horizon branch 5 times, most recently from 8c6d25a to 6c75de3 Compare April 3, 2023 09:54
@bshephar bshephar force-pushed the add-horizon branch 3 times, most recently from 428e8fe to b4e67be Compare April 3, 2023 10:28
@fmount
Copy link
Contributor

fmount commented Apr 4, 2023

/test precommit-check

fmount
fmount requested changes Apr 4, 2023
View reviewed changes
custom-bundle.Dockerfile
@@ -48,6 +49,7 @@ FROM $DATAPLANE_BUNDLE as dataplane-bundle
FROM $NOVA_BUNDLE as nova-bundle
FROM $IRONIC_BUNDLE as ironic-bundle
FROM $OPENSTACK_STORAGE_BUNDLE as openstack-storage-bundle
FROM $HORIZON_BUNDLE as horizon-bundle
Copy link
Contributor

Choose a reason for hiding this comment

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

@dprince are we ok to merge the horizon bundle in the openstack-operator?

controllers/core/openstackcontrolplane_controller.go Outdated Show resolved Hide resolved
@bshephar bshephar force-pushed the add-horizon branch 3 times, most recently from f8dfff9 to 83367fe Compare April 4, 2023 22:24
@fmount
Copy link
Contributor

fmount commented Apr 5, 2023

From the build-deploy job:

  • The operator looks healthy [1]
  • It's an optional component (enabled is false by default): we don't see any pod here [2], which is expected

I had a minor comment on the samples/ area for network-isolation, otherwise looks good.

[1] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openstack-k8s-operators_openstack-operator/226/pull-ci-openstack-k8s-operators-openstack-operator-master-openstack-operator-build-deploy/1643379112682721280/artifacts/openstack-operator-build-deploy/openstack-k8s-operators-deploy/build-log.txt

[2] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openstack-k8s-operators_openstack-operator/226/pull-ci-openstack-k8s-operators-openstack-operator-master-openstack-operator-build-deploy/1643379112682721280/artifacts/openstack-operator-build-deploy/openstack-k8s-operators-gather/artifacts/pods/

@bshephar
Copy link
Contributor Author

bshephar commented Apr 5, 2023

From the build-deploy job:

  • The operator looks healthy [1]
  • It's an optional component (enabled is false by default): we don't see any pod here [2], which is expected

I had a minor comment on the samples/ area for network-isolation, otherwise looks good.

[1] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openstack-k8s-operators_openstack-operator/226/pull-ci-openstack-k8s-operators-openstack-operator-master-openstack-operator-build-deploy/1643379112682721280/artifacts/openstack-operator-build-deploy/openstack-k8s-operators-deploy/build-log.txt

[2] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openstack-k8s-operators_openstack-operator/226/pull-ci-openstack-k8s-operators-openstack-operator-master-openstack-operator-build-deploy/1643379112682721280/artifacts/openstack-operator-build-deploy/openstack-k8s-operators-gather/artifacts/pods/

Added to the network-isolation samples in the latest update.

@fmount
Copy link
Contributor

fmount commented Apr 5, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 5, 2023
@openshift-ci openshift-ci bot removed the lgtm label Apr 13, 2023
@bshephar
Add Horizon to OpenStackControlPlane
bb2c3bd 
This change adds the necessary pieces to deploy the Horizon operator.
@openshift-ci openshift-ci bot added the lgtm label Apr 18, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bshephar, dprince, fmount

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-merge-robot openshift-merge-robot merged commit 7858aa1 into openstack-k8s-operators:master Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuggi stuggi stuggi left review comments

@gibizer gibizer gibizer left review comments

@fmount fmount fmount approved these changes

@dprince dprince dprince approved these changes

@Sandeepyadav93 Sandeepyadav93 Awaiting requested review from Sandeepyadav93

@olliewalsh olliewalsh Awaiting requested review from olliewalsh

Assignees

@dprince dprince

@fmount fmount

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bshephar @gibizer @viroel @fmount @dprince @stuggi @openshift-merge-robot