[OCPCLOUD-1107] External cloud-provider support via FeatureGate in post-install by Danil-Grigorev · Pull Request #2386 · openshift/machine-config-operator

Danil-Grigorev · 2021-02-02T21:23:34Z

- What I did

Implementing external cloud provider selection forced by feature gate. Described in the proposal: https://github.com/openshift/enhancements/pull/463/files#diff-3e0e2c48e70215076dfe36c13768a823ab7080d929d80292f37db2ef5a2121e8R201

This PR is main blocker on other work with integrating CCM in Openshift: https://issues.redhat.com/browse/OCPCLOUD-1107

- How to verify it

- Description for the changelog

kikisdeliveryservice

feel like the team/arch also needs to review the enhancement

/hold

kikisdeliveryservice · 2021-02-02T22:27:00Z

Feels like this should leverage: #2352 via: https://github.com/openshift/api/blob/master/config/v1/types_feature.go#L121

JoelSpeed · 2021-02-03T11:59:46Z

Does this not need to be in the switch so that we can enable the external provider platform by platform?

Eventually, yes. In TechPreviewNoUpgrade it is on feature gate to set invariant configuration when found. I don't think anyone will risk setting such gate on production cluster for platform we don't have configs for. This just means less changes in development cycle, if we don't make it a switch.

I had thought we would want this to only affect things on platforms we have allowed it to, hence being provider specific, let's discuss on the call next week

kikisdeliveryservice

@Danil-Grigorev I've gone through the enhancements, but am having problems connecting the dots here.

Could you add some detail/specific description as to what this PR is trying to accomplish, whether this PR is like step 1 of X etc... if this is day2 post-install is this user initiated? etc? Will there be any followup prs or are there prereq prs for this? etc

Thanks! :)

Danil-Grigorev · 2021-02-09T11:27:46Z

@kikisdeliveryservice Sure. The prerequisite PR will be an API change for FeatureGate in openshift/api#738 The name of the featureGate, it's location and related changes in functionality may be corrected based on new data from kubernetes/enhancements#2443. What it will result into - we will have a way to deploy a techPreview clusters with out-of-tree providers code, and attempt migration from in-tree in post install time, while identifying issues (CI use-case). It is user initiated.

Ideal scenario - follow up PRs will only remove selections from platform switch case, say: AWS is now by default external in 1.24. Realistically, we will still have to force some kubelet flags even after the upstream feature gate is implemented, as the goal is to identify issues in migration, which may break some storage, networking and cloud-credentials functionality in the cluster if done blindly.

Actual change, which we will probably need - implement our selection (or more realistically wait for some helper upstream implementation) in MCO install phase, where the bootstrap methods are called. This way a fresh cluster will get out-of-tree providers by default with related DisableCloudProviders feature gate . This is out-of-scope for this PR.

kikisdeliveryservice · 2021-02-11T16:42:32Z

Thanks for the summary @Danil-Grigorev !

Danil-Grigorev · 2021-02-17T10:32:08Z

/retest

- Add external-cloud-provider feature gate into render - Add exclusion list to featureGate selection: a list of openshift specific featureGates, which will not be passed to default kubelet config

Danil-Grigorev · 2021-06-17T15:33:16Z

@JoelSpeed @kikisdeliveryservice All comments addressed. I checked, permission change happens on revendor, and it seems expected. Please review.

JoelSpeed · 2021-06-17T16:01:37Z

checked, permission change happens on revendor, and it seems expected. Please review.

Perhaps since this seems unrelated to the contents of this PR, should we just drop these two files from the commit? Would make the history cleaner and then the vendor discrepancy can be sorted at a later point?

JoelSpeed

/lgtm

Thanks @Danil-Grigorev

elmiko · 2021-06-21T20:17:20Z

just wanted to note that i have been test driving this patch for the last few days and it seems to be working for me. i have not given a thorough testing of the negative case (eg when the gate is not applied), but i have done a basic smoke test on that.

kikisdeliveryservice · 2021-06-22T18:38:44Z

/retest

elmiko · 2021-06-22T19:24:01Z

@Danil-Grigorev , i talked with @kikisdeliveryservice in slack and it sounds like we could help here by adding a must-gather for a cluster that has had the feature gate enabled as a day 2 operation. so, basically:

create normal cluster with in-tree providers
enable feature gate and switch cluster to out-of-tree
once cluster is healthy, collect must-gather

i think we are already capturing this behavior in the openshift-e2e-aws-ccm step. but since we need this PR to make it work, none of those jobs have been passing. @Danil-Grigorev does that make sense?

kikisdeliveryservice · 2021-06-22T19:40:29Z

just to follow-on the above, would this ever be disabled day2 @Danil-Grigorev ?

Danil-Grigorev · 2021-06-22T19:44:50Z

@kikisdeliveryservice No, applying the feature gate would move cluster to TechPreview state, where we officially do not support rollback. Yet the code does not prevent it from happening and works both ways.

openshift-ci · 2021-06-22T21:20:15Z

@Danil-Grigorev: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/okd-e2e-aws	`39e07de`	link	`/test okd-e2e-aws`
ci/prow/e2e-aws-disruptive	`39e07de`	link	`/test e2e-aws-disruptive`

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.

elmiko · 2021-06-23T15:01:22Z

quick update here, we have shared a must-gather with @kikisdeliveryservice

kikisdeliveryservice

Reviewed must gather and verified:

we started off with --cloud-provider=aws
and
successfully switched to --cloud-provider=external

Based on my understanding from: https://github.com/openshift/enhancements/pull/463/files#diff-3e0e2c48e70215076dfe36c13768a823ab7080d929d80292f37db2ef5a2121e8R270 also saw: ExternalCloudProvider: true alongside --cloud-provider=aws which helps force upgrade from intree to out of tree.

Since this has also been extensively tested by that team, let's merge this to unblock work and move forward.

openshift-ci · 2021-06-23T18:41:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Danil-Grigorev, JoelSpeed, kikisdeliveryservice, rphillips

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~~ [kikisdeliveryservice]

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

kikisdeliveryservice · 2021-06-23T18:42:34Z

/hold cancel

sinnykumari · 2021-06-24T07:39:36Z

/test e2e-agnostic-upgrade

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2021

openshift-ci-robot requested review from ashcrow and kikisdeliveryservice February 2, 2021 21:23

ashcrow requested review from yuqi-zhang and removed request for ashcrow February 2, 2021 21:26

Danil-Grigorev force-pushed the external-provider-support branch from 20321ce to ac4d0dd Compare February 2, 2021 21:28

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2021

Danil-Grigorev mentioned this pull request Feb 2, 2021

[WIP] Rearrange template for the external cloud-provider #2093

Closed

kikisdeliveryservice requested a review from crawford February 2, 2021 21:38

kikisdeliveryservice changed the title ~~External cloud-provider support via FeatureGate in post-install~~ [WIP] External cloud-provider support via FeatureGate in post-install Feb 2, 2021

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2021

Danil-Grigorev force-pushed the external-provider-support branch from ac4d0dd to f05c4d0 Compare February 2, 2021 21:41

kikisdeliveryservice reviewed Feb 2, 2021

View reviewed changes

Comment thread pkg/controller/kubelet-config/kubelet_config_controller.go Outdated

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2021

kikisdeliveryservice requested a review from rphillips February 2, 2021 22:10

Danil-Grigorev force-pushed the external-provider-support branch from f05c4d0 to 62fc109 Compare February 3, 2021 14:47

JoelSpeed reviewed Feb 4, 2021

View reviewed changes

Danil-Grigorev force-pushed the external-provider-support branch from e4be2f8 to 655727b Compare February 4, 2021 20:55

kikisdeliveryservice reviewed Feb 5, 2021

View reviewed changes

kikisdeliveryservice mentioned this pull request Feb 8, 2021

WIP: Ignore CSIMigrationControlPlane FeatureSet #2371

Closed

Danil-Grigorev force-pushed the external-provider-support branch from b042fdd to d36cbbb Compare February 11, 2021 13:27

Danil-Grigorev changed the title ~~[WIP] External cloud-provider support via FeatureGate in post-install~~ External cloud-provider support via FeatureGate in post-install Feb 11, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2021

Danil-Grigorev mentioned this pull request Feb 11, 2021

Initial AWS CCM deployment using upstream image openshift/cluster-cloud-controller-manager-operator#15

Merged

4 tasks

Danil-Grigorev force-pushed the external-provider-support branch 2 times, most recently from c81a10b to 1631be6 Compare February 16, 2021 19:05

Danil-Grigorev force-pushed the external-provider-support branch from 8fd0155 to f96cd2f Compare June 17, 2021 15:04

Include feature informer into template controller

a4bf0b1

- Add external-cloud-provider feature gate into render - Add exclusion list to featureGate selection: a list of openshift specific featureGates, which will not be passed to default kubelet config

Danil-Grigorev force-pushed the external-provider-support branch from f96cd2f to 4956c68 Compare June 17, 2021 15:26

Danil-Grigorev requested review from JoelSpeed, kikisdeliveryservice and sinnykumari June 17, 2021 15:28

Revendor library-go with ExternalCloudProvider FG filtering logic

39e07de

Danil-Grigorev force-pushed the external-provider-support branch from 4956c68 to 39e07de Compare June 17, 2021 19:09

Danil-Grigorev mentioned this pull request Jun 17, 2021

Remove image overrides from aws-ccm job and make job required openshift/release#19400

Merged

JoelSpeed reviewed Jun 18, 2021

View reviewed changes

openshift-ci Bot assigned JoelSpeed Jun 18, 2021

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2021

elmiko mentioned this pull request Jun 21, 2021

[OCPCLOUD-1106] Add afterburn task to update AWS hostname to match instance metadata #2401

Merged

kikisdeliveryservice approved these changes Jun 23, 2021

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 23, 2021

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 23, 2021

openshift-merge-robot merged commit 7de6ba3 into openshift:master Jun 24, 2021

Danil-Grigorev deleted the external-provider-support branch June 24, 2021 12:14

Conversation

Danil-Grigorev commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kikisdeliveryservice commented Feb 2, 2021

Uh oh!

JoelSpeed Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev commented Feb 9, 2021

Uh oh!

kikisdeliveryservice commented Feb 11, 2021

Uh oh!

Danil-Grigorev commented Feb 17, 2021

Uh oh!

Danil-Grigorev commented Jun 17, 2021

Uh oh!

JoelSpeed commented Jun 17, 2021

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

elmiko commented Jun 21, 2021

Uh oh!

kikisdeliveryservice commented Jun 22, 2021

Uh oh!

elmiko commented Jun 22, 2021

Uh oh!

kikisdeliveryservice commented Jun 22, 2021

Uh oh!

Danil-Grigorev commented Jun 22, 2021

Uh oh!

openshift-ci Bot commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elmiko commented Jun 23, 2021

Uh oh!

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Jun 23, 2021

Uh oh!

kikisdeliveryservice commented Jun 23, 2021

Uh oh!

sinnykumari commented Jun 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Danil-Grigorev commented Feb 2, 2021 •

edited

Loading

openshift-ci Bot commented Jun 22, 2021 •

edited

Loading