Support patching installer manifests #2499

2uasimojo · 2024-10-23T22:06:47Z

With this change, you can use new API field ClusterDeployment.Spec.Provisioning.CustomizationRef to point to a ClusterDeploymentCustomization (hereinafter "CDC") object in the same namespace as the ClusterDeployment (CD).

ClusterDeploymentCustomizations:
CDC accepts a new subfield, Spec.InstallerManifestPatches, which consists of:

Glob: a string representing a file glob, relative to the installer working directory, matching one or more manifest files.
Patches: a list of PatchEntity representing RFC6902 JSON patches to apply to the matched manifest(s).

Also, I got really annoyed having to type out clusterdeploymentcustomizations on the CLI, so I added abbreviation cdc to the schema.

ClusterPools:
CDC was already being used by ClusterPool-owned CDs to allow patching the install-config generated from the template referred to by ClusterPool.Spec.InstallConfigSecretTemplateRef. With this change, ClusterPool-owned CDs can start using manifest patches in two ways (not mutually exclusive):

Patches specific to the CD can be included in the InstallerManifestPatches field of the existing Inventory CDCs.
Patches applicable to all CDs in the pool can be provided by a CDC referenced via a new ClusterPool.Spec.CustomizationRef field.

HIVE-1793

openshift-ci · 2024-10-23T22:07:17Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [2uasimojo]

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

2uasimojo · 2024-10-23T22:10:24Z

/test e2e

2uasimojo · 2024-11-09T23:50:22Z

This is going well:

efried@efried-thinkpadp16vgen1:~/go/src/github.com/openshift/hive$ oc get cd efried416 -o yaml | yq r - spec.provisioning.customizationRef
name: mycdc

mycdc looks like:

apiVersion: hive.openshift.io/v1
kind: ClusterDeploymentCustomization
metadata:
  name: mycdc
  namespace: efried
spec:
  installerManifestPatches:
  - manifestSelector:
      glob: cluster-api/*/*machine*.yaml
    patches:
    - op: add
      path: /metadata/labels
      valueJSON: |
        {"efried.openshift.io/foo": "bar"}

The installmanager logs include:

time="2024-11-09T23:44:50Z" level=info msg="Found 1 InstallerManifestPatch entries from ClusterDeployment.Spec.CustomizationRef mycdc" installID=qgwtrjmj
time="2024-11-09T23:44:50Z" level=info msg="manifest patch glob 0 (cluster-api/*/*machine*.yaml) matched 8 files" installID=qgwtrjmj
time="2024-11-09T23:44:50Z" level=info msg="patching manifest /output/cluster-api/machines/10_inframachine_efried416-ttb8s-bootstrap.yaml" installID=qgwtrjmj
time="2024-11-09T23:44:50Z" level=info msg="patching manifest /output/cluster-api/machines/10_inframachine_efried416-ttb8s-master-0.yaml" installID=qgwtrjmj
time="2024-11-09T23:44:50Z" level=info msg="patching manifest /output/cluster-api/machines/10_inframachine_efried416-ttb8s-master-1.yaml" installID=qgwtrjmj
time="2024-11-09T23:44:50Z" level=info msg="patching manifest /output/cluster-api/machines/10_inframachine_efried416-ttb8s-master-2.yaml" installID=qgwtrjmj
time="2024-11-09T23:44:50Z" level=info msg="patching manifest /output/cluster-api/machines/10_machine_efried416-ttb8s-bootstrap.yaml" installID=qgwtrjmj
time="2024-11-09T23:44:50Z" level=info msg="patching manifest /output/cluster-api/machines/10_machine_efried416-ttb8s-master-0.yaml" installID=qgwtrjmj
time="2024-11-09T23:44:50Z" level=info msg="patching manifest /output/cluster-api/machines/10_machine_efried416-ttb8s-master-1.yaml" installID=qgwtrjmj
time="2024-11-09T23:44:50Z" level=info msg="patching manifest /output/cluster-api/machines/10_machine_efried416-ttb8s-master-2.yaml" installID=qgwtrjmj

and a sample manifest in the in-progress provision pod was correctly patched:

apiVersion: cluster.x-k8s.io/v1beta1
kind: Machine
metadata:
  creationTimestamp: null
  labels:
    efried.openshift.io/foo: bar    # <=== L@@K
  name: efried416-ttb8s-master-0
spec:
  bootstrap:
    dataSecretName: efried416-ttb8s-master
  clusterName: efried416-ttb8s
  infrastructureRef:
    apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
    kind: AWSMachine
    name: efried416-ttb8s-master-0
status:
  bootstrapReady: false
  infrastructureReady: false

2uasimojo · 2024-11-09T23:51:39Z

I just need to test the clusterpool path (and maybe noodle how the error conditions manifest) and we'll be good to land this.

2uasimojo · 2024-11-09T23:51:54Z

/cc @abraverm

abraverm · 2024-11-11T15:09:49Z

In Clusterpool we do reservation of the CDC, should this mechanism somewhat migrate/copied to CD controller?

2uasimojo · 2024-11-11T15:56:28Z

In Clusterpool we do reservation of the CDC, should this mechanism somewhat migrate/copied to CD controller?

For pool inventory, reserving CDCs makes sense because they're intended to enable exclusive/unique settings such as reserved IP addresses. And manifest-patching CDCs used for clusterpool inventory will still be subject to reservation as usual. But for this use case -- CDs' CustomizationRef-named CDCs -- I think we explicitly want manifest patching to be usable by multiple CDs without restriction. Does that make sense?

codecov · 2024-11-14T00:47:21Z

Codecov Report

Attention: Patch coverage is 63.54167% with 70 lines in your changes missing coverage. Please review.

Project coverage is 49.92%. Comparing base (41f153f) to head (8dfac7a).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/installmanager/installmanager.go	25.00%	29 Missing and 1 partial ⚠️
.../clusterdeployment/clusterdeployment_controller.go	30.00%	28 Missing ⚠️
...g/controller/clusterpool/clusterpool_controller.go	74.46%	11 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2499      +/-   ##
==========================================
+ Coverage   49.86%   49.92%   +0.06%     
==========================================
  Files         281      281              
  Lines       32968    33135     +167     
==========================================
+ Hits        16439    16544     +105     
- Misses      15196    15257      +61     
- Partials     1333     1334       +1

Files with missing lines	Coverage Δ
pkg/controller/clusterpool/collections.go	`75.74% <100.00%> (+0.16%)`	⬆️
pkg/controller/utils/clusterdeployment.go	`77.48% <100.00%> (+6.05%)`	⬆️
pkg/controller/utils/sa.go	`67.90% <ø> (ø)`
...entcustomization/clusterdeploymentcustomization.go	`93.33% <100.00%> (+1.12%)`	⬆️
pkg/test/clusterpool/clusterpool.go	`95.23% <100.00%> (+0.18%)`	⬆️
...shift/hive/apis/hive/v1/clusterdeployment_types.go	`0.00% <ø> (ø)`
...is/hive/v1/clusterdeploymentcustomization_types.go	`0.00% <ø> (ø)`
...m/openshift/hive/apis/hive/v1/clusterpool_types.go	`0.00% <ø> (ø)`
...g/controller/clusterpool/clusterpool_controller.go	`58.04% <74.46%> (+1.45%)`	⬆️
.../clusterdeployment/clusterdeployment_controller.go	`66.15% <30.00%> (-0.88%)`	⬇️
... and 1 more

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

apis/hive/v1/clusterdeployment_types.go

apis/hive/v1/clusterpool_types.go

2uasimojo

Forgot to submit these queued up comments that came out of the live review we did on 11/19.

pkg/controller/clusterdeployment/clusterdeployment_controller.go

pkg/controller/clusterdeployment/clusterdeployment_controller_test.go

pkg/controller/clusterpool/clusterpool_controller.go

pkg/controller/utils/clusterdeployment.go

pkg/installmanager/installmanager.go

openshift-ci · 2025-01-15T20:43:25Z

@2uasimojo: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-vsphere	`c8f29d8`	link	true	`/test e2e-vsphere`

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

With this change, you can use new API field `ClusterDeployment.Spec.Provisioning.CustomizationRef` to point to a ClusterDeploymentCustomization (hereinafter "CDC") object in the same namespace as the ClusterDeployment (CD). ClusterDeploymentCustomizations: CDC accepts a new subfield, `Spec.InstallerManifestPatches`, which consists of: - `Glob`: a string representing a file glob, relative to the installer working directory, matching one or more manifest files. - `Patches`: a list of `PatchEntity` representing RFC6902 JSON patches to apply to the matched manifest(s). Also, I got really annoyed having to type out `clusterdeploymentcustomizations` on the CLI, so I added abbreviation `cdc` to the schema. ClusterPools: CDC was already being used by ClusterPool-owned CDs to allow patching the install-config generated from the template referred to by `ClusterPool.Spec.InstallConfigSecretTemplateRef`. With this change, ClusterPool-owned CDs can start using manifest patches in two ways (not mutually exclusive): - Patches specific to the CD can be included in the `InstallerManifestPatches` field of the existing Inventory CDCs. - Patches applicable to all CDs in the pool can be provided by a CDC referenced via a new ClusterPool.Spec.CustomizationRef field. HIVE-1793

suhanime

Post merge review
Did 2 passes so I think we're good? Just a few nits/minor changes if you feel a fup is worthwhile @2uasimojo

suhanime · 2025-03-13T14:46:07Z

apis/hive/v1/clusterdeployment_types.go

+	// CustomizationRef is a reference to a ClusterDeploymentCustomization containing
+	// InstallerManifestPatches to be applied to the manifests generated by openshift-install prior
+	// to starting the installation. (InstallConfigPatches will be ignored -- those changes should
+	// be made directly to the install-config.yaml referenced by InstallConfigSecretRef.)


I understand the point of this note, but I feel that cdc related things should be added as a comment right before the definition of cdc struct, that way, in case the cdc is expanded in the future, the comment is more likely to be amended and not go out-of-date

Sorry, I don't follow. This is CD's type file, not CDC's. InstallConfigPatches are not ignored when this is used in an inventory CDC.

Eh it's fine, I'm not too attached to moving the comment either ways. When I commented on it, I was thinking about what if an option 3 is introduced in cdc def in the future, and we forget to edit the comment here to say 3 will also be ignored? But it's fine, you have a similar comment on clusterpool's cdc ref, so we can let it be.

suhanime · 2025-03-13T16:55:03Z

pkg/controller/clusterdeployment/clusterdeployment_controller.go

+					return matchingCDs
+				})),
+	); err != nil {
+		return errors.Wrap(err, "cannot start watch on ClusterDeploymentCustomizations")


Name/namespace here would help

This is an error setting up the Watch() for any instance of CDC. We don't have a ns/name in this context. (The resolution done in the func is only triggered for a specific instance when the Watch() actually pops.)

suhanime · 2025-03-13T17:56:24Z

pkg/controller/clusterpool/clusterpool_controller.go


 	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
+	"k8s.io/apimachinery/pkg/runtime"


nit: Add a space before this line to keep the k8s deps separate

suhanime · 2025-03-13T18:39:14Z

pkg/controller/utils/clusterdeployment.go


 	log "github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"


nit: Not related to your PR but the logrus dep above should be separate from k8s dep

suhanime · 2025-03-13T18:45:34Z

pkg/controller/utils/clusterdeployment.go

+// return a nil object and a nil error (this is not considered an error condition).
+// Any other error -- including if the referenced CDC doesn't exist -- is bubbled up.
+func LoadManifestPatches(c client.Client, cd *hivev1.ClusterDeployment, log log.FieldLogger) ([]hivev1.InstallerManifestPatch, error) {
+	// Leetle helper to avoid chains of conditionals checking for nils along object paths


Typo in Leetle? Or can leave, apparently it was the spelling using in the 1600s. Love the helper!

Heh, this is just me trying to be cute.

2uasimojo · 2025-03-13T20:20:29Z

Post merge review Did 2 passes so I think we're good? Just a few nits/minor changes if you feel a fup is worthwhile @2uasimojo

Thanks @suhanime -- fup via #2598

2uasimojo marked this pull request as draft October 23, 2024 22:07

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2024

openshift-ci bot requested review from jstuever and suhanime October 23, 2024 22:07

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2024

2uasimojo force-pushed the HIVE-1793/manifest-patch branch from 677edab to 6e54c52 Compare October 28, 2024 22:21

2uasimojo force-pushed the HIVE-1793/manifest-patch branch 2 times, most recently from 786df32 to 61ae998 Compare November 9, 2024 23:12

openshift-ci bot requested a review from abraverm November 9, 2024 23:51

2uasimojo force-pushed the HIVE-1793/manifest-patch branch 2 times, most recently from 4c36dc3 to ea46045 Compare November 13, 2024 22:58

2uasimojo marked this pull request as ready for review November 13, 2024 23:01

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024

openshift-ci bot requested a review from dlom November 13, 2024 23:01

2uasimojo force-pushed the HIVE-1793/manifest-patch branch from ea46045 to 4239df0 Compare November 14, 2024 22:50

2uasimojo mentioned this pull request Nov 14, 2024

RFE: Patch Installer Manifests #2497

Merged

suhanime reviewed Nov 19, 2024

View reviewed changes

apis/hive/v1/clusterdeployment_types.go Show resolved Hide resolved

apis/hive/v1/clusterpool_types.go Show resolved Hide resolved

2uasimojo commented Dec 18, 2024

View reviewed changes

2uasimojo force-pushed the HIVE-1793/manifest-patch branch from 4239df0 to c8f29d8 Compare January 6, 2025 20:31

2uasimojo force-pushed the HIVE-1793/manifest-patch branch from c8f29d8 to a44905f Compare February 18, 2025 22:56

2uasimojo force-pushed the HIVE-1793/manifest-patch branch from a44905f to 8dfac7a Compare March 10, 2025 21:39

2uasimojo mentioned this pull request Mar 10, 2025

HIVE-2797: Forbid editing CD ClusterPoolRef CustomizationRef #2597

Merged

openshift-merge-bot bot merged commit 8dfac7a into openshift:master Mar 11, 2025
9 of 10 checks passed

suhanime reviewed Mar 13, 2025

View reviewed changes

2uasimojo deleted the HIVE-1793/manifest-patch branch March 13, 2025 19:57

2uasimojo mentioned this pull request Sep 4, 2025

[WIP] HIVE-2859: Add ManifestsSecretRef to ClusterPool for manifest management #2745

Open

Support patching installer manifests #2499

Support patching installer manifests #2499

Uh oh!

Conversation

2uasimojo commented Oct 23, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Oct 23, 2024

Uh oh!

2uasimojo commented Oct 23, 2024

Uh oh!

2uasimojo commented Nov 9, 2024

Uh oh!

2uasimojo commented Nov 9, 2024

Uh oh!

2uasimojo commented Nov 9, 2024

Uh oh!

abraverm commented Nov 11, 2024

Uh oh!

2uasimojo commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

2uasimojo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openshift-ci bot commented Jan 15, 2025

Uh oh!

Uh oh!

suhanime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2uasimojo commented Mar 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

2uasimojo commented Oct 23, 2024 •

edited by openshift-ci bot

Loading

2uasimojo commented Nov 11, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading