MGMT-15690: Add support for external platform #5738

adriengentil · 2023-11-27T11:32:49Z

Add a new platform type external and new struct External under Platform in order to configure it
Add validations to the External struct and ability to create/update it:
- platformName: is required and must not be empty
- cloudControllerManager: set to an empty string (CCM disabled) if not set explicitly
Add feature support, OCP 4.14 and user managed networking is required to install a cluster with external platform
This new platform is behind an OCM capability, disabled by default

I will plug external platform/platformName=oci with the existing oci integration in a follow-up PR.

openshift-ci · 2023-11-27T11:32:58Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

adriengentil · 2023-11-27T11:33:25Z

/test unit-test edge-e2e-metal-assisted edge-e2e-oci-assisted

openshift-ci · 2023-11-27T11:33:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriengentil

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:

~~OWNERS~~ [adriengentil]

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

adriengentil · 2023-11-27T11:58:02Z

/test edge-unit-test

codecov · 2023-11-27T12:11:49Z

Codecov Report

Merging #5738 (8317fec) into master (25d0ae7) will increase coverage by 1.37%.
Report is 6 commits behind head on master.
The diff coverage is 53.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5738      +/-   ##
==========================================
+ Coverage   68.00%   69.38%   +1.37%     
==========================================
  Files         233      234       +1     
  Lines       34324    36233    +1909     
==========================================
+ Hits        23342    25140    +1798     
- Misses       8915     8968      +53     
- Partials     2067     2125      +58

Files	Coverage Δ
internal/bminventory/inventory.go	`77.04% <100.00%> (+6.75%)`	⬆️
internal/featuresupport/feature_support_level.go	`89.28% <ø> (ø)`
internal/featuresupport/features_networking.go	`83.81% <100.00%> (+0.48%)`	⬆️
internal/provider/external/base.go	`0.00% <ø> (ø)`
internal/provider/registry/registry.go	`64.78% <100.00%> (+0.50%)`	⬆️
internal/common/conversions.go	`5.40% <0.00%> (ø)`
internal/provider/external/inventory.go	`0.00% <0.00%> (ø)`
internal/cluster/validations/validations.go	`26.35% <0.00%> (+0.48%)`	⬆️
internal/featuresupport/features_platforms.go	`91.25% <86.36%> (-0.78%)`	⬇️
internal/common/common.go	`24.79% <0.00%> (-1.07%)`	⬇️
... and 2 more

... and 8 files with indirect coverage changes

openshift-ci-robot · 2023-11-27T14:04:41Z

@adriengentil: This pull request references MGMT-15690 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 task to target the "4.15.0" version, but no target version was set.

In response to this:

define external platform type in swagger

generate code

create external provider

Add capability check for external platform

add platform name and CCM configuration

set external fields can be nullable

register external cluster

update cluster

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-27T14:14:45Z

@adriengentil: This pull request references MGMT-15690 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 task to target the "4.15.0" version, but no target version was set.

In response to this:

Add a new platform type external and new struct External under Platform in order to configure it

Add validations to the External struct:

platformName: is required and must not be empty

cloudControllerManger: set to an empty string (CCM disabled) if not set explicitly

Add feature support, OCP 4.14 and user managed networking is required to install a cluster with external platform

This new platform is behind an OCM capability, disabled by default

I will plug external platform/platformName=oci with the existing oci integration in a follow-up PR.

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-27T14:15:26Z

@adriengentil: This pull request references MGMT-15690 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 task to target the "4.15.0" version, but no target version was set.

In response to this:

Add a new platform type external and new struct External under Platform in order to configure it

Add validations to the External struct and ability to create/update it:

platformName: is required and must not be empty

cloudControllerManger: set to an empty string (CCM disabled) if not set explicitly

Add feature support, OCP 4.14 and user managed networking is required to install a cluster with external platform

This new platform is behind an OCM capability, disabled by default

I will plug external platform/platformName=oci with the existing oci integration in a follow-up PR.

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.

adriengentil · 2023-11-27T14:28:00Z

/cc @pawanpinjarkar

adriengentil · 2023-11-27T15:49:17Z

Created a SNO cluster with:

aicli -U http://10.1.155.26:8080/ create cluster ext-sno -Ppull_secret=~/pull-secret -Popenshift_version=4.14 -Puser_managed_networking=True -Pplatform='{"type":"external", "external": {"platform_name": "agdcorp" , "cloud_controller_manager": ""}}' -Psno=True

Install config is updated accordingly:

...
  "platform": {
    "external": {
      "PlatformName": "agdcorp",
      "CloudControllerManager": ""
    }
  },
...

And the infrastructure object of the cluster using the external integration without CCM is created as expected:

apiVersion: config.openshift.io/v1
kind: Infrastructure
metadata:
  creationTimestamp: "2023-11-27T15:39:13Z"
  generation: 1
  name: cluster
  resourceVersion: "527"
  uid: 391f1b27-77ea-4696-a299-e99e71a694f2
spec:
  cloudConfig:
    name: ""
  platformSpec:
    external:
      platformName: agdcorp
    type: External
status:
  apiServerInternalURI: https://api-int.ext-sno.karmalabs.corp:6443
  apiServerURL: https://api.ext-sno.karmalabs.corp:6443
  controlPlaneTopology: SingleReplica
  cpuPartitioning: None
  etcdDiscoveryDomain: ""
  infrastructureName: ext-sno-gprfr
  infrastructureTopology: SingleReplica
  platform: External
  platformStatus:
    external:
      cloudControllerManager:
        state: None
    type: External

adriengentil · 2023-11-27T16:00:36Z

/cc @avishayt @gamli75

adriengentil · 2023-11-27T16:06:39Z

/test edge-e2e-oci-assisted

adriengentil · 2023-11-27T17:08:37Z

/retest
looks like flakes in prow

adriengentil · 2023-11-28T08:09:57Z

/test edge-subsystem-aws edge-subsystem-kubeapi-aws

internal/bminventory/inventory.go

internal/provider/common.go

gamli75 · 2023-11-28T09:44:05Z

internal/bminventory/inventory.go

@@ -2543,6 +2543,17 @@ func (b *bareMetalInventory) updatePlatformParams(params installer.V2UpdateClust
 	if params.ClusterUpdateParams.Platform != nil && common.PlatformTypeValue(params.ClusterUpdateParams.Platform.Type) != "" {
 		updates["platform_type"] = params.ClusterUpdateParams.Platform.Type
 		updates["platform_is_external"] = swag.BoolValue(params.ClusterUpdateParams.Platform.IsExternal)
+		if *params.ClusterUpdateParams.Platform.Type == models.PlatformTypeExternal {


sometime we use common.PlatformTypeValue(params.ClusterUpdateParams.Platform.Type) and sometimes we use *params.ClusterUpdateParams.Platform.Type - why?

common.PlatformTypeValue() returns an empty string when Platform.Type is nil. At this point of the code we know the type is not nil, so it's safe to look at the content the variable.

gamli75 · 2023-11-28T09:46:17Z

internal/bminventory/inventory.go

@@ -2543,6 +2543,17 @@ func (b *bareMetalInventory) updatePlatformParams(params installer.V2UpdateClust
 	if params.ClusterUpdateParams.Platform != nil && common.PlatformTypeValue(params.ClusterUpdateParams.Platform.Type) != "" {
 		updates["platform_type"] = params.ClusterUpdateParams.Platform.Type
 		updates["platform_is_external"] = swag.BoolValue(params.ClusterUpdateParams.Platform.IsExternal)
+		if *params.ClusterUpdateParams.Platform.Type == models.PlatformTypeExternal {
+			if params.ClusterUpdateParams.Platform.External != nil && params.ClusterUpdateParams.Platform.External.PlatformName != nil {
+				updates["platform_external_platform_name"] = params.ClusterUpdateParams.Platform.External.PlatformName


do we have consts for platform_external_platform_name, platform_external_cloud_controller_manager?

no, those are columns names in DB, looks like we never defined any consts for them in the past 🤔

gamli75 · 2023-11-28T09:48:32Z

swagger.yaml

@@ -6221,6 +6225,28 @@ definitions:
      - vsphere
      - none
      - oci
+      - external


I thought that we are going to remove oci, @avishayt ?

It will come in a follow-up PR, I need to plug the oci code to the external/platformName=oci, then I will be able to remove the oci platform type

avishayt · 2023-11-28T11:13:25Z

swagger.yaml

@@ -5413,6 +5414,9 @@ definitions:
    properties:
      type:
        $ref: '#/definitions/platform_type'
+      external:
+        $ref: '#/definitions/platform_external'
+        x-nullable: true
      is_external:


We should have another PR to remove is_external, right?

yes, will remove oci platform type and is_external at the same time

internal/provider/common.go

openshift-ci-robot · 2023-11-28T15:28:13Z

@adriengentil: This pull request references MGMT-15690 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 task to target the "4.15.0" version, but no target version was set.

In response to this:

Add a new platform type external and new struct External under Platform in order to configure it

Add validations to the External struct and ability to create/update it:

platformName: is required and must not be empty

cloudControllerManager: set to an empty string (CCM disabled) if not set explicitly

Add feature support, OCP 4.14 and user managed networking is required to install a cluster with external platform

This new platform is behind an OCM capability, disabled by default

I will plug external platform/platformName=oci with the existing oci integration in a follow-up PR.

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.

pawanpinjarkar · 2023-11-28T22:34:49Z

@adriengentil I successfully tested SNO IPV4 cluster installation for the external platform with the below config in the install-config via dev-scripts.

"platform": {
        "external": {
           "platformName": "nonoci"
            }
    },

Ref installer PR openshift/installer#7585

adriengentil · 2023-11-29T08:24:18Z

/retest
looks like flakes

adriengentil · 2023-11-29T10:14:09Z

/retest
flakes :(

adriengentil · 2023-11-29T12:35:55Z

@avishayt @gamli75 @rccrdpccl are we good to go with this PR?

adriengentil · 2023-11-29T12:36:39Z

/override ci/prow/edge-e2e-nutanix-assisted ci/prow/edge-e2e-nutanix-assisted-4-14 ci/prow/edge-e2e-vsphere-assisted
nutanix jobs are broken
vsphere job is flaky

openshift-ci · 2023-11-29T12:37:06Z

@adriengentil: Overrode contexts on behalf of adriengentil: ci/prow/edge-e2e-nutanix-assisted, ci/prow/edge-e2e-nutanix-assisted-4-14, ci/prow/edge-e2e-vsphere-assisted

In response to this:

/override ci/prow/edge-e2e-nutanix-assisted ci/prow/edge-e2e-nutanix-assisted-4-14 ci/prow/edge-e2e-vsphere-assisted
nutanix jobs is broken
vsphere job is flaky

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.

rccrdpccl · 2023-11-29T14:25:15Z

/lgtm
/hold

Unhold whenever we're ready :)

adriengentil · 2023-11-29T15:42:33Z

/unhold

openshift-ci-robot · 2023-11-29T17:37:00Z

/retest-required

Remaining retests: 0 against base HEAD e13da27 and 2 for PR HEAD 8317fec in total

adriengentil · 2023-11-29T19:20:28Z

/retest

openshift-ci · 2023-11-29T21:48:34Z

@adriengentil: all tests passed!

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/test-infra repository. I understand the commands that are listed here.

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 27, 2023

openshift-ci bot added the api-review Categorizes an issue or PR as actively needing an API review. label Nov 27, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2023

adriengentil force-pushed the support-external-platform-4 branch from 935ee0c to 6995910 Compare November 27, 2023 13:56

adriengentil changed the title ~~support external platform 4~~ MGMT-15690: Add support for external platform Nov 27, 2023

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

openshift-ci bot requested a review from pawanpinjarkar November 27, 2023 14:28

openshift-ci bot requested review from avishayt and gamli75 November 27, 2023 16:00

adriengentil marked this pull request as ready for review November 27, 2023 16:00

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 27, 2023

openshift-ci bot requested review from danielerez and danmanor November 27, 2023 16:06

adriengentil added 4 commits November 27, 2023 17:19

define external platform type in swagger

413ee0b

generate code

fa23661

create external provider

2c0263a

Add capability check for external platform

d7c9294

adriengentil added 2 commits November 27, 2023 17:19

update cluster

b9c3823

add unit tests for the external provider

8e08cb3

adriengentil force-pushed the support-external-platform-4 branch from 6995910 to 8e08cb3 Compare November 27, 2023 16:29

rccrdpccl reviewed Nov 28, 2023

View reviewed changes

internal/bminventory/inventory.go Outdated Show resolved Hide resolved

internal/provider/common.go Outdated Show resolved Hide resolved

internal/provider/common.go Outdated Show resolved Hide resolved

gamli75 reviewed Nov 28, 2023

View reviewed changes

adriengentil added 2 commits November 28, 2023 11:33

refactor updatePlatformParams

c44ecac

refactor getUpdateParamsForPlatformUMNMandatory

1e1f728

avishayt reviewed Nov 28, 2023

View reviewed changes

adriengentil added 2 commits November 28, 2023 15:36

split provider/common.go into platform.go and platform_validations.go

ed0ba31

remove redondant checks with swagger

8317fec

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 29, 2023

openshift-ci bot assigned rccrdpccl Nov 29, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 29, 2023

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

pawanpinjarkar mentioned this pull request Nov 29, 2023

AGENT-736: Dev-script support for generic external platform openshift-metal3/dev-scripts#1611

Merged

openshift-merge-bot bot merged commit 5582850 into openshift:master Nov 29, 2023
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-15690: Add support for external platform #5738

MGMT-15690: Add support for external platform #5738

adriengentil commented Nov 27, 2023 •

edited

openshift-ci bot commented Nov 27, 2023

adriengentil commented Nov 27, 2023

openshift-ci bot commented Nov 27, 2023

adriengentil commented Nov 27, 2023

codecov bot commented Nov 27, 2023 •

edited

openshift-ci-robot commented Nov 27, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Nov 27, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Nov 27, 2023 •

edited by openshift-ci bot

adriengentil commented Nov 27, 2023

adriengentil commented Nov 27, 2023 •

edited

adriengentil commented Nov 27, 2023

adriengentil commented Nov 27, 2023

adriengentil commented Nov 27, 2023

adriengentil commented Nov 28, 2023

gamli75 Nov 28, 2023

adriengentil Nov 28, 2023 •

edited

gamli75 Nov 28, 2023

adriengentil Nov 28, 2023

gamli75 Nov 28, 2023

adriengentil Nov 28, 2023 •

edited

avishayt Nov 28, 2023

adriengentil Nov 28, 2023 •

edited

openshift-ci-robot commented Nov 28, 2023 •

edited by openshift-ci bot

pawanpinjarkar commented Nov 28, 2023

adriengentil commented Nov 29, 2023

adriengentil commented Nov 29, 2023

adriengentil commented Nov 29, 2023

adriengentil commented Nov 29, 2023 •

edited

openshift-ci bot commented Nov 29, 2023

rccrdpccl commented Nov 29, 2023

adriengentil commented Nov 29, 2023

openshift-ci-robot commented Nov 29, 2023

adriengentil commented Nov 29, 2023

openshift-ci bot commented Nov 29, 2023

MGMT-15690: Add support for external platform #5738

MGMT-15690: Add support for external platform #5738

Conversation

adriengentil commented Nov 27, 2023 • edited

openshift-ci bot commented Nov 27, 2023

adriengentil commented Nov 27, 2023

openshift-ci bot commented Nov 27, 2023

adriengentil commented Nov 27, 2023

codecov bot commented Nov 27, 2023 • edited

Codecov Report

openshift-ci-robot commented Nov 27, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Nov 27, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Nov 27, 2023 • edited by openshift-ci bot

adriengentil commented Nov 27, 2023

adriengentil commented Nov 27, 2023 • edited

adriengentil commented Nov 27, 2023

adriengentil commented Nov 27, 2023

adriengentil commented Nov 27, 2023

adriengentil commented Nov 28, 2023

gamli75 Nov 28, 2023

Choose a reason for hiding this comment

adriengentil Nov 28, 2023 • edited

Choose a reason for hiding this comment

gamli75 Nov 28, 2023

Choose a reason for hiding this comment

adriengentil Nov 28, 2023

Choose a reason for hiding this comment

gamli75 Nov 28, 2023

Choose a reason for hiding this comment

adriengentil Nov 28, 2023 • edited

Choose a reason for hiding this comment

avishayt Nov 28, 2023

Choose a reason for hiding this comment

adriengentil Nov 28, 2023 • edited

Choose a reason for hiding this comment

openshift-ci-robot commented Nov 28, 2023 • edited by openshift-ci bot

pawanpinjarkar commented Nov 28, 2023

adriengentil commented Nov 29, 2023

adriengentil commented Nov 29, 2023

adriengentil commented Nov 29, 2023

adriengentil commented Nov 29, 2023 • edited

openshift-ci bot commented Nov 29, 2023

rccrdpccl commented Nov 29, 2023

adriengentil commented Nov 29, 2023

openshift-ci-robot commented Nov 29, 2023

adriengentil commented Nov 29, 2023

openshift-ci bot commented Nov 29, 2023

adriengentil commented Nov 27, 2023 •

edited

codecov bot commented Nov 27, 2023 •

edited

openshift-ci-robot commented Nov 27, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Nov 27, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Nov 27, 2023 •

edited by openshift-ci bot

adriengentil commented Nov 27, 2023 •

edited

adriengentil Nov 28, 2023 •

edited

adriengentil Nov 28, 2023 •

edited

adriengentil Nov 28, 2023 •

edited

openshift-ci-robot commented Nov 28, 2023 •

edited by openshift-ci bot

adriengentil commented Nov 29, 2023 •

edited