OCPEDGE-649: check if CloudCredentialManager present by qJkee · Pull Request #4188 · openshift/machine-config-operator

qJkee · 2024-02-14T17:35:44Z

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

This is a new PR in order to keep actual context. The old one is #3999

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

openshift-ci-robot · 2024-02-14T17:35:49Z

@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.16.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

This is a new PR in order to keep actual context. The old one is #3999

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.

qJkee · 2024-02-14T17:36:03Z

/cc @cdoern

cdoern

looks good. I caught a few things looking over this briefly. I will give this another look next week. I agree with your overall choices and defaults in most of these scenarios!

cdoern · 2024-02-16T20:50:07Z

 	github.com/stretchr/testify v1.8.4
 	github.com/vincent-petithory/dataurl v1.0.0
-	golang.org/x/net v0.18.0
+	golang.org/x/net v0.19.0


might have asked this, but any reason for these bumps?

I think this comes from openshift/api

cdoern · 2024-02-16T20:50:45Z


 replace k8s.io/kube-openapi => github.com/openshift/kube-openapi v0.0.0-20230816122517-ffc8f001abb0
+
+replace github.com/openshift/api => github.com/qjkee/api v0.0.0-20240209124943-dfcecbd06e01


again, we would like this merged before we can merge this cc @JoelSpeed

This is just a temp replace. Once this PR is approved I'll merge api PR and bump it there

cdoern · 2024-02-16T20:52:40Z

+	external, err := isTopologyExternal(conf.manifestDir)
+	if err != nil {
+		klog.Infoln("failed to check if topology is external", err)
+	} else if external {


makes sense. just hypershift, right?

Yes, this check is used to detect if we are in hypershift environment

cdoern · 2024-02-16T20:54:50Z

+		cfg.configClientSet = cl
+	}
+
+	cv, err := cfg.configClientSet.ConfigV1().ClusterVersions().Get(context.Background(), "version", metav1.GetOptions{})


can we use a lister here?

cdoern · 2024-02-16T20:55:45Z

+		return false, fmt.Errorf("failed to read cluster config, %w", err)
+	}
+
+	obji, err := runtime.Decode(kscheme.Codecs.UniversalDecoder(corev1.SchemeGroupVersion), f)


interesting. can we not just unmarshal into a configmap and if we fail, error?

this approach used in multiple places, so I decided to with it as well

openshift-ci · 2024-02-16T21:00:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, qJkee

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

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

qJkee · 2024-02-17T05:07:57Z

/hold for QE testing

openshift-ci · 2024-02-27T00:36:20Z

@qJkee: 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-gcp-op	`dbb040e`	link	true	`/test e2e-gcp-op`
ci/prow/okd-scos-e2e-aws-ovn	`dbb040e`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-azure-ovn-upgrade-out-of-change	`dbb040e`	link	false	`/test e2e-azure-ovn-upgrade-out-of-change`

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

I'm not sure this PR is actually even required.

The --cloud-provider flag is currently set to the "" value for all cases where the capability can be disabled, which is the intention of this PR as well.

For platform None, Baremetal, and when the platofrm is External and CloudControllerManager.State is None, the library go cloud provider functions already behave as intended.

Since these configurations are all currently immutable, I don't see a need to add this complexity within the MCO.

JoelSpeed · 2024-02-29T10:40:15Z

+		return disabled, err
+	}
+
+	disabled, err = checkInstallConfigForCCMODisabled(conf)


The cluster version should be the source of truth, why is this required? If the clusterversion checks fail I would not expect a fallback

JoelSpeed · 2024-02-29T10:41:32Z

+
+// checkInstallConfigForCCMODisabled reads install config from the filesystem
+// and checks if CloudController capability disabled
+func checkInstallConfigForCCMODisabled(cfg config) (bool, error) {


Should not check install config, it's mutable, where the APIs themselves are not, why are we including this fallback?

JoelSpeed · 2024-02-29T10:49:22Z

+	if err != nil {
+		klog.Errorln("failed to check if cloud provider external", err)
+	} else if external {
+		if cfg.CloudControllerDisabled && isCCMNone(cfg.Infra.Status.PlatformStatus) {


The latter half of this check is already done in library-go, it's impossible to reach this code on platforms where the cloud controller manager may be disabled.

qJkee added 4 commits February 9, 2024 15:59

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

fix tests

a18b0d5

fix linter warnings

68b7d40

add check for hypershift platform

45eee00

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

openshift-ci Bot requested review from cdoern, djoshy and mtrmac February 14, 2024 17:36

cdoern approved these changes Feb 16, 2024

View reviewed changes

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2024

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

modify check for external platform

dbb040e

JoelSpeed reviewed Feb 29, 2024

View reviewed changes

qJkee closed this Mar 4, 2024


		replace k8s.io/kube-openapi => github.com/openshift/kube-openapi v0.0.0-20230816122517-ffc8f001abb0

		replace github.com/openshift/api => github.com/qjkee/api v0.0.0-20240209124943-dfcecbd06e01

Conversation

qJkee commented Feb 14, 2024

Uh oh!

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

Uh oh!

Uh oh!

qJkee commented Feb 14, 2024

Uh oh!

cdoern left a comment

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!

openshift-ci Bot commented Feb 16, 2024

Uh oh!

qJkee commented Feb 17, 2024

Uh oh!

openshift-ci Bot commented Feb 27, 2024

Uh oh!

JoelSpeed left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

openshift-ci-robot commented Feb 14, 2024 •

edited by openshift-ci Bot

Loading