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

Support Octavia setting route annotations #1071

Conversation

fernandoroyosanchez
Copy link

@fernandoroyosanchez fernandoroyosanchez commented Sep 13, 2024

This patch allows Octavia to set its route annotations.

Depends-On: openstack-k8s-operators/octavia-operator#403

@openshift-ci openshift-ci bot requested review from fao89 and olliewalsh September 13, 2024 14:46
Copy link
Contributor

openshift-ci bot commented Sep 13, 2024

Hi @fernandoroyosanchez. Thanks for your PR.

I'm waiting for a openstack-k8s-operators member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/7fdca9b46620434084372ecd4d1d1156

openstack-k8s-operators-content-provider FAILURE in 9m 12s
⚠️ podified-multinode-edpm-deployment-crc SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ cifmw-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ openstack-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@fernandoroyosanchez
Copy link
Author

recheck

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/1c1d9306d9604e1f926a2bc55e0dff9c

openstack-k8s-operators-content-provider FAILURE in 7m 49s
⚠️ podified-multinode-edpm-deployment-crc SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ cifmw-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ openstack-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@bshephar
Copy link
Contributor

/ok-to-test

@bshephar
Copy link
Contributor

recheck

@bshephar
Copy link
Contributor

It was missing the bump on the openstack-k8s-operators versions to pick up the change in the octavia-operator:

    [1/2] STEP 14/15: RUN if [ -f $CACHITO_ENV_FILE ] ; then source $CACHITO_ENV_FILE ; fi ; env ${GO_BUILD_EXTRA_ENV_ARGS} go build ${GO_BUILD_EXTRA_ARGS} -a -o ${DEST_ROOT}/manager main.go
    # github.com/openstack-k8s-operators/openstack-operator/apis/core/v1beta1
    apis/core/v1beta1/openstackcontrolplane_webhook.go:894:92: r.Spec.Octavia.Template.GetDefaultRouteAnnotations undefined (type *"github.com/openstack-k8s-operators/octavia-operator/api/v1beta1".OctaviaSpecCore has no field or method GetDefaultRouteAnnotations)
    Error: building at STEP "RUN if [ -f $CACHITO_ENV_FILE ] ; then source $CACHITO_ENV_FILE ; fi ; env ${GO_BUILD_EXTRA_ENV_ARGS} go build ${GO_BUILD_EXTRA_ARGS} -a -o ${DEST_ROOT}/manager main.go": while running runtime: exit status 1
    make: *** [Makefile:209: docker-build] Error 1

I assume we have picked it up in some of the bumps that have happened since that merged. So rechecking now.

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/dc09be44f80a418facb38360b67aaecd

openstack-k8s-operators-content-provider FAILURE in 7m 58s
⚠️ podified-multinode-edpm-deployment-crc SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ cifmw-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ openstack-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

bshephar added a commit to bshephar/octavia-operator that referenced this pull request Oct 29, 2024
The GetDefaultRouteAnnotations function needs to be a method of
the OctaviaSpecCore struct rather than OctaviaSpec. This will resolve the issue
seen in: openstack-k8s-operators/openstack-operator#1071

Signed-off-by: Brendan Shephard <[email protected]>
Copy link
Contributor

@bshephar bshephar left a comment

Choose a reason for hiding this comment

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

I have tested this change with my proposed PR in the Octavia Operator:
openstack-k8s-operators/octavia-operator#405

If you don't remove the line from the functional tests like I have suggested here, functional tests will fail with the following:

  [PANICKED] Test Panicked
  In [It] at: /Users/brendanshephard/sdk/go1.21.12/src/runtime/panic.go:261 @ 10/29/24 18:14:55.846

  runtime error: invalid memory address or nil pointer dereference

  Full Stack Trace
    github.com/openstack-k8s-operators/openstack-operator/tests/functional/ctlplane_test.glob..func2.14.3()
        /Users/brendanshephard/Code/openstack-k8s-operators/openstack-operator/tests/functional/ctlplane/openstackoperator_controller_test.go:579 +0x6f0
  

As I mentioned, this is because Octavia isn't enabled in the functional tests.

@bshephar
Copy link
Contributor

To move this one forward, we first need to merge:
openstack-k8s-operators/octavia-operator#405

Then we need to bump the octavia-operator version in go.mod and apis/go.mod.
And also remove that line from the functional tests that I highlighted in my Request for Change.

@fernandoroyosanchez
Copy link
Author

Thx @bshephar for taking care of this one, I forgot to modify commit msg to put depend-ons from this one octavia-operator/403, where basically I do the SpecCore change and some additional changes. Anyway feel free to continue ahead with the octavia-operator/405 you proposed, I will rebase mine in that case.

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/8d3193e6af7045428be6142dc3d4745e

openstack-k8s-operators-content-provider FAILURE in 8m 17s
⚠️ podified-multinode-edpm-deployment-crc SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ cifmw-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ openstack-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/6f6a8d71280a493f947047d726282b55

openstack-k8s-operators-content-provider FAILURE in 10m 44s
⚠️ podified-multinode-edpm-deployment-crc SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ cifmw-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ openstack-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@@ -891,6 +891,7 @@ func (r *OpenStackControlPlane) DefaultServices() {
}

r.Spec.Octavia.Template.Default()
setOverrideSpec(&r.Spec.Octavia.APIOverride.Route, r.Spec.Octavia.Template.GetDefaultRouteAnnotations())
Copy link
Contributor

Choose a reason for hiding this comment

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

r.Spec.Octavia.Template is type *OctaviaSpecCore. But you have added this method on the *OctaviaAPISpecCore object:
https://github.com/openstack-k8s-operators/octavia-operator/pull/403/files#diff-d85dfd4f9255eccbf67bef3351764d06d3770a1e4366b602d8324480705acd14R201

So this currently fails:

❯ go vet ./...
# github.com/openstack-k8s-operators/openstack-operator/apis/core/v1beta1
apis/core/v1beta1/openstackcontrolplane_webhook.go:894:92: r.Spec.Octavia.Template.GetDefaultRouteAnnotations undefined (type *"github.com/openstack-k8s-operators/octavia-operator/api/v1beta1".OctaviaSpecCore has no field or method GetDefaultRouteAnnotations)

Did you mean to add the method to the OctaviaSpecCore struct instead of OctaviaAPISpecCore?

@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/openstack-k8s-operators/openstack-operator for 1071,d06a65b5b93e1380ea7d0872768858102e2cf42d

Copy link
Contributor

openshift-ci bot commented Nov 7, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fernandoroyosanchez

The full list of commands accepted by this bot can be found 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

Copy link
Contributor

openshift-ci bot commented Nov 7, 2024

@fernandoroyosanchez: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/precommit-check d06a65b link true /test precommit-check
ci/prow/images d06a65b link true /test images
ci/prow/openstack-operator-build-deploy-kuttl d06a65b link true /test openstack-operator-build-deploy-kuttl

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-sigs/prow repository. I understand the commands that are listed here.

@fernandoroyosanchez
Copy link
Author

Closed by error after rebasing discarding the changes...new one here #1175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants