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

Allow deploying Standalone with ironic as the compute driver #561

Merged

Conversation

hjensas
Copy link
Contributor

@hjensas hjensas commented Oct 2, 2023

devsetup - Standalone ironic - for adoption testing

Allow setting up standalone node with:

  • ironic as the compute driver
  • additional network interface (br-baremetal)

NOTE: There is quite a bit of re-factoring of the standalone scripts in this change.

  • A jinja2_render function has been added so that tripleo config files can be dynamically rendered with additional networks and enabled services for the role.
  • A tempdir is used for the REPO_SETUP_CMDS and CMDS_FILE unless overriden by the user.

TODO:

  • Need to either add the crc network to the edpm standalone node, or set up routing + dns or hosts entries so that edpm standalone can resolve CRC OCP services so that ironic can connect to the virtual redfish BMC's running in OCP.
  • Need to configure neutron bridgemappins and ServiceNetMap overrides for ironic.

To try this:

cd ..  # back to install_yamls
make nmstate
make namespace
cd devsetup  # back to install_yamls/devsetup
make bmaas BMAAS_INSTANCE_DISK_SIZE=2
make bmaas_generate_nodes_yaml | tail -n +2 | tee /tmp/ironic_nodes.yaml

cat << EOF > /tmp/addtional_nets.json
[
  {
    "type": "network",
    "name": "crc-bmaas",
    "standalone_config": {
      "type": "linux_bridge",
      "name": "baremetal",
      "mtu": 1500,
      "ip_subnet": "172.20.1.0/24",
      "allocation_pools": [
        {
          "start": "172.20.1.4",
          "end": "172.20.1.250"
        }
      ]
    }
  }
]
EOF
    
export EDPM_COMPUTE_ADDITIONAL_NETWORKS=$(jq -c . /tmp/addtional_nets.json)
export EDPM_COMPUTE_SUFFIX=standalone
export IP_ADRESS_SUFFIX=100
export STANDALONE_COMPUTE_DRIVER=ironic
make standalone

@hjensas hjensas requested review from steveb, cescgina and fao89 October 2, 2023 15:03
@openshift-ci openshift-ci bot requested review from gibizer and karelyatin October 2, 2023 15:03
@hjensas hjensas force-pushed the 03-standalone-ironic branch from 8b96ee0 to ff09e43 Compare October 2, 2023 15:11
@hjensas
Copy link
Contributor Author

hjensas commented Oct 2, 2023

@cescgina I think we want to run the adoption against this ...

@fao89 fao89 requested review from jistr and fultonj October 2, 2023 15:14
@hjensas hjensas force-pushed the 03-standalone-ironic branch 2 times, most recently from bf367fe to ada08f0 Compare October 2, 2023 15:23
@cescgina
Copy link
Contributor

cescgina commented Oct 2, 2023

@cescgina I think we want to run the adoption against this ...

ack, will try to review tomorrow, but testing it meanwhile in https://review.rdoproject.org/r/c/testproject/+/49765. Just a heads-up, there are two jobs running there, one that runs both standalone and crc in vms nested inside a single node, and a new one that has crc in a separate node. The networking is slightly different in this second job, it might cause some problems.

@gibizer
Copy link
Contributor

gibizer commented Oct 2, 2023

ironic as the compute driver

I'm wondering if the recently merged ironic virt driver support changes the direction here or not. With openstack-k8s-operators/nova-operator#477 you can ask nova-operator to start a nova-compute service in a pod and configure it with the ironic virt driver.

@hjensas
Copy link
Contributor Author

hjensas commented Oct 2, 2023

ironic as the compute driver

I'm wondering if the recently merged ironic virt driver support changes the direction here or not. With openstack-k8s-operators/nova-operator#477 you can ask nova-operator to start a nova-compute service in a pod and configure it with the ironic virt driver.

No, this is about deploying a legacy tripleo standalone with ironic. The goal is then to test the adoption of nova+ironic from the "legacy" cloud to the new podified controlplane running nova+ironic (ironic virt driver) in a pod.

@hjensas
Copy link
Contributor Author

hjensas commented Oct 2, 2023

@cescgina I think we want to run the adoption against this ...

ack, will try to review tomorrow, but testing it meanwhile in https://review.rdoproject.org/r/c/testproject/+/49765. Just a heads-up, there are two jobs running there, one that runs both standalone and crc in vms nested inside a single node, and a new one that has crc in a separate node. The networking is slightly different in this second job, it might cause some problems.

Thanks, I can see in logs that the patch applied and the testproject jobs ran green:

data-plane-adoption-github-rdo-centos-9-extracted-crc https://review.rdoproject.org/zuul/build/8a80f13fce524b2fa057e4055d239133 : SUCCESS in 1h 30m 42s
data-plane-adoption-github-rdo-centos-9-crc-single-node https://review.rdoproject.org/zuul/build/428c0db0415c408790598d7bb4e96843 : SUCCESS in 1h 26m 02s

https://logserver.rdoproject.org/65/49765/18/check/data-plane-adoption-github-rdo-centos-9-crc-single-node/428c0db/ci-framework-data/logs/ci_make_002_run_standalone.log

print(j2_template.render(**vars))
'
+ jinja2_render standalone/role.j2 /tmp/tmp.prbUhQ9N6V/tmp.DP9GGx66YR.yaml
+ local j2_template_file
+ local j2_vars
+ j2_template_file=standalone/role.j2
+ j2_vars_file=/tmp/tmp.prbUhQ9N6V/tmp.DP9GGx66YR.yaml
+ /usr/bin/python3 -c '
import yaml
import jinja2

with open('\''/tmp/tmp.prbUhQ9N6V/tmp.DP9GGx66YR.yaml'\'', '\''r'\'') as f:
    vars = yaml.safe_load(f.read())

with open('\''standalone/role.j2'\'', '\''r'\'') as f:
    template = f.read()

j2_template = jinja2.Template(template)

print(j2_template.render(**vars))
'

@hjensas hjensas force-pushed the 03-standalone-ironic branch from ada08f0 to 92c2538 Compare October 3, 2023 14:38
@jistr
Copy link
Contributor

jistr commented Oct 4, 2023

Thanks for working on this Harald, it looks good (to the extent i'm able to review :) ). We should just make sure to have a CI run on the latest changes before merging. If there is more stuff to add we could also chunk it up into multiple PRs perhaps.

@hjensas
Copy link
Contributor Author

hjensas commented Oct 4, 2023

Thanks for working on this Harald, it looks good (to the extent i'm able to review :) ). We should just make sure to have a CI run on the latest changes before merging. If there is more stuff to add we could also chunk it up into multiple PRs perhaps.

Agreed on chunking up into multipl PRs, it is unfortunate that we cannot stack changes in GitHub-- like we can do on gerrit. I have some TODO's but I will do that in separate PRs.

The push yesterday was just a re-base on #560 - there was a conflict after merging that.

@cescgina can you re-trigger a test-project with adoption jobs on the latest version of this to ensure I did not accidentally break anything?

@cescgina
Copy link
Contributor

cescgina commented Oct 4, 2023

Thanks for working on this Harald, it looks good (to the extent i'm able to review :) ). We should just make sure to have a CI run on the latest changes before merging. If there is more stuff to add we could also chunk it up into multiple PRs perhaps.

Agreed on chunking up into multipl PRs, it is unfortunate that we cannot stack changes in GitHub-- like we can do on gerrit. I have some TODO's but I will do that in separate PRs.

The push yesterday was just a re-base on #560 - there was a conflict after merging that.

@cescgina can you re-trigger a test-project with adoption jobs on the latest version of this to ensure I did not accidentally break anything?

Sure thing, running here: https://review.rdoproject.org/r/c/testproject/+/49765

@hjensas
Copy link
Contributor Author

hjensas commented Oct 4, 2023

Sure thing, running here: https://review.rdoproject.org/r/c/testproject/+/49765

Thanks, the job ran green. 👍

https://review.rdoproject.org/r/c/testproject/+/49765

noop https://review.rdoproject.org/zuul/build/ae0eaafc8a134a449816d9812fe0b980 : SUCCESS in 0s
data-plane-adoption-github-rdo-centos-9-extracted-crc https://review.rdoproject.org/zuul/build/73eb5d2eb920489a92a750cc6e2f58e1 : SUCCESS in 1h 34m 42s
data-plane-adoption-github-rdo-centos-9-crc-single-node https://review.rdoproject.org/zuul/build/fccf77a0c3744a56b4c5e45bba9ae7f4 : SUCCESS in 1h 23m 19s

Use a tempdir for default CMDS_FILE and REPO_SETUP_CMDS files.
As already done for repo commands, only create the commands file if it
does not exist.

Signed-off-by: Harald Jensås <[email protected]>
Use jinj2 for files:
  network_data, deployed_network, net_config

Using the jinj2_render function we can dynamically create
these files based on user arguments such as extra networks
defined in EDPM_COMPUTE_ADDITIONAL_NETWORKS.

Signed-off-by: Harald Jensås <[email protected]>
@@ -80,7 +89,7 @@ export HOST_PRIMARY_RESOLV_CONF_ENTRY=${HOST_PRIMARY_RESOLV_CONF_ENTRY}
export INTERFACE_MTU=${INTERFACE_MTU:-1500}
export NTP_SERVER=${NTP_SERVER:-"clock.corp.redhat.com"}
export EDPM_COMPUTE_CEPH_ENABLED=${EDPM_COMPUTE_CEPH_ENABLED:-true}
export CEPH_ARGS="${CEPH_ARGS:--e \$HOME/deployed_ceph.yaml -e /usr/share/openstack-tripleo-heat-templates/environments/cephadm/cephadm-rbd-only.yaml}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we'll ever need to do it, but I think that with this change we loose the ability to modify the ceph deployment. If I understand correctly, with this change we can only choose to not deploy ceph, or deploy it with -e /usr/share/openstack-tripleo-heat-templates/environments/cephadm/cephadm-rbd-only.yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh, you are right. I misread that and thought it was always redefined in openstack.sh.
I can do a follow up to fix that? (Or if people insist I can change this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So ... I updated it to make it possible to modify the ceph deployment again. Good catch @cescgina++

Allow setting up standalone node with:
* ironic as the compute driver
* additional network interface (br-baremetal)

To try this:

```bash
cd ..  # back to install_yamls
make nmstate
make namespace
cd devsetup  # back to install_yamls/devsetup
make bmaas BMAAS_INSTANCE_DISK_SIZE=2
make bmaas_generate_nodes_yaml | tail -n +2 | tee /tmp/ironic_nodes.yaml

cat << EOF > /tmp/addtional_nets.json
[
  {
    "type": "network",
    "name": "crc-bmaas",
    "standalone_config": {
      "type": "linux_bridge",
      "name": "baremetal",
      "mtu": 1500,
      "ip_subnet": "172.20.1.0/24",
      "allocation_pools": [
        {
          "start": "172.20.1.4",
          "end": "172.20.1.250"
        }
      ]
    }
  }
]
EOF

export EDPM_COMPUTE_ADDITIONAL_NETWORKS=$(jq -c . /tmp/addtional_nets.json)
export EDPM_COMPUTE_SUFFIX=standalone
export IP_ADRESS_SUFFIX=100
export COMPUTE_DRIVER=ironic
make standalone
```

Signed-off-by: Harald Jensås <[email protected]>
@hjensas hjensas force-pushed the 03-standalone-ironic branch from 92c2538 to 8b43c1e Compare October 4, 2023 17:13
@jistr
Copy link
Contributor

jistr commented Oct 4, 2023

Cool. Should we go for a merge? My only outstanding concern is if this could potentially make it harder to merge openstack-k8s-operators/data-plane-adoption#171 but since the MariaDB/OVN data copy works with this PR, i think EDPM should too... cc @fao89

@jistr
Copy link
Contributor

jistr commented Oct 4, 2023

So i'm leaning towards merging this.

@fao89
Copy link
Contributor

fao89 commented Oct 4, 2023

Cool. Should we go for a merge? My only outstanding concern is if this could potentially make it harder to merge openstack-k8s-operators/data-plane-adoption#171 but since the MariaDB/OVN data copy works with this PR, i think EDPM should too... cc @fao89

+1 to merge

@fao89
Copy link
Contributor

fao89 commented Oct 4, 2023

/approve

@openshift-ci openshift-ci bot added the approved label Oct 4, 2023
@fao89
Copy link
Contributor

fao89 commented Oct 4, 2023

it is approved, the first lgtm will merge it

@openshift-ci openshift-ci bot added the lgtm label Oct 5, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fao89, hjensas, jistr

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot merged commit b8b9776 into openstack-k8s-operators:main Oct 5, 2023
@hjensas hjensas deleted the 03-standalone-ironic branch October 5, 2023 09:03
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.

5 participants