Add nova 3 cell DT #401
Add nova 3 cell DT #401
Conversation
|
Skipping CI for Draft Pull Request. |
jamepark4
force-pushed
the
add_multicell_dt
branch
9 times, most recently
from
364ebe9 to
38d7c4b
Compare
jamepark4
force-pushed
the
add_multicell_dt
branch
6 times, most recently
from
19005c6 to
0f33d8c
Compare
| config.kubernetes.io/local-config: "true" | ||
|
|
||
| data: | ||
| ssh_keys: |
There was a problem hiding this comment.
I am not a fan of hacing ssh_keys set in two values files in this PR. The nodeset in the library will create the same secret dataplane-ansible-ssh-private-key-secret for both nodesets. If a user looks at this they may be led to believe they can use different keys for the (I assume pre-provisioned nodes?) when using this DT - but that will not work since applying the second nodeset will modify the same secret instead of creating a new one.
There was a problem hiding this comment.
Ack I can removed these.
There was a problem hiding this comment.
I am not sure you can remove them, if you cannot - then this change should not be blocked by this comment from me.
I was trying to point out an issue with the library not supporting multiple nodesets in a good way. Essentilly you have two nodesets referencing lib/dataplane/nodeset/kustomization.yaml#L6-L8 - so you end up applying two separate files defining the same secret resource.
jamepark4
force-pushed
the
add_multicell_dt
branch
from
0f33d8c to
9cadc05
Compare
jamepark4
force-pushed
the
add_multicell_dt
branch
5 times, most recently
from
5bcd2c7 to
4e22901
Compare
jamepark4
force-pushed
the
add_multicell_dt
branch
from
4e22901 to
01ef803
Compare
|
recheck |
jamepark4
force-pushed
the
add_multicell_dt
branch
from
45d5c2c to
d379db8
Compare
|
@abays CI errors have been addressed, do you mind looking this over and providing any input/reviews if you have the bandwidth? |
jamepark4
added
ready-review
jamepark4
force-pushed
the
add_multicell_dt
branch
4 times, most recently
from
2e36514 to
2005568
Compare
| value: openstack-edpm-2 | ||
| - op: replace | ||
| path: /spec/services/13 | ||
| value: nova-cell-2 |
There was a problem hiding this comment.
This patching feels strange to me. We take default NodeSet and duplicate it for cell2 and patch it to connect the computes in it to cell2. But we
i) don't define the computes of NodeSet/openstack-edpm-2 here,
ii) we have a complete NodeSet parameter definition with the specific compute in nodeset2/values.yaml
So basically we have two ways to pass values to NodeSet/openstack-edpm2:
- here with the kustomize patch
- via nodeset2/values.yaml that is applied by another kustomize patch I assume.
This make it hard to follow what parameter comes from where. I don't want to block on this but I feel this needs to be refactored if possible to provide parameters to the NodeSet from a single source to help maintaining the kustomization. Is it something that can be done in a follow up?
@abays how do you feel about this issue? Do you agree that this is a problem? If so do you think it can be refactored to a single source? Is it something new in the proposed DT or is it widespread across the DTs?
There was a problem hiding this comment.
I feel this needs to be refactored if possible to provide parameters to the NodeSet from a single source to help maintaining the kustomization.
So what I think might happen when using this in CIFMW is the same problem that was encountered in this PR [1]. Since ultimately the lib dataplane nodeset is used [2] by both nodesets, they will collide with each other when CIFMW's kustomize_deploy role generates all of the YAMLs ahead time. But maybe you somehow found a way around the problem. I'll be curious to see the test results for this. Since this doesn't change any existing, underlying lib, va, or dt content, I'm good with merging it for now.
[1] #378
[2] https://github.com/jamepark4/architecture/blob/191d2bfabdc1355f5ddce7bf47a264fa46c12b45/dt/nova/nova-three-cells/edpm/nodeset/kustomization.yaml#L20
There was a problem hiding this comment.
Using CIFMW downstream I'm not seeing any collisions since the automation/vars uses two different values: edpm-nodeset-values and edpm-nodeset2-values. With nodeset2's name being updated by [1]. kustomize_deploy ends up generating two yaml definitions, nodeset.yaml and nodeset2.yaml. The different yamls are very similar but the Nodeset names allows them to be applied without any issues.
There was a problem hiding this comment.
That's great news. @mnietoji maybe you can review how two nodesets were used here and apply the same design to fix your PR (if you haven't already figured it out on your own)?
jamepark4
force-pushed
the
add_multicell_dt
branch
2 times, most recently
from
cfc1af1 to
556a603
Compare
jamepark4
force-pushed
the
add_multicell_dt
branch
from
556a603 to
ae78d13
Compare
Standard deployments currently deploy cell0 and cell1. Create a DT that deploys cell0, cell1, and cell2.
jamepark4
force-pushed
the
add_multicell_dt
branch
from
ae78d13 to
191d2bf
Compare
There was a problem hiding this comment.
Thanks for the cleanups. Looks good to me now.
Also I see that computes are properly mapped to different cells in https://sf.apps.int.gpc.ocp-hub.prod.psi.redhat.com/zuul/t/components-integration/build/6f99555d33dc4e4c9404e01703ab3aa4/log/logs/controller-0/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/ctlplane/nova/host_list
So this is good to go.
|
lgtm |
|
I have no objections to this PR if @abays wants to merge it. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abays, gibizer, jamepark4 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 |
softwarefactory-project-zuul
bot
merged commit b945fba
into
openstack-k8s-operators:main
Standard deployments currently deploy cell0 and cell1. Create a DT that deploys cell0, cell1, and cell2.
Depends-On: openstack-k8s-operators/ci-framework#2423