Skip to content

Commit

Permalink
Refactoring cluster manager to invoke provider's SpecChanged method (#…
Browse files Browse the repository at this point in the history 
…1808)

* Refactoring cluster manager to invoke provider's SpecChanged method

* Linting

* Adding unit tests for provider SpecChanged method

* Make test independent of machineconfig order

* Moving vsphere provider spec comparison logic into provider component

* Fixing unit test comments and naming

* Refactoring to use existing UpgradeNeeded provider method

* Simplifying changes to avoid unnecessary calls

* Renaming unused variables and removing unused function

* Removing unnecessary capi manager validation logic
  • Loading branch information
@maxdrib
maxdrib committed Apr 13, 2022
1 parent 8697e37 commit 1b065ac
Show file tree
Hide file tree
Showing 15 changed files with 285 additions and 235 deletions.
105 changes: 2 additions & 103 deletions pkg/clustermanager/cluster_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
_ "embed"
"errors"
"fmt"
"reflect"
"strings"
"time"

Expand Down Expand Up @@ -449,7 +448,7 @@ func (c *ClusterManager) UpgradeCluster(ctx context.Context, managementCluster,
return nil
}

func (c *ClusterManager) EKSAClusterSpecChanged(ctx context.Context, cluster *types.Cluster, newClusterSpec *cluster.Spec, datacenterConfig providers.DatacenterConfig, machineConfigs []providers.MachineConfig) (bool, error) {
func (c *ClusterManager) EKSAClusterSpecChanged(ctx context.Context, cluster *types.Cluster, newClusterSpec *cluster.Spec) (bool, error) {
cc, err := c.clusterClient.GetEksaCluster(ctx, cluster, newClusterSpec.Cluster.Name)
if err != nil {
return false, err
Expand Down Expand Up @@ -477,107 +476,7 @@ func (c *ClusterManager) EKSAClusterSpecChanged(ctx context.Context, cluster *ty
}
}

logger.V(3).Info("Clusters are the same, checking provider spec")
// compare provider spec
switch cc.Spec.DatacenterRef.Kind {
case v1alpha1.VSphereDatacenterKind:
machineConfigMap := make(map[string]*v1alpha1.VSphereMachineConfig)

existingVdc, err := c.clusterClient.GetEksaVSphereDatacenterConfig(ctx, cc.Spec.DatacenterRef.Name, cluster.KubeconfigFile, newClusterSpec.Cluster.Namespace)
if err != nil {
return false, err
}
vdc := datacenterConfig.(*v1alpha1.VSphereDatacenterConfig)
if !reflect.DeepEqual(existingVdc.Spec, vdc.Spec) {
logger.V(3).Info("New provider spec is different from the new spec")
return true, nil
}

for _, config := range machineConfigs {
mc := config.(*v1alpha1.VSphereMachineConfig)
machineConfigMap[mc.Name] = mc
}
existingCpVmc, err := c.clusterClient.GetEksaVSphereMachineConfig(ctx, cc.Spec.ControlPlaneConfiguration.MachineGroupRef.Name, cluster.KubeconfigFile, newClusterSpec.Cluster.Namespace)
if err != nil {
return false, err
}
cpVmc := machineConfigMap[newClusterSpec.Cluster.Spec.ControlPlaneConfiguration.MachineGroupRef.Name]
if !reflect.DeepEqual(existingCpVmc.Spec, cpVmc.Spec) {
logger.V(3).Info("New control plane machine config spec is different from the existing spec")
return true, nil
}
for _, workerNodeGroupConfiguration := range cc.Spec.WorkerNodeGroupConfigurations {
existingWnVmc, err := c.clusterClient.GetEksaVSphereMachineConfig(ctx, workerNodeGroupConfiguration.MachineGroupRef.Name, cluster.KubeconfigFile, newClusterSpec.Cluster.Namespace)
if err != nil {
return false, err
}
wnVmc := machineConfigMap[workerNodeGroupConfiguration.MachineGroupRef.Name]
if !reflect.DeepEqual(existingWnVmc.Spec, wnVmc.Spec) {
logger.V(3).Info("New worker node machine config spec is different from the existing spec")
return true, nil
}
}
if cc.Spec.ExternalEtcdConfiguration != nil {
existingEtcdVmc, err := c.clusterClient.GetEksaVSphereMachineConfig(ctx, cc.Spec.ExternalEtcdConfiguration.MachineGroupRef.Name, cluster.KubeconfigFile, newClusterSpec.Cluster.Namespace)
if err != nil {
return false, err
}
etcdVmc := machineConfigMap[newClusterSpec.Cluster.Spec.ExternalEtcdConfiguration.MachineGroupRef.Name]
if !reflect.DeepEqual(existingEtcdVmc.Spec, etcdVmc.Spec) {
logger.V(3).Info("New etcd machine config spec is different from the existing spec")
return true, nil
}
}
case v1alpha1.CloudStackDatacenterKind:
existingCsdc, err := c.clusterClient.GetEksaCloudStackDatacenterConfig(ctx, cc.Spec.DatacenterRef.Name, cluster.KubeconfigFile, newClusterSpec.Cluster.Namespace)
if err != nil {
return false, err
}
csDc := datacenterConfig.(*v1alpha1.CloudStackDatacenterConfig)
if !existingCsdc.Spec.Equal(&csDc.Spec) {
logger.V(3).Info("New provider spec is different from the new spec")
return true, nil
}

machineConfigsSpecChanged, err := c.cloudstackMachineConfigsSpecChanged(ctx, cc, cluster, newClusterSpec, machineConfigs)
if err != nil {
return false, err
}
if machineConfigsSpecChanged {
return true, nil
}
// TODO: Make sure happy paths at least are unit tested, especially if moving code to provider
default:
// Run upgrade flow
return true, nil
}

return false, nil
}

func (c *ClusterManager) cloudstackMachineConfigsSpecChanged(ctx context.Context, cc *v1alpha1.Cluster, cluster *types.Cluster, newClusterSpec *cluster.Spec, machineConfigs []providers.MachineConfig) (bool, error) {
machineConfigMap := make(map[string]*v1alpha1.CloudStackMachineConfig)
for _, config := range machineConfigs {
mc := config.(*v1alpha1.CloudStackMachineConfig)
machineConfigMap[mc.Name] = mc
}

for _, oldMcRef := range cc.MachineConfigRefs() {
existingCsmc, err := c.clusterClient.GetEksaCloudStackMachineConfig(ctx, oldMcRef.Name, cluster.KubeconfigFile, newClusterSpec.Cluster.Namespace)
if err != nil {
return false, err
}
csmc, ok := machineConfigMap[oldMcRef.Name]
if !ok {
logger.V(3).Info(fmt.Sprintf("Old machine config spec %s not found in the existing spec", oldMcRef.Name))
return true, nil
}
if !existingCsmc.Spec.Equal(&csmc.Spec) {
logger.V(3).Info(fmt.Sprintf("New machine config spec %s is different from the existing spec", oldMcRef.Name))
return true, nil
}
}

logger.V(3).Info("Clusters are the same")
return false, nil
}

Expand Down
64 changes: 4 additions & 60 deletions pkg/clustermanager/cluster_manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1031,11 +1031,8 @@ func TestClusterManagerClusterSpecChangedNoChanges(t *testing.T) {
tt.mocks.client.EXPECT().GetEksaCluster(tt.ctx, tt.cluster, tt.clusterSpec.Cluster.Name).Return(tt.oldClusterConfig, nil)
tt.mocks.client.EXPECT().GetBundles(tt.ctx, tt.cluster.KubeconfigFile, tt.cluster.Name, "").Return(test.Bundles(t), nil)
tt.mocks.client.EXPECT().GetEksdRelease(tt.ctx, gomock.Any(), constants.EksaSystemNamespace, gomock.Any())
tt.mocks.client.EXPECT().GetEksaVSphereDatacenterConfig(tt.ctx, tt.oldClusterConfig.Spec.DatacenterRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldDatacenterConfig, nil)
tt.mocks.client.EXPECT().GetEksaVSphereMachineConfig(tt.ctx, tt.oldClusterConfig.Spec.ControlPlaneConfiguration.MachineGroupRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldControlPlaneMachineConfig, nil)
tt.mocks.client.EXPECT().GetEksaVSphereMachineConfig(tt.ctx, tt.oldClusterConfig.Spec.WorkerNodeGroupConfigurations[0].MachineGroupRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldWorkerMachineConfig, nil)
tt.mocks.client.EXPECT().GetEksaOIDCConfig(tt.ctx, tt.oldClusterConfig.Spec.IdentityProviderRefs[0].Name, tt.cluster.KubeconfigFile, tt.clusterSpec.Cluster.Namespace).Return(tt.oldOIDCConfig, nil)
diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec, tt.newDatacenterConfig, []providers.MachineConfig{tt.newControlPlaneMachineConfig, tt.newWorkerMachineConfig})
diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec)
assert.Nil(t, err, "Error should be nil")
assert.False(t, diff, "No changes should have been detected")
}
Expand All @@ -1045,7 +1042,7 @@ func TestClusterManagerClusterSpecChangedClusterChanged(t *testing.T) {
tt.newClusterConfig.Spec.KubernetesVersion = "1.20"

tt.mocks.client.EXPECT().GetEksaCluster(tt.ctx, tt.cluster, tt.clusterSpec.Cluster.Name).Return(tt.oldClusterConfig, nil)
diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec, tt.newDatacenterConfig, []providers.MachineConfig{tt.newControlPlaneMachineConfig, tt.newWorkerMachineConfig})
diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec)
assert.Nil(t, err, "Error should be nil")
assert.True(t, diff, "Changes should have been detected")
}
Expand All @@ -1058,57 +1055,7 @@ func TestClusterManagerClusterSpecChangedEksDReleaseChanged(t *testing.T) {
tt.mocks.client.EXPECT().GetBundles(tt.ctx, tt.cluster.KubeconfigFile, tt.cluster.Name, "").Return(test.Bundles(t), nil)
tt.mocks.client.EXPECT().GetEksdRelease(tt.ctx, gomock.Any(), constants.EksaSystemNamespace, gomock.Any())
tt.mocks.client.EXPECT().GetEksaOIDCConfig(tt.ctx, tt.clusterSpec.Cluster.Spec.IdentityProviderRefs[0].Name, tt.cluster.KubeconfigFile, tt.clusterSpec.Cluster.Namespace).Return(tt.oldOIDCConfig, nil)
diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec, tt.newDatacenterConfig, []providers.MachineConfig{tt.newControlPlaneMachineConfig, tt.newWorkerMachineConfig})
assert.Nil(t, err, "Error should be nil")
assert.True(t, diff, "Changes should have been detected")
}

func TestClusterManagerClusterSpecChangedNoChangesDatacenterSpecChanged(t *testing.T) {
tt := newSpecChangedTest(t)
tt.newDatacenterConfig.Spec.Insecure = false
tt.clusterSpec.OIDCConfig = tt.oldOIDCConfig.DeepCopy()

tt.mocks.client.EXPECT().GetEksaCluster(tt.ctx, tt.cluster, tt.clusterSpec.Cluster.Name).Return(tt.oldClusterConfig, nil)
tt.mocks.client.EXPECT().GetBundles(tt.ctx, tt.cluster.KubeconfigFile, tt.cluster.Name, "").Return(test.Bundles(t), nil)
tt.mocks.client.EXPECT().GetEksdRelease(tt.ctx, gomock.Any(), constants.EksaSystemNamespace, gomock.Any())
tt.mocks.client.EXPECT().GetEksaOIDCConfig(tt.ctx, tt.clusterSpec.Cluster.Spec.IdentityProviderRefs[0].Name, tt.cluster.KubeconfigFile, tt.clusterSpec.Cluster.Namespace).Return(tt.oldOIDCConfig, nil)
tt.mocks.client.EXPECT().GetEksaVSphereDatacenterConfig(tt.ctx, tt.oldClusterConfig.Spec.DatacenterRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldDatacenterConfig, nil)
diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec, tt.newDatacenterConfig, []providers.MachineConfig{tt.newControlPlaneMachineConfig, tt.newWorkerMachineConfig})
assert.Nil(t, err, "Error should be nil")
assert.True(t, diff, "Changes should have been detected")
}

func TestClusterManagerClusterSpecChangedNoChangesControlPlaneMachineConfigSpecChanged(t *testing.T) {
tt := newSpecChangedTest(t)
tt.newControlPlaneMachineConfig.Spec.NumCPUs = 4
tt.clusterSpec.OIDCConfig = tt.oldOIDCConfig.DeepCopy()

tt.mocks.client.EXPECT().GetEksaCluster(tt.ctx, tt.cluster, tt.clusterSpec.Cluster.Name).Return(tt.oldClusterConfig, nil)
tt.mocks.client.EXPECT().GetBundles(tt.ctx, tt.cluster.KubeconfigFile, tt.cluster.Name, "").Return(test.Bundles(t), nil)
tt.mocks.client.EXPECT().GetEksdRelease(tt.ctx, gomock.Any(), constants.EksaSystemNamespace, gomock.Any())
tt.mocks.client.EXPECT().GetEksaVSphereDatacenterConfig(tt.ctx, tt.oldClusterConfig.Spec.DatacenterRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldDatacenterConfig, nil)
tt.mocks.client.EXPECT().GetEksaVSphereMachineConfig(tt.ctx, tt.oldClusterConfig.Spec.ControlPlaneConfiguration.MachineGroupRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldControlPlaneMachineConfig, nil)
tt.mocks.client.EXPECT().GetEksaOIDCConfig(tt.ctx, tt.clusterSpec.Cluster.Spec.IdentityProviderRefs[0].Name, tt.cluster.KubeconfigFile, tt.clusterSpec.Cluster.Namespace).Return(tt.oldOIDCConfig, nil)
diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec, tt.newDatacenterConfig, []providers.MachineConfig{tt.newControlPlaneMachineConfig, tt.newWorkerMachineConfig})

assert.Nil(t, err, "Error should be nil")
assert.True(t, diff, "Changes should have been detected")
}

func TestClusterManagerClusterSpecChangedNoChangesWorkerNodeMachineConfigSpecChanged(t *testing.T) {
tt := newSpecChangedTest(t)
tt.newWorkerMachineConfig.Spec.NumCPUs = 4
tt.clusterSpec.OIDCConfig = tt.oldOIDCConfig.DeepCopy()

tt.mocks.client.EXPECT().GetEksaCluster(tt.ctx, tt.cluster, tt.clusterSpec.Cluster.Name).Return(tt.oldClusterConfig, nil)
tt.mocks.client.EXPECT().GetBundles(tt.ctx, tt.cluster.KubeconfigFile, tt.cluster.Name, "").Return(test.Bundles(t), nil)
tt.mocks.client.EXPECT().GetEksdRelease(tt.ctx, gomock.Any(), constants.EksaSystemNamespace, gomock.Any())
tt.mocks.client.EXPECT().GetEksaVSphereDatacenterConfig(tt.ctx, tt.oldClusterConfig.Spec.DatacenterRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldDatacenterConfig, nil)
tt.mocks.client.EXPECT().GetEksaVSphereMachineConfig(tt.ctx, tt.oldClusterConfig.Spec.ControlPlaneConfiguration.MachineGroupRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldControlPlaneMachineConfig, nil)
tt.mocks.client.EXPECT().GetEksaVSphereMachineConfig(tt.ctx, tt.oldClusterConfig.Spec.WorkerNodeGroupConfigurations[0].MachineGroupRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldWorkerMachineConfig, nil)
tt.mocks.client.EXPECT().GetEksaOIDCConfig(tt.ctx, tt.clusterSpec.Cluster.Spec.IdentityProviderRefs[0].Name, tt.cluster.KubeconfigFile, tt.clusterSpec.Cluster.Namespace).Return(tt.oldOIDCConfig, nil)
diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec, tt.newDatacenterConfig, []providers.MachineConfig{tt.newControlPlaneMachineConfig, tt.newWorkerMachineConfig})

diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec)
assert.Nil(t, err, "Error should be nil")
assert.True(t, diff, "Changes should have been detected")
}
Expand All @@ -1125,11 +1072,8 @@ func TestClusterManagerClusterSpecChangedGitOpsDefault(t *testing.T) {
tt.mocks.client.EXPECT().GetEksaGitOpsConfig(tt.ctx, tt.clusterSpec.Cluster.Spec.GitOpsRef.Name, tt.cluster.KubeconfigFile, tt.clusterSpec.Cluster.Namespace).Return(oldGitOpsConfig, nil)
tt.mocks.client.EXPECT().GetBundles(tt.ctx, tt.cluster.KubeconfigFile, tt.cluster.Name, "").Return(test.Bundles(t), nil)
tt.mocks.client.EXPECT().GetEksdRelease(tt.ctx, gomock.Any(), constants.EksaSystemNamespace, gomock.Any())
tt.mocks.client.EXPECT().GetEksaVSphereDatacenterConfig(tt.ctx, tt.oldClusterConfig.Spec.DatacenterRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldDatacenterConfig, nil)
tt.mocks.client.EXPECT().GetEksaVSphereMachineConfig(tt.ctx, tt.oldClusterConfig.Spec.ControlPlaneConfiguration.MachineGroupRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldControlPlaneMachineConfig, nil)
tt.mocks.client.EXPECT().GetEksaVSphereMachineConfig(tt.ctx, tt.oldClusterConfig.Spec.WorkerNodeGroupConfigurations[0].MachineGroupRef.Name, gomock.Any(), gomock.Any()).Return(tt.oldWorkerMachineConfig, nil)
tt.mocks.client.EXPECT().GetEksaOIDCConfig(tt.ctx, tt.clusterSpec.Cluster.Spec.IdentityProviderRefs[0].Name, tt.cluster.KubeconfigFile, tt.clusterSpec.Cluster.Namespace).Return(tt.oldOIDCConfig, nil)
diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec, tt.newDatacenterConfig, []providers.MachineConfig{tt.newControlPlaneMachineConfig, tt.newWorkerMachineConfig})
diff, err := tt.clusterManager.EKSAClusterSpecChanged(tt.ctx, tt.cluster, tt.clusterSpec)

assert.Nil(t, err, "Error should be nil")
assert.False(t, diff, "No changes should have been detected")
Expand Down
44 changes: 41 additions & 3 deletions pkg/providers/cloudstack/cloudstack.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -971,6 +971,32 @@ func (p *cloudstackProvider) GenerateCAPISpecForCreate(ctx context.Context, clus
return controlPlaneSpec, workersSpec, nil
}

func (p *cloudstackProvider) machineConfigsSpecChanged(ctx context.Context, cc *v1alpha1.Cluster, cluster *types.Cluster, newClusterSpec *cluster.Spec) (bool, error) {
machineConfigMap := make(map[string]*v1alpha1.CloudStackMachineConfig)
for _, config := range p.MachineConfigs(nil) {
mc := config.(*v1alpha1.CloudStackMachineConfig)
machineConfigMap[mc.Name] = mc
}

for _, oldMcRef := range cc.MachineConfigRefs() {
existingCsmc, err := p.providerKubectlClient.GetEksaCloudStackMachineConfig(ctx, oldMcRef.Name, cluster.KubeconfigFile, newClusterSpec.Cluster.Namespace)
if err != nil {
return false, err
}
csmc, ok := machineConfigMap[oldMcRef.Name]
if !ok {
logger.V(3).Info(fmt.Sprintf("Old machine config spec %s not found in the existing spec", oldMcRef.Name))
return true, nil
}
if !existingCsmc.Spec.Equal(&csmc.Spec) {
logger.V(3).Info(fmt.Sprintf("New machine config spec %s is different from the existing spec", oldMcRef.Name))
return true, nil
}
}

return false, nil
}

func (p *cloudstackProvider) GenerateMHC() ([]byte, error) {
data := map[string]string{
"clusterName": p.clusterConfig.Name,
Expand Down Expand Up @@ -1057,10 +1083,22 @@ func (p *cloudstackProvider) MachineConfigs(_ *cluster.Spec) []providers.Machine
return configs
}

func (p *cloudstackProvider) UpgradeNeeded(ctx context.Context, newSpec, currentSpec *cluster.Spec) (bool, error) {
newV, oldV := newSpec.VersionsBundle.CloudStack, currentSpec.VersionsBundle.CloudStack
func (p *cloudstackProvider) UpgradeNeeded(ctx context.Context, newSpec, currentSpec *cluster.Spec, cluster *types.Cluster) (bool, error) {
cc := currentSpec.Cluster
existingCsdc, err := p.providerKubectlClient.GetEksaCloudStackDatacenterConfig(ctx, cc.Spec.DatacenterRef.Name, cluster.KubeconfigFile, newSpec.Cluster.Namespace)
if err != nil {
return false, err
}
if !existingCsdc.Spec.Equal(&p.datacenterConfig.Spec) {
logger.V(3).Info("New provider spec is different from the new spec")
return true, nil
}

return newV.ClusterAPIController.ImageDigest != oldV.ClusterAPIController.ImageDigest, nil
machineConfigsSpecChanged, err := p.machineConfigsSpecChanged(ctx, cc, cluster, newSpec)
if err != nil {
return false, err
}
return machineConfigsSpecChanged, nil
}

func (p *cloudstackProvider) DeleteResources(ctx context.Context, clusterSpec *cluster.Spec) error {
Expand Down

0 comments on commit 1b065ac

Please sign in to comment.