BGP basic configurations #193

eduolivares · 2024-04-22T11:18:05Z

This patch includes kustomizations required to deploy BGP at different levels:

metallb
nad
control-plane
edpm

dt/bgp/kustomization.yaml

examples/dt/bgp/config/edpm/kustomization.yaml

abays · 2024-04-22T14:27:05Z

Could we get various .md READMEs included in this PR using the same pattern followed for other DTs and VAs? i.e. [1] [2] [3]

[1] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/README.md
[2] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/control-plane.md
[3] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/data-plane.md

eduolivares · 2024-04-22T15:04:11Z

Could we get various .md READMEs included in this PR using the same pattern followed for other DTs and VAs? i.e. [1] [2] [3]

[1] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/README.md [2] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/control-plane.md [3] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/data-plane.md

yes, I'll update the PR with them

eduolivares · 2024-04-24T16:18:32Z

Could we get various .md READMEs included in this PR using the same pattern followed for other DTs and VAs? i.e. [1] [2] [3]

[1] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/README.md [2] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/control-plane.md [3] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/data-plane.md

md files added.
@abays, please let me know how they look to you.

abays · 2024-04-29T10:33:51Z

Could we get various .md READMEs included in this PR using the same pattern followed for other DTs and VAs? i.e. [1] [2] [3]
[1] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/README.md [2] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/control-plane.md [3] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/dt/uni01alpha/data-plane.md

md files added. @abays, please let me know how they look to you.

The MD files look good

fultonj · 2024-04-29T18:46:29Z

@eduolivares This looks it's had some work behind it. Have you tested it or done any of the things from this doc?

#207

eduolivares · 2024-04-30T07:49:58Z

@eduolivares This looks it's had some work behind it. Have you tested it or done any of the things from this doc?

#207

Yes. I tested this PR last week, using downstream jobs and it worked fine. Controlplane and Dataplane are successfully deployed.
I have some problems to run tests from the test-operator, but I think it's reasonable to merge this PR and resolve those issue later.
The reason is that I am not sure yet whether those problems will be fixed with changes in architecture (maybe metallb, but most probably not), test-operator, ci-fmw or some playbook in the downstream job.

leifmadsen · 2024-04-30T13:22:40Z

@eduolivares This looks it's had some work behind it. Have you tested it or done any of the things from this doc?
#207

Yes. I tested this PR last week, using downstream jobs and it worked fine. Controlplane and Dataplane are successfully deployed. I have some problems to run tests from the test-operator, but I think it's reasonable to merge this PR and resolve those issue later. The reason is that I am not sure yet whether those problems will be fixed with changes in architecture (maybe metallb, but most probably not), test-operator, ci-fmw or some playbook in the downstream job.

Awesome thanks! I'm satisfied with this PR since it contains automation changes, has been noted as being manually tested, and has self-contained changes that wouldn't affect base components (and thus VAs).

fultonj · 2024-04-30T19:10:01Z

Would you please rebase this PR on the main branch?

Mainly you're missing this: #197

When I test your PR using the current VA1 commands [1] it fails to build.

[1] https://github.com/openstack-k8s-operators/architecture/blob/main/examples/va/hci/dataplane-pre-ceph.md#initialize-pre-ceph

$ kustomize build edpm-pre-ceph/nodeset
$ kustomize build edpm-pre-ceph/deployment
$ kustomize build deployment

leifmadsen · 2024-04-30T19:16:22Z

Would you please rebase this PR on the main branch?

Mainly you're missing this: #197

Huh I'm surprised we don't get a button for that here. Maybe we need to enable the feature in GitHub that requests PRs be up to date against main. When we enable that, there will be a button to allow us to automatically rebase these in the UI.

(I don't have admin permissions for this repo, so I can't enable it, or I would :))

fultonj · 2024-04-30T19:17:51Z

examples/dt/bgp/dataplane.md

+```
+Generate the dataplane CRs.
+```
+kustomize build > dataplane.yaml


After rebasing on the main branch, this command will need to be updated since it will fail as below. Please split the dataplane nodeset and deployment into separate CR files.

[fultonj@runcible edpm{bgp-basic-config}]$ kustomize build > dataplane.yaml Error: accumulating components: accumulateDirectory: "recursed accumulation of path '/home/fultonj/review/architecture/dt/bgp/edpm': accumulating components: accumulateDirectory: \"recursed accumulation of path '/home/fultonj/review/architecture/lib/dataplane': accumulating components: accumulateDirectory: \\\"recursed accumulation of path '/home/fultonj/review/architecture/lib/dataplane/nodeset': nothing selected by ConfigMap.[noVer].[noGrp]/edpm-nodeset-values.[noNs]:data.nodeset.ansible\\\"\"" [fultonj@runcible edpm{bgp-basic-config}]$

I have provided a PR against the base repo/branch to implement the refactoring. Please give it a check if you have a chance: eduolivares#1

fultonj · 2024-04-30T19:24:53Z

Would you please rebase this PR on the main branch?
Mainly you're missing this: #197

Huh I'm surprised we don't get a button for that here. Maybe we need to enable the feature in GitHub that requests PRs be up to date against main. When we enable that, there will be a button to allow us to automatically rebase these in the UI.

(I don't have admin permissions for this repo, so I can't enable it, or I would :))

I do not have the Settings button to enable it either as described here.

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-rebasing-for-pull-requests

Rebasing via the CLI will work though (I always do it that way myself in order to resolve conflicts).

leifmadsen · 2024-04-30T19:31:01Z

Huh I'm surprised we don't get a button for that here. Maybe we need to enable the feature in GitHub that requests PRs be up to date against main. When we enable that, there will be a button to allow us to automatically rebase these in the UI.
(I don't have admin permissions for this repo, so I can't enable it, or I would :))

I do not have the Settings button to enable it either as described here.

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-rebasing-for-pull-requests

Rebasing via the CLI will work though (I always do it that way myself in order to resolve conflicts).

Yea that works fine. It was nice with STF repos so we could just see when the PR was out of sync with a visual indicator :)

abays · 2024-05-01T09:37:18Z

Huh I'm surprised we don't get a button for that here. Maybe we need to enable the feature in GitHub that requests PRs be up to date against main. When we enable that, there will be a button to allow us to automatically rebase these in the UI.
(I don't have admin permissions for this repo, so I can't enable it, or I would :))

I do not have the Settings button to enable it either as described here.
https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-rebasing-for-pull-requests
Rebasing via the CLI will work though (I always do it that way myself in order to resolve conflicts).

Yea that works fine. It was nice with STF repos so we could just see when the PR was out of sync with a visual indicator :)

We actually already have Allow rebase merging checked. Do we want the option under it enabled?

leifmadsen · 2024-05-01T11:20:26Z

@abays yea that's the option I was referring to :)

abays · 2024-05-01T11:38:26Z

@abays yea that's the option I was referring to :)

Cool. I enabled it now.

leifmadsen · 2024-05-01T14:39:33Z

FWIW I'm working on #193 (comment) as I hit the rebase button and broke this PR, so I'll submit another PR targeting this one with the expected changes.

leifmadsen · 2024-05-01T14:55:20Z

I opened eduolivares#1 against the base repo/branch to fix the test-kustomize failure I caused when I did the PR rebase against main.

eduolivares · 2024-05-03T04:51:47Z

Running a d/s job to test @leifmadsen's dataplane split.

eduolivares · 2024-05-06T12:09:21Z

OK, several issues have been fixed since last week:

dataplane split
nncp default routes removed
octavia configuration was incomplete (we didn't see it before because the octavia operator missed some validations)

Current version works. It has just been tested d/s and the deployment works. The tempest tests fail until we find a solution for [1], which is my next task.

This PR is ready for reviews again and hopefully ready for merge.

[1] https://issues.redhat.com/browse/OSPRH-4826

abays · 2024-05-06T12:14:10Z

automation/vars/default.yaml

+        values:
+          - name: network-values
+            src_file: nncp/values.yaml
+        build_output: ../control-plane.yaml


This should maybe just be like this [1]?

[1]

architecture/automation/vars/uni01alpha.yaml

Line 39 in 3e0b37a

build_output: control-plane.yaml

examples/dt/bgp/edpm/deployment/kustomization.yaml

examples/dt/bgp/edpm/nodeset/kustomization.yaml

abays · 2024-05-06T12:31:42Z

dt/bgp/edpm/deployment/nova_custom.yaml

It might be better to put this in nodeset instead of deployment, because the other (normal) OpenStackDataPlaneServices are created when the OpenStackDataPlaneNodeSet is created [1][2].

[1] https://github.com/openstack-k8s-operators/dataplane-operator/blob/a1ed2be65e1f1ba6569afde0cea828d1ae8b3bb2/controllers/openstackdataplanenodeset_controller.go#L220-L230
[2] https://github.com/openstack-k8s-operators/dataplane-operator/blob/a1ed2be65e1f1ba6569afde0cea828d1ae8b3bb2/pkg/deployment/service.go#L75-L76

abays · 2024-05-06T12:31:57Z

dt/bgp/edpm/deployment/kustomization.yaml

+resources:
+  - nova_custom.yaml


See this comment: https://github.com/openstack-k8s-operators/architecture/pull/193/files#r1590963284

abays · 2024-05-06T12:43:01Z

dt/bgp/edpm/deployment/nova_custom.yaml

It might be better to put this in nodeset instead of deployment, because the other (normal) OpenStackDataPlaneServices are created when the OpenStackDataPlaneNodeSet is created [1][2].

[1] https://github.com/openstack-k8s-operators/dataplane-operator/blob/a1ed2be65e1f1ba6569afde0cea828d1ae8b3bb2/controllers/openstackdataplanenodeset_controller.go#L220-L230
[2] https://github.com/openstack-k8s-operators/dataplane-operator/blob/a1ed2be65e1f1ba6569afde0cea828d1ae8b3bb2/pkg/deployment/service.go#L75-L76

This patch includes kustomizations required to deploy BGP at different levels: - metallb - nad - control-plane - edpm

abays

/lgtm

eduolivares force-pushed the bgp-basic-config branch from f9b0e6d to ab5ce64 Compare April 22, 2024 11:46

abays reviewed Apr 22, 2024

View reviewed changes

dt/bgp/kustomization.yaml Outdated Show resolved Hide resolved

abays reviewed Apr 22, 2024

View reviewed changes

examples/dt/bgp/config/edpm/kustomization.yaml Outdated Show resolved Hide resolved

eduolivares force-pushed the bgp-basic-config branch from ab5ce64 to ae18746 Compare April 22, 2024 13:14

eduolivares force-pushed the bgp-basic-config branch 2 times, most recently from 5419af4 to 8cc383c Compare April 24, 2024 16:11

eduolivares force-pushed the bgp-basic-config branch 2 times, most recently from af73b0e to 790baf5 Compare April 29, 2024 08:50

leifmadsen added deployment-topology Deployment topology changes or implementations needs-testing Testing proof is required prior to merge triaged Issue has been initially reviewed and triaged labels Apr 29, 2024

fultonj mentioned this pull request Apr 29, 2024

Add first BGP DTs - WIP #44

Closed

eduolivares mentioned this pull request Apr 30, 2024

[va-hci] Remove references to edpm images #204

Merged

leifmadsen requested a review from fultonj April 30, 2024 13:23

leifmadsen added ready-merge Request looks ready to be merged into repository and removed needs-testing Testing proof is required prior to merge labels Apr 30, 2024

leifmadsen approved these changes Apr 30, 2024

View reviewed changes

fultonj removed the ready-merge Request looks ready to be merged into repository label Apr 30, 2024

fultonj reviewed Apr 30, 2024

View reviewed changes

leifmadsen force-pushed the bgp-basic-config branch from 790baf5 to 18a2869 Compare May 1, 2024 14:08

leifmadsen assigned eduolivares May 1, 2024

leifmadsen self-requested a review May 1, 2024 18:42

eduolivares force-pushed the bgp-basic-config branch 4 times, most recently from 9f0324e to 5b3dbeb Compare May 6, 2024 08:58

abays reviewed May 6, 2024

View reviewed changes

examples/dt/bgp/edpm/deployment/kustomization.yaml Outdated Show resolved Hide resolved

abays reviewed May 6, 2024

View reviewed changes

examples/dt/bgp/edpm/nodeset/kustomization.yaml Outdated Show resolved Hide resolved

abays reviewed May 6, 2024

View reviewed changes

BGP basic configurations

427db7c

This patch includes kustomizations required to deploy BGP at different levels: - metallb - nad - control-plane - edpm

eduolivares force-pushed the bgp-basic-config branch from 169e7d4 to 427db7c Compare May 6, 2024 13:32

abays approved these changes May 6, 2024

View reviewed changes

fultonj merged commit a0c052b into openstack-k8s-operators:main May 6, 2024
4 checks passed

eduolivares deleted the bgp-basic-config branch May 6, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BGP basic configurations #193

BGP basic configurations #193

eduolivares commented Apr 22, 2024

abays commented Apr 22, 2024

eduolivares commented Apr 22, 2024

eduolivares commented Apr 24, 2024

abays commented Apr 29, 2024

fultonj commented Apr 29, 2024

eduolivares commented Apr 30, 2024

leifmadsen commented Apr 30, 2024

fultonj commented Apr 30, 2024

leifmadsen commented Apr 30, 2024

fultonj Apr 30, 2024

leifmadsen May 1, 2024

fultonj commented Apr 30, 2024

leifmadsen commented Apr 30, 2024

abays commented May 1, 2024

leifmadsen commented May 1, 2024

abays commented May 1, 2024

leifmadsen commented May 1, 2024

leifmadsen commented May 1, 2024

eduolivares commented May 3, 2024

eduolivares commented May 6, 2024

abays May 6, 2024

abays May 6, 2024

abays May 6, 2024 •

edited

Loading

abays May 6, 2024

abays left a comment

		resources:
		- nova_custom.yaml

BGP basic configurations #193

BGP basic configurations #193

Conversation

eduolivares commented Apr 22, 2024

abays commented Apr 22, 2024

eduolivares commented Apr 22, 2024

eduolivares commented Apr 24, 2024

abays commented Apr 29, 2024

fultonj commented Apr 29, 2024

eduolivares commented Apr 30, 2024

leifmadsen commented Apr 30, 2024

fultonj commented Apr 30, 2024

leifmadsen commented Apr 30, 2024

fultonj Apr 30, 2024

Choose a reason for hiding this comment

leifmadsen May 1, 2024

Choose a reason for hiding this comment

fultonj commented Apr 30, 2024

leifmadsen commented Apr 30, 2024

abays commented May 1, 2024

leifmadsen commented May 1, 2024

abays commented May 1, 2024

leifmadsen commented May 1, 2024

leifmadsen commented May 1, 2024

eduolivares commented May 3, 2024

eduolivares commented May 6, 2024

abays May 6, 2024

Choose a reason for hiding this comment

abays May 6, 2024

Choose a reason for hiding this comment

abays May 6, 2024 • edited Loading

Choose a reason for hiding this comment

abays May 6, 2024

Choose a reason for hiding this comment

abays left a comment

Choose a reason for hiding this comment

abays May 6, 2024 •

edited

Loading