Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCPBUGS-33931, OCPCLOUD-2513: Remove cloud-provider, cloud-config, and cloud-volume-plugin flags #806

Merged
merged 3 commits into from
May 17, 2024

Conversation

soltysh
Copy link
Member

@soltysh soltysh commented May 15, 2024

Replaces #799

/assign @JoelSpeed @nrb @ingvagabund

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 15, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented May 15, 2024

@soltysh: This pull request references OCPCLOUD-2513 which is a valid jira issue.

In response to this:

Replaces #799

/assign @JoelSpeed @nrb @ingvagabund

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 openshift-eng/jira-lifecycle-plugin repository.

@soltysh
Copy link
Member Author

soltysh commented May 15, 2024

/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label May 15, 2024
@openshift-ci openshift-ci bot requested review from atiratree and mfojtik May 15, 2024 11:41
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2024
@JoelSpeed
Copy link
Contributor

With the cloud volume plugin completely removed, will this remove the value on upgrades?

Copy link
Contributor

openshift-ci bot commented May 15, 2024

@soltysh: 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/images b20a157 link true /test images
ci/prow/e2e-aws-operator b20a157 link true /test e2e-aws-operator
ci/prow/e2e-aws-ovn b20a157 link true /test e2e-aws-ovn
ci/prow/e2e-aws-operator-preferred-host b20a157 link true /test e2e-aws-operator-preferred-host
ci/prow/e2e-aws-ovn-upgrade b20a157 link true /test e2e-aws-ovn-upgrade

Full PR test history. Your PR dashboard.

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.

@JoelSpeed
Copy link
Contributor

/test ?

Copy link
Contributor

openshift-ci bot commented May 15, 2024

@JoelSpeed: The following commands are available to trigger required jobs:

  • /test e2e-aws-operator
  • /test e2e-aws-operator-preferred-host
  • /test e2e-aws-ovn
  • /test e2e-aws-ovn-upgrade
  • /test images
  • /test unit
  • /test verify
  • /test verify-deps

The following commands are available to trigger optional jobs:

  • /test okd-scos-images
  • /test security

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openshift-cluster-kube-controller-manager-operator-master-e2e-aws-operator
  • pull-ci-openshift-cluster-kube-controller-manager-operator-master-e2e-aws-operator-preferred-host
  • pull-ci-openshift-cluster-kube-controller-manager-operator-master-e2e-aws-ovn
  • pull-ci-openshift-cluster-kube-controller-manager-operator-master-e2e-aws-ovn-upgrade
  • pull-ci-openshift-cluster-kube-controller-manager-operator-master-images
  • pull-ci-openshift-cluster-kube-controller-manager-operator-master-security
  • pull-ci-openshift-cluster-kube-controller-manager-operator-master-unit
  • pull-ci-openshift-cluster-kube-controller-manager-operator-master-verify
  • pull-ci-openshift-cluster-kube-controller-manager-operator-master-verify-deps

In response to this:

/test ?

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.

@JoelSpeed
Copy link
Contributor

/payload-job periodic-ci-openshift-release-master-ci-4.16-e2e-azure-sdn-upgrade

Copy link
Contributor

openshift-ci bot commented May 15, 2024

@JoelSpeed: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-master-ci-4.16-e2e-azure-sdn-upgrade

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a496ffe0-12b3-11ef-8f3e-0db820aded8e-0

@soltysh
Copy link
Member Author

soltysh commented May 15, 2024

/hold
for that payload results

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2024
@ingvagabund
Copy link
Member

The changes look good from the KCM-o point of view. Leaving the cloud part check for @JoelSpeed.
LGTM

@JoelSpeed
Copy link
Contributor

With the cloud volume plugin completely removed, will this remove the value on upgrades?

No, looking at the gather from the azure upgrade the cloud volume plugin config is left over.

We need to keep the observer and clear the value or include a removal step for this key in the main cloud observer

@JoelSpeed
Copy link
Contributor

Have created a commit that should resolve the issue, JoelSpeed@6481e27

@soltysh
Copy link
Member Author

soltysh commented May 16, 2024

/payload-job periodic-ci-openshift-release-master-ci-4.16-e2e-azure-sdn-upgrade

Copy link
Contributor

openshift-ci bot commented May 16, 2024

@soltysh: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-master-ci-4.16-e2e-azure-sdn-upgrade

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d4ddeac0-1366-11ef-8a72-b1fadc630491-0

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

I believe the issue here is https://github.com/openshift/cluster-kube-controller-manager-operator/pull/806/files#diff-1e1c9f596fa20c6b4f54622a098756ee0d2f7203d19139a58a250c7a7bcb2af4R50-R55

The cloud config observer is clearing the cloud-config configmap, and the KCM pods are complaining that they can't open it.
The KCM pods should get updated to no longer rely on the configmap, but I'm guessing there's some sort of ordering issue here that means that the configmap is being removed, which then breaks the existing KCMs, and prevents the observed config from being updated.

I think the next thing to try is to revert the removal of the configmap and see if the upgrade can continue

@JoelSpeed
Copy link
Contributor

2024-05-16T12:10:16.858059702Z I0516 12:10:16.857953 1 event.go:364] Event(v1.ObjectReference{Kind:"Deployment", Namespace:"openshift-kube-controller-manager-operator", Name:"kube-controller-manager-operator", UID:"c70bddce-6b21-479c-8ad5-612f1e7fd570", APIVersion:"apps/v1", ResourceVersion:"", FieldPath:""}): type: 'Normal' reason: 'ObserveCloudVolumePlugin' observed change in config

These events are repeating throughout the logs. It appears to be trying to remove the config over and over, without success and never settles. So likely also an issue with the config observation code itself

@dinhxuanvu
Copy link
Member

Your reference on previous comment is a bit confusing on the configmap removal part. I think what you meant is this code change https://github.com/openshift/cluster-kube-controller-manager-operator/pull/806/files#diff-1e1c9f596fa20c6b4f54622a098756ee0d2f7203d19139a58a250c7a7bcb2af4 coming from this PR specifically openshift/library-go@b8bcc87.

@dinhxuanvu
Copy link
Member

I opened this PR to test out Joel's theory on reverting the cloud-config configmap deletion.

soltysh and others added 3 commits May 17, 2024 15:20
The following flags were removed as they are depecrated and will
eventually create an error when used.

* --cloud-config
* --cloud-provider
* --cloud-volume-plugin
* --configure-cloud-routes

Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
@soltysh
Copy link
Member Author

soltysh commented May 17, 2024

/payload-job periodic-ci-openshift-release-master-ci-4.16-e2e-azure-sdn-upgrade

Copy link
Contributor

openshift-ci bot commented May 17, 2024

@soltysh: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-master-ci-4.16-e2e-azure-sdn-upgrade

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/58b3c3d0-1450-11ef-84b1-3fe441cb6228-0

@soltysh
Copy link
Member Author

soltysh commented May 17, 2024

/payload-job periodic-ci-openshift-release-master-ci-4.16-e2e-azure-sdn-upgrade

Copy link
Contributor

openshift-ci bot commented May 17, 2024

@soltysh: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-master-ci-4.16-e2e-azure-sdn-upgrade

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/727d3cb0-1464-11ef-9e43-1087e71d9b89-0

@soltysh
Copy link
Member Author

soltysh commented May 17, 2024

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 17, 2024
@dinhxuanvu
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2024
Copy link
Contributor

openshift-ci bot commented May 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

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

@openshift-merge-bot openshift-merge-bot bot merged commit 0bc5f82 into openshift:master May 17, 2024
10 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-kube-controller-manager-operator-container-v4.17.0-202405172342.p0.g0bc5f82.assembly.stream.el9 for distgit ose-cluster-kube-controller-manager-operator.
All builds following this will include this PR.

@soltysh soltysh deleted the bump_deps branch May 18, 2024 06:39
@JoelSpeed
Copy link
Contributor

/lgtm

@JoelSpeed JoelSpeed changed the title OCPCLOUD-2513: Remove cloud-provider, cloud-config, and cloud-volume-plugin flags OCPBUGS-33931, OCPCLOUD-2513: Remove cloud-provider, cloud-config, and cloud-volume-plugin flags May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants