pass featuregate args to config-operator to get rendered featuregates

[deads2k]

For openshift/enhancements#1373

I could probably live with this having to be vendored sometimes, but it would make me sad.

Part 5 of the three step plan to make individual featuregates observable via the API. This was previously avoided because when featuregates get promoted from TechPreview to Default, during an upgrade, an old operator may try to enable a TechPreview variant of a feature.

This builds on openshift/cluster-config-operator#288 with a goal of making the development-time flow

1. update openshift/api with new featuregate
2. vendor into near zero-dep openshfit/cluster-config-operator (automated someday?)
3. done.

with a runtime flow of

1. installer builds the featuregate.yaml with spec completed
2. bootkube passes featuregate.yaml to cluster-config-operator render
3. cluster-config-operator render completes status and overwrites the featuregate.yaml
4. other operator in render receive featuregate.yaml and consume featuregates from that status
5. cluster-bootstrap writes both the spec and status
6. in the running cluster, cluster-config-operator consistently writes the featuregate.status

shortcomings

• There is some generation I don't know how to do.
• It relies on add featuregate status cluster-config-operator#288 to work and that hasn't merged
• I don't know if I got the filename write
• I don't know if a file can be overwritten during rendering
• I don't know where the other input files come from
• Technically I could leave this after the CVO and get 90% of the benefit. If we have ordering problems I will do so to get unstuck.

[patrickdillon]

/assign

I will try to dig in to assist a bit more here and in openshift/cluster-config-operator#288

[JoelSpeed]

Technically I could leave this after the CVO and get 90% of the benefit. If we have ordering problems I will do so to get unstuck.

I'm not sure this gotcha is true, if you leave it until after the CVO, there may be a diff between the FGs observed by the MCO on render and on first boot of the operator, if this happens, the cluster fails to bootstrap. You'd have to make sure that the FGs are consistent in the view of MCO until after it has rendered the first machine config into the cluster. The one that is rendered is not persisted in the API

[JoelSpeed]

Does this file actually have to be called this? In other render commands I've seen they search through the manifests folder and look at a partial object meta to identify if the file is a featuregate. Pre the installer supporting feature gates, it could have been called whatever I liked, nothing to say that's not still the case for some who have set up clusters without FG support in the installer before

[patrickdillon]

This is a good point, spelling it out a bit more...

This PR will make the installer always lay down a feature set manifest. If there is a conflicting feature set manifest the installer will throw an error pre-install.

It would definitely be possible for a user to delete the installer-generated manifest. But in those cases only the feature set spec would be set and not the status, right?

[deads2k]

It would definitely be possible for a user to delete the installer-generated manifest. But in those cases only the feature set spec would be set and not the status, right?

Yes. sometimes that's acceptable and the customer should know. I'm open to future refinement in 4.14, but I'd like to unstick azure ccm.

[JoelSpeed]

If we are going to merge as is, I think we should probably check how many CI jobs rely on the ability to lay down a feature gate manifest blindly. I suspect it's non-zero and that this would break anyone currently relying on that mechanism

[JoelSpeed]

A quick scan of the release repo leads me to:

• https://github.com/openshift/release/blob/bb21ccca00ad923ea66912c5897335d48087851d/ci-operator/step-registry/ipi/conf/techpreview/ipi-conf-techpreview-commands.sh#L4
• https://github.com/openshift/release/blob/bb21ccca00ad923ea66912c5897335d48087851d/ci-operator/step-registry/ipi/conf/customnoupgrade/ipi-conf-customnoupgrade-commands.sh#L14
  Both of which, IIUC, set up a feature gate manifest under a custom name during the openshift-install manifest stage. The current logic wouldn't pick these up and would invalidate all techpreview and customnoupgrade CI jobs we have.

Either we need to rename those to fit the pattern, or fix the logic to run a search in CCO of the manifest dir (the latter being the preferred long term IMO)

[deads2k]

/test all

[patrickdillon]

This seems sane to me.

There is some generation I don't know how to do.

Is this still an issue?

I don't know if I got the filename write

LGTM

I don't know if a file can be overwritten during rendering

The pattern in the PR should work well.

I don't know where the other input files come from

It doesn't seem like this is a blocker?

[patrickdillon]

This is a good point, spelling it out a bit more...

This PR will make the installer always lay down a feature set manifest. If there is a conflicting feature set manifest the installer will throw an error pre-install.

It would definitely be possible for a user to delete the installer-generated manifest. But in those cases only the feature set spec would be set and not the status, right?

[deads2k]

I have pushed an update to set the VERSION, which will prevent flapping during install.

[JoelSpeed]

/hold until we've clarified if this will break TPNU and CNU variants of CI jobs

[deads2k]

code worked in https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_installer/6990/pull-ci-openshift-installer-master-e2e-aws-ovn/1650960438084505600/artifacts/e2e-aws-ovn/gather-must-gather/artifacts/

[JoelSpeed]

/hold cancel

CI jobs are fixed, we have PRs up for the CCO changes I wanted to see, I think this is ready

/lgtm

[smg247]

/test all

[deads2k]

success will have

    - apiVersion: config.openshift.io/v1
      fieldsType: FieldsV1
      fieldsV1:
        f:status:
          .: {}
          f:featureGates:
            .: {}
            k:{"version":"4.14.0-0.ci.test-2023-04-25-223643-ci-op-4mjy16rb-latest"}:
              .: {}
              f:disabled: {}
              f:enabled: {}
              f:version: {}
      manager: cluster-bootstrap
      operation: Update
      subresource: status

in featuregates (from https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_installer/6990/pull-ci-openshift-installer-master-e2e-aws-ovn/1650960438084505600)

The current failed

    - apiVersion: config.openshift.io/v1
      fieldsType: FieldsV1
      fieldsV1:
        f:status: {}
      manager: cluster-bootstrap
      operation: Update
      subresource: status
      time: "2023-04-27T21:48:16Z"
    - apiVersion: config.openshift.io/v1
      fieldsType: FieldsV1
      fieldsV1:
        f:status:
          f:featureGates:
            .: {}
            k:{"version":"4.14.0-0.ci.test-2023-04-27-213820-ci-op-zfiwkxdz-latest"}:
              .: {}
              f:disabled: {}
              f:enabled: {}
              f:version: {}
      manager: cluster-config-operator
      operation: Update
      subresource: status
      time: "2023-04-27T21:52:20Z"

the cluster-config-operator had to overwrite it in https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_installer/6990/pull-ci-openshift-installer-master-e2e-aws-ovn/1651685466501550080

I need to figure out whether it was the version arg or something else

[deads2k]

/hold until I get that worked out.

[deads2k]

/retest

[deads2k]

/retest

[deads2k]

/hold cancel

this one is back to installing correctly. I'd like to unstick the external cloud provider with this and continue to work on handling different filenames later.

[deads2k]

bug is the extgernal cloud provider revert.

[patrickdillon]

/retest

[patrickdillon]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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 [patrickdillon]

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

[patrickdillon]

/skip
hopefully this will abort the unnecessary retest

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD 2ffd73e and 2 for PR HEAD 07da028 in total

[openshift-ci]

@deads2k: The following tests 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/okd-scos-e2e-aws-upgrade	dec02f912d5d077824bacd5b4adc718002a8c533	link	false	/test okd-scos-e2e-aws-upgrade
ci/prow/e2e-aws-ovn-workers-rhel8	dec02f912d5d077824bacd5b4adc718002a8c533	link	false	/test e2e-aws-ovn-workers-rhel8
ci/prow/e2e-aws-ovn-upgrade	dec02f912d5d077824bacd5b4adc718002a8c533	link	false	/test e2e-aws-ovn-upgrade
ci/prow/okd-e2e-aws-ovn	07da028	link	false	/test okd-e2e-aws-ovn
ci/prow/okd-e2e-aws-ovn-upgrade	07da028	link	false	/test okd-e2e-aws-ovn-upgrade

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

[openshift-bot]

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-installer-container-v4.14.0-202304290741.p0.g8e8fb72.assembly.stream for distgit ose-installer.
All builds following this will include this PR.

[openshift-bot]

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-baremetal-installer-container-v4.14.0-202304290741.p0.g8e8fb72.assembly.stream for distgit ose-baremetal-installer.
All builds following this will include this PR.

[openshift-bot]

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-installer-artifacts-container-v4.14.0-202304290741.p0.g8e8fb72.assembly.stream for distgit ose-installer-artifacts.
All builds following this will include this PR.