modules: attempt to make bootstrap more robust #244
Conversation
This removes some unneeded code.
Make this look a little more like shell.
openshift-ci-robot
added
the
size/L
openshift-ci-robot
added
the
approved
There were a number of strange and unneeded operations being done.
This runs bootkube.service and tectonic.service repeatedly until they succeed. This isn't going to work in its current form (bootkube still has trouble being restarted) but it gets us closer.
| --output=/assets/kco-bootstrap | ||
|
|
||
| cp --recursive "$PWD/kco-bootstrap/bootstrap-configs" /etc/kubernetes/bootstrap-configs | ||
| cp --recursive "$PWD/kco-bootstrap/bootstrap-configs" . |
There was a problem hiding this comment.
Muscle memory. Both should work.
| --output=/assets/kco-bootstrap | ||
|
|
||
| cp --recursive "$PWD/kco-bootstrap/bootstrap-configs" /etc/kubernetes/bootstrap-configs | ||
| cp --recursive "$PWD/kco-bootstrap/bootstrap-configs" . |
There was a problem hiding this comment.
cp --recursive "$PWD/kco-bootstrap/bootstrap-configs" /etc/kubernetes/bootstrap-configs
cp --recursive "$PWD/kco-bootstrap/bootstrap-configs" .the end goal is to end up at /etc/kubernetes/bootstrap-configs;
why do both?
or atleast copy to $PWD and then copy $PWD one to /etc/kubernetes/bootstrap-configs
There was a problem hiding this comment.
The second cp was to satisfy the conditional above. I thought those files in /etc were removed by kubelet. If not, we could just read /etc in the conditional.
There was a problem hiding this comment.
No the /etc/kubernetes/bootstrap-configs is not deleted by kubelet.
It is read by bootstrap-control-plane components
There was a problem hiding this comment.
We can
- Change conditional to /etc and drop cp .
- Keep it as is, no big deal
There was a problem hiding this comment.
Keeping it as is, seems okay for now. 👌
|
@crawford few comments otherwise |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/lgtm /hold cancel |
openshift-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, crawford The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| mkdir -p /etc/ssl/mcs/ | ||
| mkdir --parents /etc/kubernetes/manifests/ | ||
| mkdir --parents /etc/mcc/bootstrap/ | ||
| mkdir --parents /etc/ssl/mcs/ |
There was a problem hiding this comment.
I'm generally in favor of long optipns and spelling things out to make source more readable. But only -p is in POSIX, so I think we want to roll this particular change back for portability.
| # 2. read the default MachineConfigPools rendered by MachineConfigOperator | ||
| # 3. read any additional MachineConfigs that are needed for the default MachineConfigPools. | ||
| cp -r "$PWD/mco-bootstrap/manifests" /etc/mcc/bootstrap/manifests | ||
| cp --recursive "$PWD/mco-bootstrap/manifests" /etc/mcc/bootstrap/manifests |
There was a problem hiding this comment.
News to me, but POSIX.1-2017 dropped -r in favor of -R and does not define a long form. They give some notes on -r vs. -R in the RATIONALE section.
| @@ -1,30 +1,20 @@ | |||
| #!/bin/sh | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
I'm fine with Bash when we need its extensions, but I'd rather stick to the POSIX shell where possible. Is this just for your <<< here-strings?
| set -e | ||
|
|
||
| if echo "$out" | grep -q "AlreadyExists" | ||
| if grep --quiet "AlreadyExists" <<< "$out" |
There was a problem hiding this comment.
And while I'd prefer the here-string form if it was in POSIX, the difference isn't large enough to make it worth breaking with POSIX-compat (for me, anyway).
| if grep --quiet "AlreadyExists" <<< "$out" | ||
| then | ||
| echo "$out, skipping" | ||
| (>&2 echo "$out, skipping") |
There was a problem hiding this comment.
You shouldn't need a subshell here. Do you remember why you added it?
And while I prefer echo "..." >&2 (looks like pushing the string into a pipe ;), I've also seen echo >&2 "...". I haven't seen folks leading with the redirect (or I see it so rarely I'vd forgotten ;).
There was a problem hiding this comment.
I was worried about the side effect of rewriting that fd, so I put it in a subshell. Let me test this the way you'd written it and follow up.
|
|
||
| stat=$(echo "$out"| tail -n +2 | grep -v '^Running') | ||
| if [ -z "$stat" ] | ||
| if ! grep --invert-match '^Running' <<< "$out" |
There was a problem hiding this comment.
And you might want to dump grep's stdout into /dev/null, unless you're intentionally writing it to let the caller know where we're at.
There was a problem hiding this comment.
Printing the progress is an intentional side-effect.
| kubectl create --filename updater/app-version-kind.yaml | ||
| kubectl create --filename updater/migration-status-kind.yaml | ||
|
|
||
| kubectl --namespace=tectonic-system get customresourcedefinition channeloperatorconfigs.tco.coreos.com |
There was a problem hiding this comment.
Now that you've dropped wait_for_crd, is the idea that the script will die here if your one-shot request fails, and we'll run through it all again when systemd gives it another pass?
There was a problem hiding this comment.
This is the equivilent to wait_for_crd. kubelet will loop until it's successful.
|
|
||
| cp --recursive "$PWD/bootstrap-configs" /etc/kubernetes/bootstrap-configs | ||
| podman rm --force etcd-signer | ||
| rm --force /etc/kubernetes/manifests/machineconfigoperator-bootstrap-pod.yaml |
There was a problem hiding this comment.
For RHCOS-only scripts, should we care? For developer scripts, I agree that we should stick with POSIX.
There was a problem hiding this comment.
For RHCOS-only scripts, should we care?
Is this RHCOS-only? Is there a BYO-something that might swap out a different POSIX-compat rm (or cp, etc., see the other comments in my review) that doesn't support the long options (e.g. BusyBox)? Using -f instead of --force seems like a small enough price to avoid having to worry about that sort of thing ;).
There was a problem hiding this comment.
Yes, bootstrap will always be an OS we control. BYOH will use a different method for bootstrapping.
5 participants
This runs bootkube.service and tectonic.service repeatedly until they
succeed. This isn't going to work in its current form (bootkube still
has trouble being restarted) but it gets us closer.