Skip to content

<carry>: enable CSI migration gates in Attach/Detach controller#601

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
jsafrane:wip-fake-adc-features
Apr 10, 2021
Merged

<carry>: enable CSI migration gates in Attach/Detach controller#601
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
jsafrane:wip-fake-adc-features

Conversation

@jsafrane
Copy link
Copy Markdown

@jsafrane jsafrane commented Mar 3, 2021

Run A/D controller with CSI migration feature flags force-enabled.

There is very minimal <carry> patch that forcefully sets CSI migration gates only in A/D controller. We will need it as <carry> patch until CSI migration of all volume plugins reaches GA (1.23-1.24?)

See openshift/enhancements#549 for details.

The patch works around two issues we have in OCP:

  • CVO downgrades Kubernetes components in wrong order: kube-controller-manager (A/D controller) is downgraded first and then it downgrades kubelets. This is not supported Kubernetes version skew, kubelet cannot be newer than KCM. A/D controller breaks in this case.

  • FeatureGates are applied in random order. Without this PR, we need to enable CSI migration flag in A/D controller first, then kubelets. With this PR, the order does not matter.

With this PR, A/D controller knows about CSI migration and will attach volumes to nodes either "in-tree style" or "CSI style", depending what the node actually wants. In a default 4.8 cluster, it will be in-tree style, as CSI migration is disabled everywhere else, so there should be no visible change in A/D controller processing. It becomes useful as the CSI migration becomes enabled on nodes (either during upgrade to 4.9 or downgrade 4.9->4.8), or when user opts-in for CSI migration as tech preview in 4.8 via FeatureGates CR.

@openshift-ci-robot openshift-ci-robot added backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 3, 2021
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from 956890a to 56f25bb Compare March 3, 2021 15:45
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@jsafrane
Copy link
Copy Markdown
Author

jsafrane commented Mar 3, 2021

/retest

@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from 56f25bb to 647ad25 Compare March 5, 2021 13:23
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

Comment thread pkg/controller/volume/attachdetach/openshift_patch.go Outdated
@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from 647ad25 to 6768b20 Compare March 18, 2021 16:09
@jsafrane jsafrane changed the title WIP: fake CSI migration gates in Attach/Detach controller Fake CSI migration gates in Attach/Detach controller Mar 18, 2021
@openshift-ci-robot 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 Mar 18, 2021
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits are valid:

The following commits could not be validated and must be approved by a top-level approver:

@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from 6768b20 to bf38af0 Compare March 18, 2021 16:13
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits are valid:

The following commits could not be validated and must be approved by a top-level approver:

@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from bf38af0 to 2d79793 Compare March 18, 2021 16:14
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits are valid:

The following commits could not be validated and must be approved by a top-level approver:

Copy link
Copy Markdown

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide more information to the carry commit, pointing to openshift/enhancements#549 and that this is owned by storage team and should be contacted with them around 3-4 releases, so around 4.11/4.12 for a potential drop.

The change lgtm, but I'd like architects to sign off the enhancement first before merging this.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 19, 2021
@soltysh
Copy link
Copy Markdown

soltysh commented Mar 19, 2021

The change lgtm, but I'd like architects to sign off the enhancement first before merging this.

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 19, 2021
@soltysh soltysh self-assigned this Mar 19, 2021
@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from 2d79793 to 637c19b Compare March 19, 2021 14:08
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits are valid:

The following commits could not be validated and must be approved by a top-level approver:

@jsafrane
Copy link
Copy Markdown
Author

@soltysh better comment added + linked the enhancement in the commit message.

@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from 637c19b to 1ccf882 Compare March 22, 2021 14:09
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits are valid:

The following commits could not be validated and must be approved by a top-level approver:

@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from 1ccf882 to 0cda7a4 Compare March 22, 2021 14:09
@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from 99dc6a7 to ed8fae7 Compare March 23, 2021 09:11
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits are valid:

The following commits could not be validated and must be approved by a top-level approver:

@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from ed8fae7 to a55fa8e Compare March 23, 2021 15:30
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits are valid:

The following commits could not be validated and must be approved by a top-level approver:

@jsafrane
Copy link
Copy Markdown
Author

/retest

1 similar comment
@jsafrane
Copy link
Copy Markdown
Author

/retest

@jsafrane jsafrane changed the title Fake CSI migration gates in Attach/Detach controller <carry>: enable CSI migration gates in Attach/Detach controller Mar 26, 2021
…t set of featuregates

The volume plugin manager for openshfit's Attach Detach controller in
kube-controller-manager uses a set of featuregates that are NOT the same as
the the other controllers in KCM and the kubelet.

This means these featuregates (if we kept the old names) would be
inconsistent inside of a single binary. There are now separate featuregates
for the volumepluginmanger when running in the Attach Detach controller to
reflect this distintion.

See openshift/enhancements#549 for details.

Stop <carrying> the patch when CSI migration becomes GA (i.e.
features.CSIMigrationAWS / features.CSIMigrationOpenStack are GA).
@jsafrane jsafrane force-pushed the wip-fake-adc-features branch from a55fa8e to 2d9a8f9 Compare April 7, 2021 12:58
@openshift-ci-robot
Copy link
Copy Markdown

@jsafrane: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@jsafrane
Copy link
Copy Markdown
Author

jsafrane commented Apr 7, 2021

Rebased to 1.21

@bertinatto
Copy link
Copy Markdown
Member

/retest

1 similar comment
@bertinatto
Copy link
Copy Markdown
Member

/retest

Copy link
Copy Markdown
Member

@bertinatto bertinatto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2021
@bertinatto
Copy link
Copy Markdown
Member

/test e2e-aws-serial

=Error: error updating LB Target Group (arn:aws:elasticloadbalancing:us-east-1:460538899914:targetgroup/ci-op-igp5p5rx-12284-ffrf5-sint/54dd658f69441292) tags: error tagging resource (arn:aws:elasticloadbalancing:us-east-1:460538899914:targetgroup/ci-op-igp5p5rx-12284-ffrf5-sint/54dd658f69441292): TargetGroupNotFound: Target groups 'arn:aws:elasticloadbalancing:us-east-1:460538899914:targetgroup/ci-op-igp5p5rx-12284-ffrf5-sint/54dd658f69441292' not found

Copy link
Copy Markdown

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, jsafrane, soltysh

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:

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

@soltysh
Copy link
Copy Markdown

soltysh commented Apr 8, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2021
@soltysh soltysh removed the backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. label Apr 8, 2021
@bertinatto
Copy link
Copy Markdown
Member

/retest

@openshift-bot
Copy link
Copy Markdown

/retest

Please review the full test history for this PR and help us cut down flakes.

@bertinatto
Copy link
Copy Markdown
Member

/test e2e-gcp

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 9, 2021

@jsafrane: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-downgrade a55fa8e47d3fb41e483041276a8525a0387369f6 link /test e2e-aws-downgrade

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.

@jsafrane
Copy link
Copy Markdown
Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants