ARO-9570: Add a controller to the ARO operator to lay down etchosts #3781
Conversation
lranjbar
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
SudoBrendan,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig,
tiguelu,
SrinivasAtmakuri,
mociarain,
kimorris27 and
tsatam
as code owners
lranjbar
added
release-blocker
next-release
lranjbar
force-pushed
the
lranjbar/ARO-9570-udr-etchosts-operator-patch
branch
from
0456936
to
9a4ad4a
Compare
| b, err := json.Marshal(ignConfig) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // canonicalise the machineconfig payload the same way as MCO | ||
| var i interface{} | ||
| err = json.Unmarshal(b, &i) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| rawExt := runtime.RawExtension{} | ||
| rawExt.Raw, err = json.Marshal(i) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &mcfgv1.MachineConfig{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: mcfgv1.SchemeGroupVersion.String(), | ||
| Kind: "MachineConfig", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: fmt.Sprintf("99-%s-aro-etc-hosts-gateway-domains", role), | ||
| Labels: map[string]string{ | ||
| "machineconfiguration.openshift.io/role": role, | ||
| }, | ||
| }, | ||
| Spec: mcfgv1.MachineConfigSpec{ | ||
| Config: rawExt, | ||
| }, | ||
| }, nil |
There was a problem hiding this comment.
Not strictly necessary for this PR if we're in a rush to get this merged, but this portion seems like it could be factored out into a shared utility function that accepts the ignition config, the role, and the MachineConfig name and returns the MachineConfig.
|
we have to change this file as well to add this operator to aro-operator, don't we? Lines 96 to 101 in 1b6e0e7 |
lranjbar
force-pushed
the
lranjbar/ARO-9570-udr-etchosts-operator-patch
branch
from
0fe87e7
to
dcd741e
Compare
lranjbar
force-pushed
the
lranjbar/ARO-9570-udr-etchosts-operator-patch
branch
from
dcd741e
to
7361eae
Compare
| err = r.Client.Get(ctx, types.NamespacedName{Namespace: "openshift-machine-api", Name: "99-master-aro-etc-hosts-gateway-domains"}, mc) | ||
| if kerrors.IsNotFound(err) { |
There was a problem hiding this comment.
Regarding the error handling here. It looks like we will drop any error from line 69 that isn't a kerrors.IsNotFound error. Should we make changes to handle other errors on the Get?
Here's an example of where we use IsNotFound, but then ensure to catch any other errors too:
https://github.com/Azure/ARO-RP/blob/master/pkg/operator/controllers/autosizednodes/autosizednodes_controller.go#L79-L91
|
|
||
| // If 99-worker-aro-etc-hosts-gateway-domains doesn't exist, create it | ||
| err = r.Client.Get(ctx, types.NamespacedName{Namespace: "openshift-machine-api", Name: "99-worker-aro-etc-hosts-gateway-domains"}, mc) | ||
| if kerrors.IsNotFound(err) { |
There was a problem hiding this comment.
Same comment as above.
|
/azp run ci,e2e |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Which issue this PR addresses:
Fixes ARO-9570
What this PR does / why we need it:
This PR adds a MachineConfigs for the ARO operator to lay down:
99-master-aro-etc-hosts-gateway-domains
99-worker-aro-etc-hosts-gateway-domains
During the UDR installation incident we have gone back and forth on how resolve the root issue. The root cause is that the gateway domains are not populated for OpenShift images to be pulled and installed. These domains are IPs so we can pass them into
/etc/hoststo resolve them before other DNS resolution options are available.Test plan for issue:
I've some minimal smoke testing on the operator with this PR.
Is there any documentation that needs to be updated for this PR?
How do you know this will function as expected in production?