Skip to content

manifests/infrastructure: Setup the platform specific status fields. #1930

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 3 commits into from
Jul 2, 2019
Merged

manifests/infrastructure: Setup the platform specific status fields. #1930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eef6a3d
vendor: update openshift/api
abhinavdahiya Jul 1, 2019
fa1abe5
pkg/asset/manifests/infrastructure: Set status.platformStatus.aws.region
wking May 7, 2019
66ce047
asset/manifests/infrastructure: setup the platform status for Azure, GCP
abhinavdahiya Jul 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 40 additions & 19 deletions pkg/asset/manifests/infrastructure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package manifests

import (
"fmt"
"path/filepath"

"github.com/ghodss/yaml"
Expand All @@ -13,6 +14,7 @@ import (

"github.com/openshift/installer/pkg/types/aws"
"github.com/openshift/installer/pkg/types/azure"
"github.com/openshift/installer/pkg/types/gcp"
"github.com/openshift/installer/pkg/types/libvirt"
"github.com/openshift/installer/pkg/types/none"
"github.com/openshift/installer/pkg/types/openstack"
Expand Down Expand Up @@ -53,24 +55,6 @@ func (i *Infrastructure) Generate(dependencies asset.Parents) error {
cloudproviderconfig := &CloudProviderConfig{}
dependencies.Get(clusterID, installConfig, cloudproviderconfig)

var platform configv1.PlatformType
switch installConfig.Config.Platform.Name() {
case aws.Name:
platform = configv1.AWSPlatformType
case none.Name:
platform = configv1.NonePlatformType
case libvirt.Name:
platform = configv1.LibvirtPlatformType
case openstack.Name:
platform = configv1.OpenStackPlatformType
case vsphere.Name:
platform = configv1.VSpherePlatformType
case azure.Name:
platform = configv1.AzurePlatformType
default:
platform = configv1.NonePlatformType
}

config := &configv1.Infrastructure{
TypeMeta: metav1.TypeMeta{
APIVersion: configv1.SchemeGroupVersion.String(),
Expand All @@ -82,13 +66,50 @@ func (i *Infrastructure) Generate(dependencies asset.Parents) error {
},
Status: configv1.InfrastructureStatus{
InfrastructureName: clusterID.InfraID,
Platform: platform,
APIServerURL: getAPIServerURL(installConfig.Config),
APIServerInternalURL: getInternalAPIServerURL(installConfig.Config),
EtcdDiscoveryDomain: getEtcdDiscoveryDomain(installConfig.Config),
PlatformStatus: &configv1.PlatformStatus{},
Copy link
Member

@runcom runcom Jul 2, 2019

Choose a reason for hiding this comment

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

The MCO was reading Platform- is it just enough to switch to PlatformStatus? cc @sgreene570

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Jul 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@sgreene570 @runcom
This is only set in new 4.2 clusters.
4.1 clusters do not have .status.platformStatus set.

A migration will be required to provide this field to all 4.1 clusters.. or operators can use fields if set and fall back to old field....
Still seeing if a migration will be required as only 4.2 platforms require .status.platformStatus to be set.

Copy link
Member

@runcom runcom Jul 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I know, so it's ok to switch to the new platformStatus in 4.2 and live with that as you're doing in this PR?

Still seeing if a migration will be required as only 4.2 platforms require .status.platformStatus to be set.

👍 awesome, that's what I was looking for as well for MCO specifically, thanks!

},
}

switch installConfig.Config.Platform.Name() {
case aws.Name:
config.Status.Platform = configv1.AWSPlatformType
config.Status.PlatformStatus.Type = configv1.AWSPlatformType
config.Status.PlatformStatus.AWS = &configv1.AWSPlatformStatus{
Region: installConfig.Config.Platform.AWS.Region,
}
case azure.Name:
config.Status.Platform = configv1.AzurePlatformType
config.Status.PlatformStatus.Type = configv1.AzurePlatformType
config.Status.PlatformStatus.Azure = &configv1.AzurePlatformStatus{
ResourceGroupName: fmt.Sprintf("%s-rg", clusterID.InfraID),
}
case gcp.Name:
config.Status.Platform = configv1.GCPPlatformType
config.Status.PlatformStatus.Type = configv1.GCPPlatformType
config.Status.PlatformStatus.GCP = &configv1.GCPPlatformStatus{
ProjectID: installConfig.Config.Platform.GCP.ProjectID,
Region: installConfig.Config.Platform.GCP.Region,
}
case libvirt.Name:
config.Status.Platform = configv1.LibvirtPlatformType
config.Status.PlatformStatus.Type = configv1.LibvirtPlatformType
case none.Name:
config.Status.Platform = configv1.NonePlatformType
config.Status.PlatformStatus.Type = configv1.NonePlatformType
case openstack.Name:
config.Status.Platform = configv1.OpenStackPlatformType
config.Status.PlatformStatus.Type = configv1.OpenStackPlatformType
case vsphere.Name:
config.Status.Platform = configv1.VSpherePlatformType
config.Status.PlatformStatus.Type = configv1.VSpherePlatformType
default:
config.Status.PlatformStatus.Type = configv1.NonePlatformType
}
config.Status.Platform = config.Status.PlatformStatus.Type
Copy link
Contributor

@patrickdillon patrickdillon Jul 2, 2019

Choose a reason for hiding this comment

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

It seems pointless to have this assignment and the same assignment in the case statements. The assignment in the case statements will be undone by this. Which one we keep depends on whether they are always equal or if there can be skew depending on the platform.

Copy link
Contributor

@ironcladlou ironcladlou Jul 2, 2019

Choose a reason for hiding this comment

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

Good catch, agree.

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Jul 2, 2019

Choose a reason for hiding this comment

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

opened #1934 to fix it.


if cloudproviderconfig.ConfigMap != nil {
// set the configmap reference.
config.Spec.CloudConfig = configv1.ConfigMapFileReference{Name: cloudproviderconfig.ConfigMap.Name, Key: cloudProviderConfigDataKey}
Expand Down
5 changes: 4 additions & 1 deletion vendor/github.com/openshift/api/config/v1/types_console.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 23 additions & 0 deletions vendor/github.com/openshift/api/config/v1/types_infrastructure.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 40 additions & 1 deletion vendor/github.com/openshift/api/config/v1/types_network.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

99 changes: 99 additions & 0 deletions vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading