Skip to content

Conversation

@staebler
Copy link
Contributor

Populate the cloud field in kube cloud.conf stored in the kube-cloud-config ConfigMap in the openshift-config-managed namespace. This field informs in which Azure cloud environment the cluster is installed and consequently which Azure API endpoint should be used when communicating via the Azure SDK.

The value populated comes from the .status.platformStatus.azure.cloudName field of the infrastructure.config.openshift.io resource. If the field is empty or missing, then the default "AZUREPUBLICCLOUD" value is set for the cloud field in the kube cloud config. If the field generated from the infrastructure resource conflicts with the field in the user-provided kube
cloud config, then the controller will error on syncing the infrastructure resource.

See openshift/enhancements#321

https://issues.redhat.com/browse/CORS-1444

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2020
@staebler staebler force-pushed the azure_other_cloud_env branch from fe1b6b9 to 415f494 Compare May 20, 2020 15:27
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2020
@staebler
Copy link
Contributor Author

/hold

This is currently vendoring an openshift/api fork until openshift/api#650 merges.

@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 May 20, 2020
@staebler staebler force-pushed the azure_other_cloud_env branch 2 times, most recently from 346a2aa to 7f5aeff Compare May 20, 2020 19:18
@staebler
Copy link
Contributor Author

346a2aa...7f5aeff

  • Changed to not force the cloud field in the kube cloud config to uppercase.

@staebler staebler force-pushed the azure_other_cloud_env branch from 7f5aeff to 9656e5d Compare June 15, 2020 14:07
@staebler
Copy link
Contributor Author

7f5aeff...9656e5d

/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 Jun 15, 2020
@jhixson74
Copy link
Member

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2020
@abhinavdahiya
Copy link
Contributor

/test e2e-azure

1 similar comment
@staebler
Copy link
Contributor Author

/test e2e-azure

@staebler
Copy link
Contributor Author

/hold

The machine config is not getting generated. The masters are stuck using 99-master-generated-registries.

@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 Jun 16, 2020
@staebler
Copy link
Contributor Author

The machine config is not getting generated. The masters are stuck using 99-master-generated-registries.

The cluster-config-operator is writing out a different cloud.conf, which is causing the machine-config-operator to render a different machineconfig. The machine-config daemon running in bootstrap node is looking for the first hash, before the cluster-config-operator changes.

@staebler staebler force-pushed the azure_other_cloud_env branch from 9656e5d to 1a7e4e0 Compare June 16, 2020 16:43
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2020
@staebler
Copy link
Contributor Author

9656e5d...1a7e4e0

  • Changed the Azure transformer to encode the cloud.conf using the same method that the installer does, so that the cloud.conf is not changed during bootstrap. The transformer had been encoding in alphabetical order and without tabs and newlines. The installer encodes in field order with tabs and newlines.

@staebler staebler force-pushed the azure_other_cloud_env branch from 1a7e4e0 to 7299eac Compare June 16, 2020 16:49
@staebler
Copy link
Contributor Author

staebler commented Jun 16, 2020

1a7e4e0...7299eac

  • Moved last changes to the correct commit.

@staebler staebler force-pushed the azure_other_cloud_env branch from 7299eac to 4dc1e74 Compare June 16, 2020 16:51
@staebler
Copy link
Contributor Author

/test e2e-azure

@staebler
Copy link
Contributor Author

The azure tests look to be failing to create the worker nodes. I assume this is due to the bug where the validation is rejecting changes to the machine when providerSpec.userDataSecret.namespace is empty.

This may have been the underlying cause of the masters not being able to transition to the new state after the cloud.conf was changed. So my recent changes to this PR may not have been necessary. I think it is better that the cluster-config-operator does not change the cloud.conf if it does not have to. But this does require copying type from https://github.com/kubernetes/kubernetes/blob/v1.13.5/pkg/cloudprovider/providers/azure. I had been hesitant to do that since it introduces something else that may become out-of-sync and is not easy to discover. I could change it to use the incoming cloud.conf unmodified when the cloud already matches and fall back to alphabetical ordering when a change is necessary?

@staebler
Copy link
Contributor Author

/test e2e-azure

@staebler
Copy link
Contributor Author

/test e2e-azure

@abhinavdahiya
Copy link
Contributor

/retest

@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2020
@abhinavdahiya
Copy link
Contributor

@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 Jun 22, 2020
k8s.io/api v0.18.2
k8s.io/apimachinery v0.18.2
k8s.io/api v0.18.3
k8s.io/apimachinery v0.18.3
Copy link
Contributor

Choose a reason for hiding this comment

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

why version skew?

@sttts
Copy link
Contributor

sttts commented Jun 23, 2020

Needs rebase.

See version skew nit.

Otherwise, sgtm.

Populate the `cloud` field in kube cloud.conf stored in the kube-cloud-config
ConfigMap in the openshift-config-managed namespace. This field informs in
which Azure cloud environment the cluster is installed and consequently which
Azure API endpoint should be used when communicating via the Azure SDK.

The value populated comes from the `.status.platformStatus.azure.cloudName`
field of the infrastructure.config.openshift.io resource. If the field is
empty or missing, then the default "AzurePublicCloud" value is set for the
`cloud` field in the kube cloud config. If the field generated from the
infrastructure resource conflicts with the field in the user-provided kube
cloud config, then the controller will error on syncing the infrastructure
resource.

See openshift/enhancements#321

https://issues.redhat.com/browse/CORS-1444
@staebler staebler force-pushed the azure_other_cloud_env branch from ec4ebaa to bf88b22 Compare June 23, 2020 13:15
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020
@staebler
Copy link
Contributor Author

Needs rebase.

See version skew nit.

Otherwise, sgtm.

The version of openshift/api has already been bumped in master, so the commit bumping the version in this PR has been removed.

@abhinavdahiya
Copy link
Contributor

/retest
/test e2e-azure
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020
@abhinavdahiya
Copy link
Contributor

/retest

@sttts
Copy link
Contributor

sttts commented Jun 24, 2020

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, jhixson74, staebler, sttts

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2020
@openshift-merge-robot openshift-merge-robot merged commit 3ff40a9 into openshift:master Jun 24, 2020
theobarberbany added a commit to theobarberbany/cluster-cloud-controller-manager-operator that referenced this pull request Dec 22, 2023
This change adds the functionality from the cluster-config-operator
azure CloudConfigTransformer.

The functionality we bring over is:

1. Ensure that the Cloud is set in the cloud.conf
   i. If it is set, verify that it is valid and does not conflict with the
      infrastructure config. If it conflicts, we want to error
  ii. If it is not set, default to public cloud (configv1.AzurePublicCloud)

 2. Verify the cloud name set in the infra config is valid, if it is not
 bail with an informative error

See openshift/cluster-config-operator#133 for
the PR that initially added this functionality.
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cluster-cloud-controller-manager-operator that referenced this pull request Jan 5, 2024
This change adds the functionality from the cluster-config-operator
azure CloudConfigTransformer.

The functionality we bring over is:

1. Ensure that the Cloud is set in the cloud.conf
   i. If it is set, verify that it is valid and does not conflict with the
      infrastructure config. If it conflicts, we want to error
  ii. If it is not set, default to public cloud (configv1.AzurePublicCloud)

 2. Verify the cloud name set in the infra config is valid, if it is not
 bail with an informative error

See openshift/cluster-config-operator#133 for
the PR that initially added this functionality.
theobarberbany added a commit to theobarberbany/cluster-cloud-controller-manager-operator that referenced this pull request Jan 5, 2024
This change adds the functionality from the cluster-config-operator
azure CloudConfigTransformer.

The functionality we bring over is:

1. Ensure that the Cloud is set in the cloud.conf
   i. If it is set, verify that it is valid and does not conflict with the
      infrastructure config. If it conflicts, we want to error
  ii. If it is not set, default to public cloud (configv1.AzurePublicCloud)

 2. Verify the cloud name set in the infra config is valid, if it is not
 bail with an informative error

See openshift/cluster-config-operator#133 for
the PR that initially added this functionality.
theobarberbany added a commit to theobarberbany/cluster-cloud-controller-manager-operator that referenced this pull request Jan 5, 2024
This change adds the functionality from the cluster-config-operator
azure CloudConfigTransformer.

The functionality we bring over is:

1. Ensure that the Cloud is set in the cloud.conf
   i. If it is set, verify that it is valid and does not conflict with the
      infrastructure config. If it conflicts, we want to error
  ii. If it is not set, default to public cloud (configv1.AzurePublicCloud)

 2. Verify the cloud name set in the infra config is valid, if it is not
 bail with an informative error

See openshift/cluster-config-operator#133 for
the PR that initially added this functionality.
theobarberbany added a commit to theobarberbany/cluster-cloud-controller-manager-operator that referenced this pull request Jan 10, 2024
This change adds the functionality from the cluster-config-operator
azure CloudConfigTransformer.

The functionality we bring over is:

1. Ensure that the Cloud is set in the cloud.conf
   i. If it is set, verify that it is valid and does not conflict with the
      infrastructure config. If it conflicts, we want to error
  ii. If it is not set, default to public cloud (configv1.AzurePublicCloud)

 2. Verify the cloud name set in the infra config is valid, if it is not
 bail with an informative error

See openshift/cluster-config-operator#133 for
the PR that initially added this functionality.
theobarberbany added a commit to theobarberbany/cluster-cloud-controller-manager-operator that referenced this pull request Jan 10, 2024
This adds a CloudConfigTransformer for Azure that will set the VMType to
'standard' if unset.

See openshift#291, OCPBUGS-25483 and OCPBUGS-20213 for more information.

This change also adds the functionality from the cluster-config-operator
azure CloudConfigTransformer.

The functionality we bring over is:

1. Ensure that the Cloud is set in the cloud.conf
   i. If it is set, verify that it is valid and does not conflict with the
      infrastructure config. If it conflicts, we want to error
  ii. If it is not set, default to public cloud (configv1.AzurePublicCloud)

 2. Verify the cloud name set in the infra config is valid, if it is not
 bail with an informative error

See openshift/cluster-config-operator#133 for
the PR that initially added this functionality.
theobarberbany added a commit to theobarberbany/cluster-cloud-controller-manager-operator that referenced this pull request Jan 10, 2024
This adds a CloudConfigTransformer for Azure that will set the VMType to
'standard' if unset.

See openshift#291, OCPBUGS-25483 and OCPBUGS-20213 for more information.

This change also adds the functionality from the cluster-config-operator
azure CloudConfigTransformer.

The functionality we bring over is:

1. Ensure that the Cloud is set in the cloud.conf
   i. If it is set, verify that it is valid and does not conflict with the
      infrastructure config. If it conflicts, we want to error
  ii. If it is not set, default to public cloud (configv1.AzurePublicCloud)

 2. Verify the cloud name set in the infra config is valid, if it is not
 bail with an informative error

See openshift/cluster-config-operator#133 for
the PR that initially added this functionality.
theobarberbany added a commit to theobarberbany/cluster-cloud-controller-manager-operator that referenced this pull request Jan 10, 2024
This adds a CloudConfigTransformer for Azure that will set the VMType to
'standard' if unset.

See openshift#291, OCPBUGS-25483 and OCPBUGS-20213 for more information.

This change also adds the functionality from the cluster-config-operator
azure CloudConfigTransformer.

The functionality we bring over is:

1. Ensure that the Cloud is set in the cloud.conf
   i. If it is set, verify that it is valid and does not conflict with the
      infrastructure config. If it conflicts, we want to error
  ii. If it is not set, default to public cloud (configv1.AzurePublicCloud)

 2. Verify the cloud name set in the infra config is valid, if it is not
 bail with an informative error

See openshift/cluster-config-operator#133 for
the PR that initially added this functionality.
theobarberbany added a commit to theobarberbany/cluster-cloud-controller-manager-operator that referenced this pull request Jan 10, 2024
This adds a CloudConfigTransformer for Azure that will set the VMType to
'standard' if unset.

See openshift#291, OCPBUGS-25483 and OCPBUGS-20213 for more information.

This change also adds the functionality from the cluster-config-operator
azure CloudConfigTransformer.

The functionality we bring over is:

1. Ensure that the Cloud is set in the cloud.conf
   i. If it is set, verify that it is valid and does not conflict with the
      infrastructure config. If it conflicts, we want to error
  ii. If it is not set, default to public cloud (configv1.AzurePublicCloud)

 2. Verify the cloud name set in the infra config is valid, if it is not
 bail with an informative error

See openshift/cluster-config-operator#133 for
the PR that initially added this functionality.
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.

6 participants