[WIP] OCPEDGE-649: check if CCMO present by qJkee · Pull Request #3999 · openshift/machine-config-operator

qJkee · 2023-10-24T15:17:58Z

Add check if CCMO is present within the cluster. If not, then do not pass the flag to kubelet related to the cloud providers

Here I try to read the install config from the cluster or from the disc and detect enabled capabilities.
If CloudController capability is disabled(cloud controller manager operator is not installed to the cluster) we return an empty value passed to the --cloud-provider kubelet flag. We pass external only in case if cloud provider is actually external to support manual installation of providers

openshift-ci-robot · 2023-10-24T15:18:04Z

@qJkee: This pull request references OCPVE-628 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.15.0" version, but no target version was set.

Details

In response to this:

Add check if CCMO is present within the cluster. If not, then do not pass the flag to kubelet related to the cloud providers

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 · 2023-10-24T15:21:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qJkee
Once this PR has been reviewed and has the lgtm label, please assign sinnykumari for approval. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

qJkee · 2023-11-23T10:03:48Z

/hold until openshift/api#1579 gets merged

sinnykumari · 2023-11-23T10:27:08Z

Is this work expected to merge in 4.15 or right now it is mainly a POC? Right now linked card is epic, please link it to a story with details so that we can understand what exactly we need to review. Also, as per PR description, it seems like it will require qe testing because we are skipping passing kubelet flags.

openshift-ci-robot · 2023-11-23T11:09:33Z

@qJkee: This pull request references OCPEDGE-649 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

Details

In response to this:

Add check if CCMO is present within the cluster. If not, then do not pass the flag to kubelet related to the cloud providers

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-robot · 2023-11-23T11:12:11Z

@qJkee: This pull request references OCPEDGE-649 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

Details

In response to this:

Add check if CCMO is present within the cluster. If not, then do not pass the flag to kubelet related to the cloud providers

Here I try to read the install config from the cluster or from the disc and detect enabled capabilities.
If CloudController capability is disabled(cloud controller manager operator is not installed to the cluster) we return an empty value passed to the --cloud-provider kubelet flag. We pass external only in case if cloud provider is actually external to support manual installation of providers

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.

cdoern

I don't think we can safely merge a PR with a replaced API. Also some general comments about comments, errors etc.

cdoern · 2023-11-27T20:48:42Z

 	sigs.k8s.io/yaml v1.3.0 // indirect
 )
+
+replace github.com/openshift/api => github.com/qjkee/api v0.0.0-20231024160705-d90a3dcdd9ec


I don't think we can ship with a replace in the go.mod

It's just for using api objects.
Once I get this approved/lgtm I'll merge my branch to the upstream(openshift/api) and then just bump regular openshift/api version.
See openshift/api#1579 (comment)

This is going to conflict with: #4012 which we want to get in first for TRT. So i advise we hold off on this one until the API bump from the linked PR gets in.

sinnykumari · 2023-11-28T09:16:14Z

Thanks all for reviewing the PR! Putting hold for QE pre-merge testing.
/cc @sergiordlr @rioliu-rh
@qJkee Is this functionality already getting tested by a QE? If not, can you help with steps our QE can take to test this functionality?

qJkee · 2023-11-28T11:26:01Z

@sinnykumari
Yeah, QE did pre-merge testing at OCPEDGE-269

cdoern

As long as comments are added and the errors are more fatal I can get behind this. However, I do not think we can merge this because it'll wreak havoc against: #4012 I see what Joel is saying about merging and agreeing on the dependencies first. However, merging this will make it impossible for the above PR to merge since the API code I am using will not include this custom API code.

sinnykumari · 2023-11-29T13:19:00Z

Overall this looks fine, also as per #3999 (comment) , respective QE team is already testing this functionality.

Major missing item is getting openshift/api#1579 merged and adapting this PR to switch back to using openshit/api

cdoern

I think I found what is breaking the unit tests, PTAL

cdoern · 2023-12-04T20:04:18Z

+            version: ""
+          type: ""
+      replicas: 3
+kind: ConfigMap


This is the configmap causing the error in the bootstrap_unit test logs that says bootstrap_test.go:432: Unknown object type *v1.ConfigMap

cdoern · 2023-12-04T20:04:36Z

@@ -0,0 +1,49 @@
+apiVersion: v1


do we need this? it seems to be breaking bootstrap tests

qJkee · 2023-12-20T14:12:56Z

/retest

qJkee · 2024-01-17T08:45:42Z

/test unit

qJkee · 2024-01-17T14:24:53Z

/retest

qJkee · 2024-01-26T06:41:30Z

/retest

qJkee · 2024-01-31T15:02:26Z

/retest

Add check if CCMO is present within the cluster. If not, then do not pass the flag --cloud-provider to the kubelet

qJkee · 2024-02-09T17:09:59Z

/retest

qJkee · 2024-02-12T13:39:11Z

/retest

qJkee · 2024-02-12T18:54:10Z

/retest

openshift-ci · 2024-02-14T19:18:45Z

@qJkee: The following test 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-ovn	`45eee00`	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.

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

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 Oct 24, 2023

openshift-ci Bot requested review from cdoern and djoshy October 24, 2023 15:21

qJkee marked this pull request as draft October 29, 2023 16:46

qJkee marked this pull request as ready for review October 29, 2023 18:27

openshift-ci Bot requested review from jkyros and sinnykumari October 29, 2023 18:28

qJkee marked this pull request as draft November 17, 2023 16:25

qJkee marked this pull request as ready for review November 23, 2023 10:02

qJkee changed the title ~~[WIP] OCPVE-628: POC: check if CCMO present~~ OCPVE-628: POC: check if CCMO present Nov 23, 2023

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 Nov 23, 2023

openshift-ci Bot requested review from cgwalters and cheesesashimi November 23, 2023 10:03

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 23, 2023

qJkee changed the title ~~OCPVE-628: POC: check if CCMO present~~ OCPEDGE-649: check if CCMO present Nov 23, 2023

cdoern suggested changes Nov 27, 2023

View reviewed changes

cheesesashimi reviewed Nov 27, 2023

View reviewed changes

Comment thread pkg/controller/template/render.go Outdated

openshift-ci Bot requested review from rioliu-rh and sergiordlr November 28, 2023 09:16

qJkee requested review from cdoern and cheesesashimi November 28, 2023 15:30

cdoern reviewed Nov 28, 2023

View reviewed changes

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 29, 2023

cdoern reviewed Dec 4, 2023

View reviewed changes

qJkee changed the title ~~OCPEDGE-649: check if CCMO present~~ [WIP] OCPEDGE-649: check if CCMO present Dec 11, 2023

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 Dec 11, 2023

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 11, 2023

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

check if CCMO present

b29696d

Add check if CCMO is present within the cluster. If not, then do not pass the flag --cloud-provider to the kubelet

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

qJkee added 3 commits February 14, 2024 09:21

fix tests

a18b0d5

fix linter warnings

68b7d40

add check for hypershift platform

45eee00

qJkee closed this Feb 14, 2024

qJkee mentioned this pull request Feb 14, 2024

OCPEDGE-649: check if CloudCredentialManager present #4188

Closed

Conversation

qJkee commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 24, 2023 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Oct 24, 2023

Uh oh!

qJkee commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sinnykumari commented Nov 23, 2023

Uh oh!

openshift-ci-robot commented Nov 23, 2023 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 23, 2023 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdoern left a comment

Choose a reason for hiding this comment

Uh oh!

cdoern Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdoern Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

qJkee commented Nov 28, 2023

Uh oh!

cdoern left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sinnykumari commented Nov 29, 2023

Uh oh!

cdoern left a comment

Choose a reason for hiding this comment

Uh oh!

cdoern Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

cdoern Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qJkee commented Dec 20, 2023

Uh oh!

qJkee commented Jan 17, 2024

Uh oh!

qJkee commented Jan 17, 2024

Uh oh!

qJkee commented Jan 26, 2024

Uh oh!

qJkee commented Jan 31, 2024

Uh oh!

qJkee commented Feb 9, 2024

Uh oh!

qJkee commented Feb 12, 2024

Uh oh!

qJkee commented Feb 12, 2024

Uh oh!

openshift-ci Bot commented Feb 14, 2024

Uh oh!

Reviewers

Assignees

Labels

qJkee commented Oct 24, 2023 •

edited

Loading

openshift-ci-robot commented Oct 24, 2023 •

edited by openshift-ci Bot

Loading

qJkee commented Nov 23, 2023 •

edited

Loading

openshift-ci-robot commented Nov 23, 2023 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Nov 23, 2023 •

edited by openshift-ci Bot

Loading

qJkee Nov 27, 2023 •

edited

Loading

sinnykumari commented Nov 28, 2023 •

edited

Loading

cdoern left a comment •

edited

Loading