RFE: Patch Installer Manifests #2497
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo 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 |
openshift-ci
bot
added
the
approved
| - `ManifestSelector` supports one subfield, `Glob`, which accepts a path matching string as | ||
| supported by golang's [filepath.Glob](https://pkg.go.dev/path/filepath#Glob). | ||
| - We will execute the glob relative to the working directory in which manifests were generated. | ||
| - For security, we will error on any glob starting with `/` or containing `../`. | ||
| - `ManifestSelector` is extensible. | ||
| E.g. in the future we may wish to match manifests based on the GVK of the object therein. |
There was a problem hiding this comment.
@patrickdillon I would really like your input here! What should the initial mechanism be for identifying the manifest(s) to be patched?
There was a problem hiding this comment.
Discussed offline with Patrick. We're going to go with file glob for the first pass.
There was a problem hiding this comment.
@2uasimojo and I discussed over slack, but recording here for posterity. Eric identified filename (glob) or GVK as two options.
The installer has existing code which will deserialize all manifests (using filename globs) and convert them into k8s client.Objects using gvk:
That said, the advantages of using GVK are not immmediately obvious.
The manifest filenames should be rather stable, and we're brainstorming whether there are methods to make that even more guaranteed.
|
/cc @abraverm Thought you might be interested to see how ClusterDeploymentCustomization is evolving :) |
With this enhancment and cluster template operator, a fully automated UPI might be possible. |
| - `ManifestSelector` is extensible. | ||
| E.g. in the future we may wish to match manifests based on the GVK of the object therein. | ||
|
|
||
| ### `ClusterDeployment.Spec.Provisioning.CustomizationRef` |
There was a problem hiding this comment.
Self-
We should fix up the clusterpool controller to automatically add this ref from the inventory to the CD. That way the clusterpool user could do inventory things in either/both ways (install-config and/or manifest patching). Note: requires it not to be a failure if a CDC reffed here contains no manifest patches? Or maybe we can be strict for non-pool clusters? Is it a problem if a non-pool CD uses installConfigPatches??
-Self
| **TBD.** | ||
|
|
||
| Where/how do we report the success/failure of | ||
| - CDC as a whole? In a new `CD.Status.Conditions[?Type=CustomizationsSuccessful]` or similar? |
There was a problem hiding this comment.
Right now the conditions are in CDC and CP, the later has a report for the status of all CDC in its inventory. CD has reference to the CDC itself, I don't know if CD should keep a copy of the error in its own conditions.
| Where/how do we report the success/failure of | ||
| - CDC as a whole? In a new `CD.Status.Conditions[?Type=CustomizationsSuccessful]` or similar? | ||
| - Individial `CDC.InstallerManifestPatches`? | ||
| How are we doing it for `InstallConfigPatches` for ClusterPools? |
There was a problem hiding this comment.
CDC has ApplySucceededCondition, and any issue reported in the condition message.
|
|
||
| - Is it a failure if `ManifestSelector` matches zero manifests? | ||
| - Is it a failure if a `Patches[].Path` matches no paths? | ||
| - Failure to execute a patch should probably bubble up as an overall failure. |
There was a problem hiding this comment.
In case of InstallConfigPatches, the CP resolution fails and the failure is saved in the CDC which causes CP to pick a different CDC in the next attempt.
Add an enhancement doc proposing API changes to allow hive consumers to specify arbitrary edits to the manifests generated by `openshift-install create manifests` prior to feeding them to `openshift-install create cluster`. HIVE-1793
2uasimojo
force-pushed
the
HIVE-1793/rfe-manifest-patch
branch
from
bb4e206
to
3f0e45e
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Finished this out alongside the code (#2499). Ready for review. |
Add an enhancement doc proposing API changes to allow hive consumers to specify arbitrary edits to the manifests generated by
openshift-install create manifestsprior to feeding them toopenshift-install create cluster.HIVE-1793