-
Notifications
You must be signed in to change notification settings - Fork 86
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
Uni delta #191
Uni delta #191
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this sort of linking violates the default "load restriction" policy of Kustomize, because you are importing resources
from outside of the dir. Is there some reason you can't just use lib/dataplane
directly as a component
in your dt/uni04delta/ceph/kustomization.yaml
? You can still use all of the patches
and the replacements
that you have there.
automation/vars/uni04delta.yaml
Outdated
oc -n openstack wait osdpd | ||
edpm-deployment | ||
--for condition=Ready | ||
--timeout=1200s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the timeout here was not sufficient it took me around 22 minutes, so better to move it to 1800 or 2400 for slow systems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
examples/dt/uni04delta/control-plane/service-values-wo-ceph-client.yaml
Outdated
Show resolved
Hide resolved
enable_debug: false | ||
gather_facts: false | ||
image_tag: current-podified | ||
neutron_physical_bridge_name: br-ex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neutron vars can be dropped from ceph nodeset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to put the neutron_physical_bridge_name: br-ex
and
neutron_public_interface_name: eth0
back. The deployment is failing without them.
@jirimacku when you're ready for this to be reviewed further, please take it out of draft and rebase. |
- ../../lib/networking/metallb | ||
- ../../lib/networking/netconfig | ||
- ../../lib/networking/nad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: this can be simplified to - ../../lib/networking
, but it works as-is as well
The deployment itself is passing - thanks to everybody!
On one of the compute nodes I realized there might be a problem with the certificates rabbitq is using:
Any hint is appreciated. |
#191 (comment) should help |
Looks like this is getting close, but there are a few open comments that still need addressing.
|
- target: | ||
kind: OpenStackDataPlaneDeployment | ||
name: .* | ||
patch: |- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this patch duplicate of above replacements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jirimacku can you check and clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to be resolved now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaned and checked.
edpm_neutron_metadata_agent_image: '{{ registry_url }}/openstack-neutron-metadata-agent-ovn:{{ image_tag }}' | ||
edpm_nodes_validation_validate_controllers_icmp: false | ||
edpm_nodes_validation_validate_gateway_icmp: false | ||
edpm_nova_compute_container_image: '{{ registry_url }}/openstack-nova-compute:{{ image_tag }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the images can be dropped. and also the references of registry_url and image_tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
- ceph_nfs | ||
- ceph_rgw_frontend | ||
- ceph_nfs_frontend | ||
storage_mtu: 9000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these storage_* i think not needed here(as should be coming from dataplane-operator based on netconfig) but likely copied from vas, so can be checked and cleanedup later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VA1 does the same because it overrides the default MTU of 1500 for the storage network and adds the storage management network. Both uniDelta and va1 deploy EDPM nodes which host Ceph.
The storage management network does not need to be created in IPAM on the controller node. It's only used by nodes hosting ceph for ceph to sync it's internal data. I don't think we need to block this over storage_mgmt_ network being here. If you have an more dataplane-operator-netconfig-like way to do this, then let me know and we can do a follow up patch on va1 and uniDelta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes was not blocking, just raised this so be checked later
kind: Kustomization | ||
|
||
components: | ||
# - ../../../va/hci/edpm-post-ceph/nodeset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment part can be dropped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
name: edpm-values | ||
annotations: | ||
config.kubernetes.io/local-config: "true" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not using _src and _dst as same path? as with that it would be usable without relying on ci-framework generation logic.
timesync_ntp_servers: | ||
- hostname: clock.redhat.com | ||
|
||
edpm_iscsid_image: '{{ registry_url }}/openstack-iscsid:{{ image_tag }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same images and all references of registry_url and image_tag can be dropped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to loose the original file for the reference. I'll suggest to use the backup: true
here:
https://github.com/openstack-k8s-operators/ci-framework/blob/main/roles/cifmw_ceph_client/tasks/edpm_values_post_ceph.yml#L23 and here:
https://github.com/openstack-k8s-operators/ci-framework/blob/main/roles/cifmw_ceph_client/tasks/edpm_service_values_post_ceph.yml#L23
and use the same file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the -wo- files. Will also make the kustomization dry run passing in the gate.
nova_api_network: internalapi | ||
nova_libvirt_network: internalapi | ||
|
||
edpm_ceph_hci_pre_enabled_services: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be only needed in ceph nodesets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
automation/vars/uni04delta.yaml
Outdated
- name: network-values | ||
build_output: control-plane.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@jirimacku I think it would be good to rebase this against |
automation/vars/uni04delta.yaml
Outdated
--timeout=600s | ||
values: | ||
- name: network-values | ||
src: nncp/values.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src: nncp/values.yaml | |
src_file: nncp/values.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Tested with test project. Deployment passed, tempest base test case passed. |
|
||
# 4. Generate the Ceph nodeset deployment plan. | ||
|
||
$ kustomize build --load-restrictor LoadRestrictionsNone > nodeset-pre-ceph.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove --load-restrictor LoadRestrictionsNone
. It's not necessary in order to build the nodeset-pre-ceph.yaml
file.
[fultonj@runcible nodeset{uniDelta}]$ kustomize build > nodeset-pre-ceph-1.yaml
[fultonj@runcible nodeset{uniDelta}]$ kustomize build --load-restrictor LoadRestrictionsNone > nodeset-pre-ceph-2.yaml
[fultonj@runcible nodeset{uniDelta}]$ diff nodeset-pre-ceph-2.yaml nodeset-pre-ceph-1.yaml
[fultonj@runcible nodeset{uniDelta}]$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
# 8. Generate the Ceph data plane deployment plan. | ||
|
||
$ kustomize build --load-restrictor LoadRestrictionsNone > deployment-pre-ceph.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove --load-restrictor LoadRestrictionsNone
. I was able to build deployment-pre-ceph.yaml
without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thank you, yes the rebased version no longer removes barbican from VA1 so that's good. Would you please squash this PR?
|
Overall this PR looks good to me and provided it can be squashed and |
metallb.universe.tf/loadBalancerIPs: 172.17.0.86 | ||
|
||
lbServiceType: LoadBalancer | ||
storageClass: host-nfs-storageclass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not using host-nfs-storageclass
but we can set it to local-storage
. However, it would change with LVM operators are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think should be changed to local-storage
so it works like all of the other VAs/DTs.
The LVMS ci-framework PR will set the generated values.yaml to have the correct name depending on if cifmw_lvms
is true or false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should keep it as host-nfs-storageclass
since it might make people think it needs some special storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
03a251e
to
5e4a6ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just few minor fixes inline
- target: | ||
kind: OpenStackDataPlaneDeployment | ||
name: .* | ||
patch: |- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jirimacku can you check and clean.
[ovs] | ||
igmp_snooping_enable = true | ||
[ovn] | ||
ovn_emit_need_to_frag = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can disable this for now as per #228
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
- ceph_nfs | ||
- ceph_rgw_frontend | ||
- ceph_nfs_frontend | ||
storage_mtu: 9000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes was not blocking, just raised this so be checked later
automation/vars/uni04delta.yaml
Outdated
oc -n openstack wait osdpns | ||
openstack-edpm | ||
--for condition=SetupReady | ||
--timeout=2400s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can lower the timeout here to for nodeset, as that doesn't need much time, likely leftover from dataplane split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
examples/dt/uni04delta/edpm.md
Outdated
Wait for nodeset deployment to finish | ||
|
||
```bash | ||
oc wait osdpns edpm-nodeset-values --for condition=SetupReady --timeout=1200s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/edpm-nodeset-values/openstack-edpm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
/lgtm |
/lgtm |
Increase timeout for nncp
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the most recent set of changes.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fultonj, jirimacku 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 |
Build succeeded (gate pipeline). ✔️ noop SUCCESS in 0s |
68b1bbb
into
openstack-k8s-operators:main
This PR contains the necessary custom resources for deploying OpenStack Services on OpenShift. The CRs are for the proposed uni*delta deployed topology.