OCPBUGS-13547: [OCPCLOUD-2034] Update Library-go and API for new featuregate changes #3688

JoelSpeed · 2023-04-27T13:11:57Z

- What I did

Update library-go and openshift api.
Fixed up the type changes and the access to feature gates to use the new feature gate access defined in library-go.
Note this means feature gate enabled/disabled decisions are now made by CCO and not distributed by revendoring openshift/api everywhere.

Noticed a mistake in the scheme addKnownTypes, see comment I've left.

Details of feature gate changes in openshift/enhancements#1373

- How to verify it

E2E should pass, all units are passing too.

- Description for the changelog

JoelSpeed · 2023-04-27T13:12:05Z

/test unit

openshift-ci · 2023-04-27T13:12:10Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

deads2k · 2023-04-27T14:43:56Z

pkg/controller/common/controller_context.go

where is this used?

Passed into controllercontext and then passed into the controllers that need it when we call createControllers

deads2k · 2023-04-27T14:44:42Z

pkg/controller/kubelet-config/helpers.go

This is just for testing? if so, please name it as such.

It's not, in the kubelet feature gate this is used to determine if there is a difference between the default features and the current set of features, only when there is a different is the feature gate kubelet config generated

JoelSpeed · 2023-04-27T16:56:01Z

/test unit
/test bootstrap-integration

JoelSpeed · 2023-04-27T16:56:36Z

cmd/machine-config-controller/start.go

The controllers depend on the FeatureGate data so we should wait until they are synced before starting the controllers

JoelSpeed · 2023-04-27T16:58:38Z

cmd/machine-config-controller/start.go

@deads2k Correct me if I'm wrong, but this has to match the value of version.Raw if it hasn't been substituted by a build time arg? (It currently doesn't, will fix)

JoelSpeed · 2023-04-27T17:00:37Z

pkg/apis/machineconfiguration.openshift.io/v1/register.go

You shouldn't be adding resources from other groups to the scheme here, it causes confusion for the decoder (and was breaking/masking issues in tests). The correct thing to fix this is configv1.Install(scheme.Scheme) at some point in the process that was complaining

JoelSpeed · 2023-04-27T17:01:22Z

pkg/controller/bootstrap/bootstrap.go

Installer will now always create a feature gate manifest, the MCO now has to depend on that to make decisions during the bootstrap

JoelSpeed · 2023-04-27T17:02:11Z

pkg/controller/common/controller_context.go

Passed into controllercontext and then passed into the controllers that need it when we call createControllers

JoelSpeed · 2023-04-27T17:03:08Z

pkg/controller/kubelet-config/kubelet_config_controller_test.go

Some controllers don't use the fgAccess so it's ok for those ones to pass a nil through

JoelSpeed · 2023-04-27T17:03:58Z

pkg/controller/kubelet-config/kubelet_config_features.go

Should probably update this at some point to use the new configv1.FeatureGateName instead of a string

JoelSpeed · 2023-04-27T17:05:54Z

pkg/controller/template/render.go

Not all render configs will have the FGAccess, assume the featuregates are off in this case (shouldn't actually matter)

JoelSpeed · 2023-04-27T17:06:48Z

templates/master/01-master-kubelet/_base/files/kubelet.yaml

These were removed from openshift/api

JoelSpeed · 2023-04-27T17:07:21Z

test/e2e-bootstrap/bootstrap_test.go

@deads2k any idea why this doesn't get set on a read?

JoelSpeed · 2023-04-27T17:53:11Z

/test bootstrap-unit

JoelSpeed · 2023-04-28T08:35:15Z

Not expecting the e2e to pass until openshift/installer#6990 has merged

JoelSpeed · 2023-05-03T09:23:47Z

/retest-required

JoelSpeed · 2023-05-03T13:19:18Z

So on upgrade, the MCO is logging some logs that may be useful,

E0503 11:03:57.982847       1 simple_featuregate_reader.go:290] cluster failed with : unable to determine features: missing desired version "machine-config-daemon-4.6.0-202006240615.p0-2058-gb0c56150-dirty" in featuregates.config.openshift.io/cluster

So the version injected for the MCO image in there does not appear to match the versions in the CVO, I wonder if this is a build time issue for CI jobs 🤔

sinnykumari · 2023-05-09T12:49:10Z

To review kubelet-config related changes
/cc @rphillips

sinnykumari · 2023-05-17T07:37:38Z

Retesting since openshift/installer#7160 has been merged now
/retest

JoelSpeed · 2023-06-05T10:05:37Z

/test e2e-hypershift

More fixes merged into HyperShift this morning, let's see how this goes

JoelSpeed · 2023-06-05T10:56:37Z

https://pr-payload-tests.ci.openshift.org/runs/ci/d6bc72d0-0377-11ee-9090-d9743d629fca-0

These both went green now! I'm dropping the EventedPLEG featuregate out of the payload in openshift/cluster-config-operator#317, once that's merged, I'll drop that final commit from my branch and I think we are good to go.

@sergiordlr Did you want to test the branch as it is now or wait until config operator is merged? They achieve the same thing so I think testing now should show the features issue we observed on friday is gone

sinnykumari · 2023-06-05T11:54:16Z

All tech preview jobs are green! ~~Also, openshift/cluster-config-operator#317 has been merged.~~

JoelSpeed · 2023-06-05T13:05:40Z

Hypershift is green too! I'll work on getting the config operator change merged, but I think otherwise, once I drop that final commit, this is good to go

Anyone got any final reservations?

JoelSpeed · 2023-06-05T13:27:08Z

Config operator has now merged, so, I've re-pushed what the actual branch should look like without my hack in place, this is ready to merge as far as I'm concerned now

sinnykumari · 2023-06-05T13:30:22Z

Thanks Joel, good from my end.
@sergiordlr Can you do any final testing needed and add qe_approved label

sinnykumari · 2023-06-05T16:58:07Z

Adding lgtm as well, techpreview and MCO tests are passing on the PR. We will get to know later if we we missed something. Thanks Joel for all the hard work!
/lgtm

Hold will be removed once qe has added qe-approved label. Since, master branch is not gated on qe laebl, we need to manually do this for pre-merge testing.

jkyros

Thanks Joel! I can't say enough about how much we appreciate your work here (vs us having to muddle through doing all of this on our own).

Things I feel are important to note here behavior-wise for the MCO team:

the machine-config-controller and machine-config-operator will both exit 0 immediately when feature gates change after the initial observation (this is probably good because it means we don't have to coordinate some sort of "are our operator and controller using the same set of feature gates" sort of thing)
the machine-config-daemon does not currently react to feature gates. While the operator/controller/daemon all share that same ctrlcommon.CreateControllerContext() that starts the featuregate controller, the daemon never starts the config informer that feeds it (and I don't think the daemon has the RBAC anyway) so it doesn't react to featureGates and won't get interrupted (this is good, right now it doesn't need to)

The MCO probably should at least follow up by:

migrating to klog and away from glog (rather than mix and match -- I assume klog should be the "winner" because it's better maintained)

openshift-ci · 2023-06-05T21:02:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkyros, JoelSpeed, sinnykumari

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~~ [jkyros,sinnykumari]

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

JoelSpeed · 2023-06-06T09:30:22Z

/retest-required

sergiordlr · 2023-06-06T11:39:31Z

Verified using IPI on GCP.

All critical MCO test cases passed.
TechPreviewNoUpgrade was enabled and there was no problem in the cluster. EventedPLEG remained disabled.

  status:
    featureGates:
    - disabled:
      - name: EventedPLEG

These tests passed after enabling the features:

59867-Create files specifying user and group
42365-add real time kernel
54085-Update osImage

CustomNoUpgrade is working properly

  spec:
    customNoUpgrade:
      enabled:
      - AdmissionWebhookMatchConditions
    featureSet: CustomNoUpgrad

We add the qe-approved label:

/label qe-approved

NOTE: We observed that after enabling TechPreviewNoUpgrade the MCO controller pod is restarted and it takes like 3 minutes to acquire the leader lease. Only in that case, if we remove the MCO controller pod the leader lease is acquired immediately. It is not considered a problem though.

JoelSpeed · 2023-06-06T11:42:38Z

QE approved, so can remove the hold now, thanks for your work on this @sergiordlr

JoelSpeed · 2023-06-06T11:42:45Z

/hold cancel

openshift-ci · 2023-06-06T14:15:36Z

@JoelSpeed: 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/e2e-alibabacloud-ovn	`3de0564`	link	false	`/test e2e-alibabacloud-ovn`
ci/prow/okd-scos-e2e-gcp-ovn-upgrade	`3de0564`	link	false	`/test okd-scos-e2e-gcp-ovn-upgrade`
ci/prow/okd-scos-e2e-aws-ovn	`3de0564`	link	false	`/test okd-scos-e2e-aws-ovn`

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.

JoelSpeed · 2023-06-06T14:17:52Z

/retest-required

openshift-ci-robot · 2023-06-06T16:38:24Z

@JoelSpeed: Jira Issue OCPBUGS-13547: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/api#1461 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-13547 has not been moved to the MODIFIED state.

Details

In response to this:

- What I did

Update library-go and openshift api.
Fixed up the type changes and the access to feature gates to use the new feature gate access defined in library-go.
Note this means feature gate enabled/disabled decisions are now made by CCO and not distributed by revendoring openshift/api everywhere.

Noticed a mistake in the scheme addKnownTypes, see comment I've left.

Details of feature gate changes in openshift/enhancements#1373

- How to verify it

E2E should pass, all units are passing too.

- Description for the changelog

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.

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 Apr 27, 2023

deads2k reviewed Apr 27, 2023

View reviewed changes

JoelSpeed force-pushed the update-ccm-feature-gates branch from 8cd33cc to 0c56e5d Compare April 27, 2023 16:49

JoelSpeed commented Apr 27, 2023

View reviewed changes

JoelSpeed force-pushed the update-ccm-feature-gates branch from 0c56e5d to 8c032e1 Compare April 27, 2023 17:09

JoelSpeed marked this pull request as ready for review April 27, 2023 17:09

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 Apr 27, 2023

openshift-ci bot requested review from jkyros and sinnykumari April 27, 2023 17:11

JoelSpeed changed the title ~~Update Library-go and API for new featuregate changes~~ [OCPCLOUD-2034] Update Library-go and API for new featuregate changes Apr 27, 2023

JoelSpeed force-pushed the update-ccm-feature-gates branch 2 times, most recently from e4ad6a7 to 9efe1b0 Compare May 2, 2023 15:09

JoelSpeed mentioned this pull request May 3, 2023

OCPBUGS-13547: Ensure --payload-version is set for MCO on bootstrap openshift/installer#7160

Merged

JoelSpeed force-pushed the update-ccm-feature-gates branch 2 times, most recently from 8bb958b to 4a8f22c Compare May 5, 2023 16:31

openshift-ci bot requested a review from rphillips May 9, 2023 12:49

JoelSpeed force-pushed the update-ccm-feature-gates branch from 4a8f22c to aa185fb Compare May 15, 2023 07:35

adriengentil mentioned this pull request May 15, 2023

OCPCLOUD-2010: Add CloudControllerManagerStatus to External platform status openshift/api#1434

Merged

JoelSpeed changed the title ~~[OCPCLOUD-2034] Update Library-go and API for new featuregate changes~~ OCPBUGS-13547: [OCPCLOUD-2034] Update Library-go and API for new featuregate changes May 17, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 17, 2023

JoelSpeed mentioned this pull request Jun 5, 2023

Disable EventedPLEG featuregate in techpreview openshift/api#1483

Merged

JoelSpeed mentioned this pull request Jun 5, 2023

Update openshift/api to disable EventedPLEG featuregate in techpreview openshift/cluster-config-operator#317

Merged

JoelSpeed force-pushed the update-ccm-feature-gates branch from cea2567 to 3de0564 Compare June 5, 2023 13:25

sinnykumari approved these changes Jun 5, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2023

openshift-ci bot assigned sinnykumari Jun 5, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2023

jkyros approved these changes Jun 5, 2023

View reviewed changes

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jun 6, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2023

openshift-merge-robot merged commit dadba5b into openshift:master Jun 6, 2023

dgoodwin mentioned this pull request Jun 7, 2023

Revert "OCPBUGS-13547: [OCPCLOUD-2034] Update Library-go and API for new featuregate changes" #3732

Closed

DennisPeriquet mentioned this pull request Jun 8, 2023

Revert "OCPBUGS-13547: [OCPCLOUD-2034] Update Library-go and API for new featuregate changes" #3739

Closed

openshift-ci-robot mentioned this pull request Jul 7, 2023

OCPBUGS-13547: Promote Azure CCM from TPNU to default openshift/api#1461

Merged

rhmdnd mentioned this pull request Aug 10, 2023

OCPBUGS-17216: Update rotate certificates check for OCP 4.14 ComplianceAsCode/content#10973

Merged

OCPBUGS-13547: [OCPCLOUD-2034] Update Library-go and API for new featuregate changes #3688

OCPBUGS-13547: [OCPCLOUD-2034] Update Library-go and API for new featuregate changes #3688

Uh oh!

Conversation

JoelSpeed commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoelSpeed commented Apr 27, 2023

Uh oh!

openshift-ci bot commented Apr 27, 2023

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!

JoelSpeed commented Apr 27, 2023

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!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Apr 27, 2023

Uh oh!

JoelSpeed commented Apr 28, 2023

Uh oh!

JoelSpeed commented May 3, 2023

Uh oh!

JoelSpeed commented May 3, 2023

Uh oh!

sinnykumari commented May 9, 2023

Uh oh!

sinnykumari commented May 17, 2023

Uh oh!

JoelSpeed commented Jun 5, 2023

Uh oh!

JoelSpeed commented Jun 5, 2023

Uh oh!

sinnykumari commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoelSpeed commented Jun 5, 2023

Uh oh!

JoelSpeed commented Jun 5, 2023

Uh oh!

sinnykumari commented Jun 5, 2023

Uh oh!

sinnykumari commented Jun 5, 2023

Uh oh!

jkyros left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jun 5, 2023

Uh oh!

JoelSpeed commented Jun 6, 2023

Uh oh!

sergiordlr commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoelSpeed commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JoelSpeed commented Apr 27, 2023 •

edited

Loading

sinnykumari commented Jun 5, 2023 •

edited

Loading

sergiordlr commented Jun 6, 2023 •

edited

Loading

JoelSpeed commented Jun 6, 2023 •

edited

Loading

openshift-ci bot commented Jun 6, 2023 •

edited

Loading