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

Move some validation from kubebuilder annotation to webhook #418

Conversation

stuggi
Copy link
Contributor

@stuggi stuggi commented Apr 19, 2024

Right now applying a OpenStackControlPlane CR without any ironic section fails with:

$ oc apply -n openstack -f ...
The OpenStackControlPlane "openstack-galera-network-isolation-3replicas" is invalid:
* spec.ironic.template.rpcTransport: Unsupported value: "": supported values: "oslo", "json-rpc"
* spec.ironic.template.ironicConductors: Invalid value: "null": spec.ironic.template.ironicConductors in body must be of type array: "null" 

To allow the OpenstackControlPlane to be deployed without specifying any ironic there is the need to change the validation/defaulting of the RPCTransport and IronicConductors.

  • RPCTransport The enum check is moved to the validations webhook for create and update, default set in the defaulting webhook.

  • IronicConductors IronicConductors not longer a required parameter from kubebuilder pov, but in the validations webhook checked that length of the array is > 0

@stuggi stuggi requested review from abays and removed request for lewisdenny April 19, 2024 14:20
@stuggi
Copy link
Contributor Author

stuggi commented Apr 19, 2024

@hjensas please have a look and let me know what you think about doing this change. basically with this change we can deploy an osctlplane with not have to specify anything for the services which are disabled per default

@stuggi stuggi force-pushed the osctlplane_integration_fix branch from dc53a06 to bbdd13c Compare April 19, 2024 14:28
Copy link
Contributor

@abays abays left a comment

Choose a reason for hiding this comment

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

/lgtm

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://review.rdoproject.org/zuul/buildset/c5eb4ab8f666411594206d1bffcdb112

openstack-k8s-operators-content-provider FAILURE in 10m 44s
⚠️ podified-multinode-ironic-deployment SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@abays
Copy link
Contributor

abays commented Apr 19, 2024

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://review.rdoproject.org/zuul/buildset/93a15dda988a4a7ca14ce1bbafa1f045

✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 06m 58s
podified-multinode-ironic-deployment FAILURE in 45m 10s

@stuggi
Copy link
Contributor Author

stuggi commented Apr 22, 2024

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://review.rdoproject.org/zuul/buildset/4ab6e1a721434ea399ac8d109f51efd6

✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 08m 18s
podified-multinode-ironic-deployment FAILURE in 45m 46s

@hjensas
Copy link
Contributor

hjensas commented Apr 22, 2024

@hjensas please have a look and let me know what you think about doing this change. basically with this change we can deploy an osctlplane with not have to specify anything for the services which are disabled per default

I'm ok with this.

Copy link
Contributor

openshift-ci bot commented Apr 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abays, hjensas, stuggi

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 [abays,hjensas,stuggi]

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

@stuggi
Copy link
Contributor Author

stuggi commented Apr 22, 2024

the tempest vif test errors looks not to be related to this pr. we also see them in #420

@abays
Copy link
Contributor

abays commented Apr 22, 2024

@abays
Copy link
Contributor

abays commented Apr 22, 2024

Also, I did not mean to imply that the Tempest failures are related to this PR :)

@stuggi
Copy link
Contributor Author

stuggi commented Apr 22, 2024

yes, it seems to be happening in current PRs

@hjensas
Copy link
Contributor

hjensas commented Apr 22, 2024

We need to disable those test - openstack-k8s-operators/ci-framework#1541. We just disabled them downstream as well, there needs to be some fixing in upstream ironic for those VIFs tests to work.

stuggi added a commit to stuggi/openstack-operator that referenced this pull request Apr 24, 2024
@stuggi
Copy link
Contributor Author

stuggi commented Apr 24, 2024

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://review.rdoproject.org/zuul/buildset/f977d80635e24bdabbe149aa72a684c9

openstack-k8s-operators-content-provider FAILURE in 9m 44s
⚠️ podified-multinode-ironic-deployment SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

Right now applying a OpenStackControlPlane CR without any ironic
section fails with:

~~~
$ oc apply -n openstack -f ...
The OpenStackControlPlane "openstack-galera-network-isolation-3replicas" is invalid:
* spec.ironic.template.rpcTransport: Unsupported value: "": supported values: "oslo", "json-rpc"
* spec.ironic.template.ironicConductors: Invalid value: "null": spec.ironic.template.ironicConductors in body must be of type array: "null"
~~~

To allow the OpenstackControlPlane to be deployed without specifying
any ironic there is the need to change the validation/defaulting of
the RPCTransport and IronicConductors.

* RPCTransport
The enum check is moved to the validations webhook for create
and update, default set in the defaulting webhook.

* IronicConductors
IronicConductors not longer a required parameter from kubebuilder
pov, but in the validations webhook checked that length of the
array is > 0
@stuggi stuggi force-pushed the osctlplane_integration_fix branch from bbdd13c to 9ef78a3 Compare April 24, 2024 12:22
@openshift-ci openshift-ci bot removed the lgtm label Apr 24, 2024
Copy link
Contributor

openshift-ci bot commented Apr 24, 2024

New changes are detected. LGTM label has been removed.

@stuggi
Copy link
Contributor Author

stuggi commented Apr 24, 2024

rebased

@abays abays added the lgtm label Apr 24, 2024
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://review.rdoproject.org/zuul/buildset/5b760f68bc3745aeb60b3e950b0588f0

openstack-k8s-operators-content-provider FAILURE in 10m 01s
⚠️ podified-multinode-ironic-deployment SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@juliakreger
Copy link
Contributor

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://review.rdoproject.org/zuul/buildset/a972e7a57a964da89e03697862e3874f

✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 12m 17s
podified-multinode-ironic-deployment FAILURE in 42m 38s

@juliakreger
Copy link
Contributor

recheck apparent race condition in port deletion triggered the prior run to fail

@openshift-merge-bot openshift-merge-bot bot merged commit 290992b into openstack-k8s-operators:main Apr 24, 2024
7 checks passed
stuggi added a commit to stuggi/openstack-operator that referenced this pull request May 23, 2024
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.

4 participants