Skip to content
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

Add in place rollout strategy option #7328

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Add in place rollout strategy option #7328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 8 additions & 7 deletions pkg/api/v1alpha1/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,19 +825,20 @@ func validatePodIAMConfig(clusterConfig *Cluster) error {
}

func validateCPUpgradeRolloutStrategy(clusterConfig *Cluster) error {
if clusterConfig.Spec.ControlPlaneConfiguration.UpgradeRolloutStrategy == nil {
cpUpgradeRolloutStrategy := clusterConfig.Spec.ControlPlaneConfiguration.UpgradeRolloutStrategy
if cpUpgradeRolloutStrategy == nil {
return nil
}

if clusterConfig.Spec.ControlPlaneConfiguration.UpgradeRolloutStrategy.Type != "RollingUpdate" {
return fmt.Errorf("ControlPlaneConfiguration: only 'RollingUpdate' supported for upgrade rollout strategy type")
if cpUpgradeRolloutStrategy.Type != "RollingUpdate" && cpUpgradeRolloutStrategy.Type != "InPlace" {
return fmt.Errorf("ControlPlaneConfiguration: only 'RollingUpdate' and 'InPlace' are supported for upgrade rollout strategy type")
}

if clusterConfig.Spec.ControlPlaneConfiguration.UpgradeRolloutStrategy.RollingUpdate.MaxSurge < 0 {
if cpUpgradeRolloutStrategy.RollingUpdate.MaxSurge < 0 {
return fmt.Errorf("ControlPlaneConfiguration: maxSurge for control plane cannot be a negative value")
}

if clusterConfig.Spec.ControlPlaneConfiguration.UpgradeRolloutStrategy.RollingUpdate.MaxSurge > 1 {
if cpUpgradeRolloutStrategy.RollingUpdate.MaxSurge > 1 {
return fmt.Errorf("ControlPlaneConfiguration: maxSurge for control plane must be 0 or 1")
}

Expand All @@ -849,8 +850,8 @@ func validateMDUpgradeRolloutStrategy(w *WorkerNodeGroupConfiguration) error {
return nil
}

if w.UpgradeRolloutStrategy.Type != "RollingUpdate" {
return fmt.Errorf("WorkerNodeGroupConfiguration: only 'RollingUpdate' supported for upgrade rollout strategy type")
if w.UpgradeRolloutStrategy.Type != "RollingUpdate" && w.UpgradeRolloutStrategy.Type != "InPlace" {
return fmt.Errorf("WorkerNodeGroupConfiguration: only 'RollingUpdate' and 'InPlace' are supported for upgrade rollout strategy type")
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a provider agnostic check. So looks like we might need to restrict "InPlace" at other providers once the rolloutStrategy support PR for other providers get merged in.? Another option is to check if the type is inplace and if cpUpgradeRolloutStrategy.Type == "InPlace" && cluster.Spec.DatacenterRef.Kind != TinkerbellDatacenterKind and error out for this case here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'll include the logic to restrict this feature for the other providers in the validations PR.

}

if w.UpgradeRolloutStrategy.RollingUpdate.MaxSurge < 0 || w.UpgradeRolloutStrategy.RollingUpdate.MaxUnavailable < 0 {
Expand Down
4 changes: 2 additions & 2 deletions pkg/api/v1alpha1/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3246,7 +3246,7 @@ func TestValidateCPUpgradeRolloutStrategy(t *testing.T) {
}{
{
name: "rolling upgrade strategy invalid",
wantErr: "ControlPlaneConfiguration: only 'RollingUpdate' supported for upgrade rollout strategy type",
wantErr: "ControlPlaneConfiguration: only 'RollingUpdate' and 'InPlace' are supported for upgrade rollout strategy type",
cluster: &Cluster{
Spec: ClusterSpec{
ControlPlaneConfiguration: ControlPlaneConfiguration{
Expand Down Expand Up @@ -3332,7 +3332,7 @@ func TestValidateMDUpgradeRolloutStrategy(t *testing.T) {
}{
{
name: "rolling upgrade strategy invalid",
wantErr: "WorkerNodeGroupConfiguration: only 'RollingUpdate' supported for upgrade rollout strategy type",
wantErr: "WorkerNodeGroupConfiguration: only 'RollingUpdate' and 'InPlace' are supported for upgrade rollout strategy type",
cluster: &Cluster{
Spec: ClusterSpec{
WorkerNodeGroupConfigurations: []WorkerNodeGroupConfiguration{{
Expand Down
5 changes: 3 additions & 2 deletions pkg/providers/tinkerbell/assert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,8 @@ func ExtraHardwareAvailableAssertionForRollingUpgrade(catalogue *hardware.Catalo
func ensureCPHardwareAvailability(spec *ClusterSpec, current ValidatableCluster, hwReq minimumHardwareRequirements) error {
maxSurge := 1

if spec.Cluster.Spec.ControlPlaneConfiguration.UpgradeRolloutStrategy != nil {
rolloutStrategy := spec.Cluster.Spec.ControlPlaneConfiguration.UpgradeRolloutStrategy
if rolloutStrategy != nil && rolloutStrategy.Type == "RollingUpdate" {
maxSurge = spec.Cluster.Spec.ControlPlaneConfiguration.UpgradeRolloutStrategy.RollingUpdate.MaxSurge
}
err := hwReq.Add(
Expand All @@ -489,7 +490,7 @@ func ensureWorkerHardwareAvailability(spec *ClusterSpec, current ValidatableClus
// As rolling upgrades and scale up/down is not permitted in a single operation, its safe to access directly using the md name.
mdName := fmt.Sprintf("%s-%s", spec.Cluster.Name, nodeGroup.Name)
if currentWngK8sversion[mdName] != desiredWngK8sVersion[mdName] || eksaVersionUpgrade {
if nodeGroup.UpgradeRolloutStrategy != nil {
if nodeGroup.UpgradeRolloutStrategy != nil && nodeGroup.UpgradeRolloutStrategy.Type == "RollingUpdate" {
maxSurge = nodeGroup.UpgradeRolloutStrategy.RollingUpdate.MaxSurge
}
err := hwReq.Add(
Expand Down
25 changes: 25 additions & 0 deletions pkg/providers/tinkerbell/tinkerbell_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,31 @@ func TestTinkerbellProviderGenerateDeploymentFileWithExternalEtcd(t *testing.T)
test.AssertContentToFile(t, string(md), "testdata/expected_results_cluster_tinkerbell_md.yaml")
}

func TestTinkerbellProviderSetupAndValidateInPlaceBottlerocketNotSupported(t *testing.T) {
clusterSpecManifest := "cluster_tinkerbell_bottlerocket_minimal_registry_mirror.yaml"
mockCtrl := gomock.NewController(t)
docker := stackmocks.NewMockDocker(mockCtrl)
helm := stackmocks.NewMockHelm(mockCtrl)
kubectl := mocks.NewMockProviderKubectlClient(mockCtrl)
stackInstaller := stackmocks.NewMockStackInstaller(mockCtrl)
writer := filewritermocks.NewMockFileWriter(mockCtrl)
forceCleanup := false

clusterSpec := givenClusterSpec(t, clusterSpecManifest)
clusterSpec.Cluster.Spec.ControlPlaneConfiguration.UpgradeRolloutStrategy.Type = "InPlace"
datacenterConfig := givenDatacenterConfig(t, clusterSpecManifest)
machineConfigs := givenMachineConfigs(t, clusterSpecManifest)
ctx := context.Background()

provider := newProvider(datacenterConfig, machineConfigs, clusterSpec.Cluster, writer, docker, helm, kubectl, forceCleanup)
provider.stackInstaller = stackInstaller

stackInstaller.EXPECT().CleanupLocalBoots(ctx, forceCleanup)

err := provider.SetupAndValidateCreateCluster(ctx, clusterSpec)
assert.Error(t, err, "expect validation to fail because in place upgrades are only supported on Ubuntu OS family")
}

func TestGenerateDeploymentFileWithMachineConfigOSImageExternalEtcd(t *testing.T) {
t.Skip("External etcd unsupported for GA")
clusterSpecManifest := "cluster_osimage_machine_config_external_etcd.yaml"
Expand Down
11 changes: 11 additions & 0 deletions pkg/providers/tinkerbell/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,22 @@
if spec.MachineConfigs[groupRef.Name].OSFamily() != controlPlaneOsFamily {
return fmt.Errorf("worker node group osFamily cannot be different from control plane osFamily")
}
if group.UpgradeRolloutStrategy != nil {
if spec.MachineConfigs[groupRef.Name].OSFamily() != v1alpha1.DefaultOSFamily && group.UpgradeRolloutStrategy.Type == "InPlace" {
return fmt.Errorf("InPlace upgrades are only supported on the Ubuntu OS family")
}

Check warning on line 33 in pkg/providers/tinkerbell/validate.go

View check run for this annotation

Codecov / codecov/patch

pkg/providers/tinkerbell/validate.go#L32-L33 

Added lines #L32 - L33 were not covered by tests
}
}

if controlPlaneOsFamily != v1alpha1.Bottlerocket && spec.DatacenterConfig.Spec.OSImageURL == "" && spec.ControlPlaneMachineConfig().Spec.OSImageURL == "" {
return fmt.Errorf("please use bottlerocket as osFamily for auto-importing or provide a valid osImageURL")
}

if spec.ControlPlaneConfiguration().UpgradeRolloutStrategy != nil {
if controlPlaneOsFamily != v1alpha1.DefaultOSFamily && spec.ControlPlaneConfiguration().UpgradeRolloutStrategy.Type == "InPlace" {
Copy link
Member

Choose a reason for hiding this comment

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

While we check for the CP and Worker rollout strategy types separately, do we also want to check if one is specified as InPlace other cannot be rolling.? I don't think our controllers are designed in such a way but this will prevent unnecessary complications.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure that makes sense. There is a separate task for fully validating this feature, so I can include this additional check there

return fmt.Errorf("InPlace upgrades are only supported on the Ubuntu OS family")
}
}
return nil
}

Expand Down