Baremetal: Bug 1801238: Pull data from ironic inspector and annotate BareMetalHost #3591

imain · 2020-05-13T02:05:16Z

Right now we don't get any introspection data about the masters
because they are provisioned by the bootstrap node and the information
is lost. This patch adds a script to pull the introspection data
from ironic running on the bootstrap and places it in the BareMetalHost CRs
for the masters. This is done via an annotation as we cannot directly
update the Status in BMH. The annotation is picked up by the BMO
and added to the BMH CRs.

There are several bugs filed about this:

openshift-metal3/dev-scripts#917
https://bugzilla.redhat.com/show_bug.cgi?id=1801238

data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template

data/data/baremetal/masters/main.tf

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh

imain · 2020-05-13T19:45:14Z

/assign @hardys

stbenjam

This looks great! A couple small comments

Shellcheck has a few nits:

./data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh:3:1: note: Not following: /usr/local/bin/release-image.sh was not specified as input (see shellcheck -x). [SC1091]
./data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh:27:15: note: Double quote to prevent globbing and word splitting. [SC2086]
./data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh:28:49: note: Double quote to prevent globbing and word splitting. [SC2086]
./data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh:41:15: note: Double quote to prevent globbing and word splitting. [SC2086]
./data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh:49:34: note: Double quote to prevent globbing and word splitting. [SC2086]

data/data/bootstrap/baremetal/systemd/units/master-bmh-update.service

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh

imain · 2020-05-13T23:11:54Z

I don't understand that shellcheck failure. Not sure how to fix it..

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh

stbenjam · 2020-05-14T14:16:20Z

One thing I noticed is the master-bmh-update doesn't seem to start on the bootkube, didn't dig into why but it just shows as 'inactive'.

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh

stbenjam · 2020-05-14T18:51:32Z

/label platform/baremetal

imain · 2020-05-14T19:40:13Z

/assign @abhinavdahiya

abhinavdahiya · 2020-05-14T19:50:14Z

pkg/asset/ignition/bootstrap/bootstrap.go

+		"ironic.service":            {},
+		"master-bmh-update.service": {},


Can there be some wants/depends_on relationship between these 2 services that allows us to not activate each individual service? or are these actually 2 independent services?

There are 2 independent .service files for them if that's what you mean. I did try it with a dependency but the service was just marked as disabled and wouldn't start.

master-bmh-update talks to ironic, but they are independent with different goals. I think it's more obvious to leave both explicitly enabled

stbenjam · 2020-05-14T20:02:31Z

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh

+
+# Wait for 3 masters to appear.
+while [ "$(curl -s http://localhost:6385/v1/nodes | jq '.nodes[] | .uuid' | wc -l)" -lt 3 ]; do
+    echo waiting for at least 3 masters.


Sorry, one more thought here: 3 might be a bad assumption. Single master installs have historically been possible and used by developers with minimal hardware. It's unsupported though.

We could maybe count how many BareMetalHost CR's there are with master in the name, or read the actual value from /opt/openshift/manifests/cluster-config.yaml.

@abhinavdahiya might have thoughts here.

or just check even for 1? If there's 1 it will not be 'active'.

I'll test it and see if that works

Yea that's probably good enough. I worried about a race but the next step takes like 10 minutes at least, so the installer will have created all the nodes by then

Yes ok. I tested it with -lt 1, meaning we just need 1 node to come up and it continues. I also didn't use any sleep in the loop so it queried constantly to check for possible race conditions. As soon as a node comes up its state is 'enroll', and then moves to introspection states, so I think we are safe race-wise looking for 1 node and then on to the next check; which is based on all node states being "active".

stbenjam · 2020-05-14T20:39:37Z

Got a good local test. You can test with the baremetal-operator from git if you check it out to $HOME, replace "base" with "ubi8" in the dockerfile and set this:

KNI_INSTALL_FROM_GIT=true
BAREMETAL_OPERATOR_LOCAL_IMAGE=file:///home/notstack/baremetal-operator
OPENSHIFT_RELEASE_IMAGE=registry.svc.ci.openshift.org/ocp/release:4.5.0-0.nightly-2020-05-14-021132

MY MASTER MACHINES HAVE NODES!!

$ oc get machines -n openshift-machine-api -o wide                           
NAME                    PHASE         TYPE   REGION   ZONE   AGE     NODE                                 PROVIDERID   STATE
ostest-master-0         Running                              49m     master-0.ostest.test.metalkube.org                
ostest-master-1         Running                              49m     master-1.ostest.test.metalkube.org                
ostest-master-2         Running                              49m     master-2.ostest.test.metalkube.org

The status has all the IP info and node ref:

status:
  addresses:
  - address: fd00:1101::2c
    type: InternalIP
  - address: fd2e:6f44:5dd8:c956::14
    type: InternalIP
  - address: master-0.ostest.test.metalkube.org
    type: Hostname
  - address: master-0.ostest.test.metalkube.org
    type: InternalDNS
  lastUpdated: "2020-05-14T20:31:33Z"
  nodeRef:
    kind: Node
    name: master-0.ostest.test.metalkube.org
    uid: 3f3bbf75-d6cc-4111-9016-d034b16ded87

imain · 2020-05-14T23:10:02Z

🤘 ;-)

…BareMetalHost Right now we don't get any introspection data about the masters because they are provisioned by the bootstrap node and the information is lost. This patch adds a script to pull the introspection data from ironic running on the bootstrap and places it in the BareMetalHost CRs for the masters. This is done via an annotation as we cannot directly update the Status in BMH. The annotation is picked up by the BMO and added to the BMH CRs. There are several bugs filed about this: openshift-metal3/dev-scripts#917 https://bugzilla.redhat.com/show_bug.cgi?id=1801238

stbenjam · 2020-05-15T00:46:40Z

/lgtm

abhinavdahiya · 2020-05-15T01:52:16Z

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh

+# shellcheck disable=SC1091
+. /usr/local/bin/release-image.sh
+
+export KUBECONFIG=/etc/kubernetes/kubeconfig


try to use https://github.com/openshift/installer/blob/master/data/data/bootstrap/systemd/units/approve-csr.service#L7

abhinavdahiya · 2020-05-15T01:53:59Z

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh

+done
+
+# Wait for a master to appear.
+while [ "$(curl -s http://localhost:6385/v1/nodes | jq '.nodes[] | .uuid' | wc -l)" -lt 1 ]; do


also do we expect the RHCOS image to ship jq ? that's odd.

Why’s that odd? It’s there.

abhinavdahiya · 2020-05-15T01:55:08Z

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh

+    # the BareMetalHost CRs as annotations, which BMO then picks up.
+    HARDWARE_DETAILS=$(podman run --quiet --net=host \
+        --rm \
+        --name baremetal-operator \


do you need to use a specific name? there are times where names container will fail if previous one was not cleaned up..

abhinavdahiya · 2020-05-15T01:57:08Z

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh

+        "${BAREMETAL_OPERATOR_IMAGE}" \
+        http://localhost:5050/v1 "$node" | jq '{hardware: .}')
+
+     oc annotate --overwrite -n openshift-machine-api baremetalhosts "$name" 'baremetalhost.metal3.io/status'="$HARDWARE_DETAILS"


what if you get an error when trying to update, like maybe machine-api isn't registered yet?

The first thing we check at the start of the script is for the presence of the CR’s

openshift-ci-robot · 2020-05-15T01:58:51Z

@imain: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-metal-ipi	`73c52db`	link	`/test e2e-metal-ipi`
ci/prow/e2e-ovirt	`fb6b325`	link	`/test e2e-ovirt`
ci/prow/e2e-openstack	`fb6b325`	link	`/test e2e-openstack`
ci/prow/e2e-aws-scaleup-rhel7	`fb6b325`	link	`/test e2e-aws-scaleup-rhel7`
ci/prow/e2e-libvirt	`fb6b325`	link	`/test e2e-libvirt`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

abhinavdahiya · 2020-05-15T02:02:03Z

/approve
this script looks very brittle to me.

This is done via an annotation as we cannot directly
update the Status in BMH.

to update the status subresources, you need to use the k8s api clients and not kubectl/oc.. and i think it would be worth it imo to move code to go if you can instead of this bash.

Right now we don't get any introspection data about the masters
because they are provisioned by the bootstrap node and the information
is lost.

i'm really confused how the data get's lost. couldn't we get the same data from ironic service when the controller is running on the control-plane? It's just not clear to me.

openshift-ci-robot · 2020-05-15T02:02:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

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~~ [abhinavdahiya]

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

openshift-ci-robot · 2020-05-15T02:06:26Z

@imain: All pull requests linked via external trackers have merged: openshift/installer#3591. Bugzilla bug 1801238 has been moved to the MODIFIED state.

In response to this:

Baremetal: Bug 1801238: Pull data from ironic inspector and annotate BareMetalHost

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

stbenjam · 2020-05-15T02:42:40Z

i'm really confused how the data get's lost. couldn't we get the same data from ironic service when the controller is running on the control-plane?

We would have to run inspection again.

Plans to collect the data in the cluster from ironic were outright rejected because of the overlap with node-exporter, even though it’s not the same data and we need the data to even make the node/machine link.

The ironic database is MySQL, and we treat it as ephemeral. It’s database isn’t migrated to the cluster. That works fine for everything except inspection information.

We are aware you can use the k8s API. But the metal3 community explicitly created the annotation for this purpose to make it easy to move hardware between clusters. I don’t see why we shouldn’t use it here as the use case is very similar.

Trust me when I say we’ve invested a significant amount of time in this problem and we had many other better solutions that didn’t pan out. This was the best fix to address the problem in the short term.

hardys · 2020-05-15T08:10:12Z

i'm really confused how the data get's lost. couldn't we get the same data from ironic service when the controller is running on the control-plane? It's just not clear to me.

@abhinavdahiya the introspection data can only be collected by rebooting the nodes, which normally happens right before deployment - the same ramdisk we use to deploy RHCOS is what collects this data.

For the masters, this happens via terraform, using ironic on the bootstrap VM, so by the time the masters are all up and running, the bootstrap VM is gone and we can't reboot/redeploy the controlplane just to collect the introspection data.

So the ironic service running on the cluster doesn't have this data for the masters (the master nodes are "adopted" by the cluster ironic, which doesn't do any introspection). This PR wires the data from the bootstrap VM into the BMH objects created by the installer for the masters.

stbenjam · 2020-05-26T11:33:07Z

/cherry-pick release-4.4

openshift-cherrypick-robot · 2020-05-26T11:33:26Z

@stbenjam: new pull request created: #3656

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This was mentioned in openshift#3591 but not addressed during the review before merging

This was also mentioned in openshift#3591 as a potential issue in the case where a restart of the script/unit results in a stale container still running with a conflicting name. Since we specify --rm the container is removed so having the human readable name for debugging isn't an issue.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 13, 2020

openshift-ci-robot requested review from hardys and markmc May 13, 2020 02:05

stbenjam reviewed May 13, 2020

View reviewed changes

data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template Outdated Show resolved Hide resolved

stbenjam reviewed May 13, 2020

View reviewed changes

data/data/baremetal/masters/main.tf Show resolved Hide resolved

stbenjam reviewed May 13, 2020

View reviewed changes

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh Outdated Show resolved Hide resolved

imain force-pushed the installer_inspection branch 3 times, most recently from bbf11a7 to be75551 Compare May 13, 2020 19:43

openshift-ci-robot assigned hardys May 13, 2020

imain force-pushed the installer_inspection branch from be75551 to 971ce92 Compare May 13, 2020 19:54

stbenjam suggested changes May 13, 2020

View reviewed changes

imain force-pushed the installer_inspection branch from 971ce92 to 963c56c Compare May 13, 2020 22:59

stbenjam reviewed May 14, 2020

View reviewed changes

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh Outdated Show resolved Hide resolved

stbenjam reviewed May 14, 2020

View reviewed changes

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh Outdated Show resolved Hide resolved

stbenjam reviewed May 14, 2020

View reviewed changes

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh Show resolved Hide resolved

stbenjam reviewed May 14, 2020

View reviewed changes

data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh Show resolved Hide resolved

imain force-pushed the installer_inspection branch 2 times, most recently from 2e0a558 to 472dcd7 Compare May 14, 2020 18:24

imain changed the title ~~WIP: Baremetal: Pull data from ironic inspector and annotate BareMetalHost~~ Baremetal: Pull data from ironic inspector and annotate BareMetalHost May 14, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 14, 2020

openshift-ci-robot added the platform/baremetal IPI bare metal hosts platform label May 14, 2020

imain force-pushed the installer_inspection branch 4 times, most recently from b10994a to 73c52db Compare May 14, 2020 19:26

openshift-ci-robot assigned abhinavdahiya May 14, 2020

abhinavdahiya reviewed May 14, 2020

View reviewed changes

stbenjam reviewed May 14, 2020

View reviewed changes

imain force-pushed the installer_inspection branch from 73c52db to fb6b325 Compare May 15, 2020 00:05

openshift-ci-robot assigned stbenjam May 15, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2020

abhinavdahiya reviewed May 15, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2020

openshift-merge-robot merged commit aecb920 into openshift:master May 15, 2020

openshift-cherrypick-robot mentioned this pull request May 26, 2020

[release-4.4] Baremetal: Bug 1840106: Pull data from ironic inspector and annotate BareMetalHost #3656

Merged

hardys mentioned this pull request Jul 2, 2020

Proposal to perform continuous HW introspection on Baremetal IPI hosts openshift/enhancements#229

Closed

hardys pushed a commit to hardys/installer that referenced this pull request Jul 28, 2020

baremetal: use kubeconfig-loopback in master-bmh-update.sh

bb90dc4

This was mentioned in openshift#3591 but not addressed during the review before merging

hardys mentioned this pull request Jul 28, 2020

baremetal: Make master-bmh-update script more robust #3969

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Baremetal: Bug 1801238: Pull data from ironic inspector and annotate BareMetalHost #3591

Baremetal: Bug 1801238: Pull data from ironic inspector and annotate BareMetalHost #3591

imain commented May 13, 2020 •

edited

imain commented May 13, 2020

stbenjam left a comment

imain commented May 13, 2020

stbenjam commented May 14, 2020

stbenjam commented May 14, 2020

imain commented May 14, 2020

abhinavdahiya May 14, 2020

imain May 14, 2020

stbenjam May 14, 2020

stbenjam May 14, 2020

imain May 14, 2020

imain May 14, 2020

stbenjam May 14, 2020 •

edited

imain May 15, 2020 •

edited

stbenjam commented May 14, 2020 •

edited

imain commented May 14, 2020

stbenjam commented May 15, 2020

abhinavdahiya May 15, 2020

abhinavdahiya May 15, 2020

stbenjam May 15, 2020

abhinavdahiya May 15, 2020

abhinavdahiya May 15, 2020

stbenjam May 15, 2020 •

edited

openshift-ci-robot commented May 15, 2020

abhinavdahiya commented May 15, 2020

openshift-ci-robot commented May 15, 2020

openshift-ci-robot commented May 15, 2020

stbenjam commented May 15, 2020 •

edited

hardys commented May 15, 2020

stbenjam commented May 26, 2020

openshift-cherrypick-robot commented May 26, 2020

Baremetal: Bug 1801238: Pull data from ironic inspector and annotate BareMetalHost #3591

Baremetal: Bug 1801238: Pull data from ironic inspector and annotate BareMetalHost #3591

Conversation

imain commented May 13, 2020 • edited

imain commented May 13, 2020

stbenjam left a comment

Choose a reason for hiding this comment

imain commented May 13, 2020

stbenjam commented May 14, 2020

stbenjam commented May 14, 2020

imain commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stbenjam May 14, 2020 • edited

Choose a reason for hiding this comment

imain May 15, 2020 • edited

Choose a reason for hiding this comment

stbenjam commented May 14, 2020 • edited

imain commented May 14, 2020

stbenjam commented May 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stbenjam May 15, 2020 • edited

Choose a reason for hiding this comment

openshift-ci-robot commented May 15, 2020

abhinavdahiya commented May 15, 2020

openshift-ci-robot commented May 15, 2020

openshift-ci-robot commented May 15, 2020

stbenjam commented May 15, 2020 • edited

hardys commented May 15, 2020

stbenjam commented May 26, 2020

openshift-cherrypick-robot commented May 26, 2020

imain commented May 13, 2020 •

edited

stbenjam May 14, 2020 •

edited

imain May 15, 2020 •

edited

stbenjam commented May 14, 2020 •

edited

stbenjam May 15, 2020 •

edited

stbenjam commented May 15, 2020 •

edited