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.

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

refactor: Introduce DeletionProcessingMode #16293

Merged
merged 1 commit into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 5 additions & 1 deletion upup/pkg/fi/cloudup/apply_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,7 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error {
var target fi.CloudupTarget
shouldPrecreateDNS := true

deletionProcessingMode := fi.DeletionProcessingModeDeleteIfNotDeferrred
switch c.TargetName {
case TargetDirect:
switch cluster.Spec.GetCloudProvider() {
Expand Down Expand Up @@ -764,6 +765,9 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error {
// Can cause conflicts with terraform management
shouldPrecreateDNS = false

// Terraform tracks & performs deletions itself
deletionProcessingMode = fi.DeletionProcessingModeIgnore

case TargetDryRun:
var out io.Writer = os.Stdout
if c.GetAssets {
Expand All @@ -786,7 +790,7 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error {
}
}

context, err := fi.NewCloudupContext(ctx, target, cluster, cloud, keyStore, secretStore, configBase, c.TaskMap)
context, err := fi.NewCloudupContext(ctx, deletionProcessingMode, target, cluster, cloud, keyStore, secretStore, configBase, c.TaskMap)
if err != nil {
return fmt.Errorf("error building context: %v", err)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func runTasks(t *testing.T, cloud awsup.AWSCloud, allTasks map[string]fi.Cloudup
Cloud: cloud,
}

context, err := fi.NewCloudupContext(ctx, target, nil, cloud, nil, nil, nil, allTasks)
context, err := fi.NewCloudupContext(ctx, fi.DeletionProcessingModeDeleteIncludingDeferred, target, nil, cloud, nil, nil, nil, allTasks)
if err != nil {
t.Fatalf("error building context: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion upup/pkg/fi/cloudup/awstasks/elastic_ip_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func checkNoChanges(t *testing.T, ctx context.Context, cloud fi.Cloud, allTasks
}
assetBuilder := assets.NewAssetBuilder(vfs.Context, cluster.Spec.Assets, cluster.Spec.KubernetesVersion, false)
target := fi.NewCloudupDryRunTarget(assetBuilder, os.Stderr)
context, err := fi.NewCloudupContext(ctx, target, nil, cloud, nil, nil, nil, allTasks)
context, err := fi.NewCloudupContext(ctx, fi.DeletionProcessingModeDeleteIncludingDeferred, target, nil, cloud, nil, nil, nil, allTasks)
if err != nil {
t.Fatalf("error building context: %v", err)
}
Expand Down
4 changes: 0 additions & 4 deletions upup/pkg/fi/cloudup/awsup/aws_apitarget.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,6 @@ func NewAWSAPITarget(cloud AWSCloud) *AWSAPITarget {
}
}

func (t *AWSAPITarget) ProcessDeletions() bool {
return true
}

func (t *AWSAPITarget) DefaultCheckExisting() bool {
return true
}
Expand Down
5 changes: 0 additions & 5 deletions upup/pkg/fi/cloudup/azure/azure_apitarget.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,6 @@ func (t *AzureAPITarget) Finish(taskMap map[string]fi.CloudupTask) error {
return nil
}

// ProcessDeletions returns true if we should delete resources.
func (t *AzureAPITarget) ProcessDeletions() bool {
return true
}

func (t *AzureAPITarget) DefaultCheckExisting() bool {
return true
}
4 changes: 0 additions & 4 deletions upup/pkg/fi/cloudup/do/api_target.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ func (t *DOAPITarget) Finish(taskMap map[string]fi.CloudupTask) error {
return nil
}

func (t *DOAPITarget) ProcessDeletions() bool {
return true
}

func (t *DOAPITarget) DefaultCheckExisting() bool {
return true
}
4 changes: 0 additions & 4 deletions upup/pkg/fi/cloudup/gce/gce_apitarget.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ func (t *GCEAPITarget) Finish(taskMap map[string]fi.CloudupTask) error {
return nil
}

func (t *GCEAPITarget) ProcessDeletions() bool {
return true
}

func (t *GCEAPITarget) DefaultCheckExisting() bool {
return true
}
4 changes: 2 additions & 2 deletions upup/pkg/fi/cloudup/gcetasks/serviceaccount_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func checkHasChanges(t *testing.T, ctx context.Context, cloud fi.Cloud, allTasks
func runTasks(t *testing.T, ctx context.Context, cloud gce.GCECloud, allTasks map[string]fi.CloudupTask) {
target := gce.NewGCEAPITarget(cloud)

context, err := fi.NewCloudupContext(ctx, target, nil, cloud, nil, nil, nil, allTasks)
context, err := fi.NewCloudupContext(ctx, fi.DeletionProcessingModeDeleteIncludingDeferred, target, nil, cloud, nil, nil, nil, allTasks)
if err != nil {
t.Fatalf("error building context: %v", err)
}
Expand All @@ -120,7 +120,7 @@ func doDryRun(t *testing.T, ctx context.Context, cloud fi.Cloud, allTasks map[st
}
assetBuilder := assets.NewAssetBuilder(vfs.Context, cluster.Spec.Assets, cluster.Spec.KubernetesVersion, false)
target := fi.NewCloudupDryRunTarget(assetBuilder, os.Stderr)
context, err := fi.NewCloudupContext(ctx, target, nil, cloud, nil, nil, nil, allTasks)
context, err := fi.NewCloudupContext(ctx, fi.DeletionProcessingModeDeleteIncludingDeferred, target, nil, cloud, nil, nil, nil, allTasks)
if err != nil {
t.Fatalf("error building context: %v", err)
}
Expand Down
4 changes: 0 additions & 4 deletions upup/pkg/fi/cloudup/hetzner/api_target.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ func (t *HetznerAPITarget) Finish(taskMap map[string]fi.CloudupTask) error {
return nil
}

func (t *HetznerAPITarget) ProcessDeletions() bool {
return true
}

func (t *HetznerAPITarget) DefaultCheckExisting() bool {
return true
}
4 changes: 0 additions & 4 deletions upup/pkg/fi/cloudup/openstack/apitarget.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ func (t *OpenstackAPITarget) Finish(taskMap map[string]fi.CloudupTask) error {
return nil
}

func (t *OpenstackAPITarget) ProcessDeletions() bool {
return true
}

func (t *OpenstackAPITarget) DefaultCheckExisting() bool {
return true
}
4 changes: 0 additions & 4 deletions upup/pkg/fi/cloudup/scaleway/api_target.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ func (s ScwAPITarget) Finish(taskMap map[string]fi.CloudupTask) error {
return nil
}

func (s ScwAPITarget) ProcessDeletions() bool {
return true
}

func (t *ScwAPITarget) DefaultCheckExisting() bool {
return true
}
5 changes: 0 additions & 5 deletions upup/pkg/fi/cloudup/terraform/target.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,6 @@ func (t *TerraformTarget) AddFileResource(resourceType string, resourceName stri
return t.AddFileBytes(resourceType, resourceName, key, d, base64)
}

func (t *TerraformTarget) ProcessDeletions() bool {
// Terraform tracks & performs deletions itself
return false
}

func (t *TerraformTarget) DefaultCheckExisting() bool {
return false
}
Expand Down
21 changes: 16 additions & 5 deletions upup/pkg/fi/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ type Context[T SubContext] struct {
tasks map[string]Task[T]
warnings []*Warning[T]

deletionProcessingMode DeletionProcessingMode

T T
}

Expand Down Expand Up @@ -74,39 +76,48 @@ type Warning[T SubContext] struct {
Message string
}

func newContext[T SubContext](ctx context.Context, target Target[T], sub T, tasks map[string]Task[T]) (*Context[T], error) {
func newContext[T SubContext](ctx context.Context, deletionProcessingMode DeletionProcessingMode, target Target[T], sub T, tasks map[string]Task[T]) (*Context[T], error) {
c := &Context[T]{
ctx: ctx,
Target: target,
tasks: tasks,
T: sub,

deletionProcessingMode: deletionProcessingMode,
}

return c, nil
}

func NewInstallContext(ctx context.Context, target InstallTarget, tasks map[string]InstallTask) (*InstallContext, error) {
// We don't expect deletions, but we would be the one to handle them.
deletionProcessingMode := DeletionProcessingModeDeleteIncludingDeferred

sub := InstallSubContext{}
return newContext[InstallSubContext](ctx, target, sub, tasks)
return newContext[InstallSubContext](ctx, deletionProcessingMode, target, sub, tasks)
}

func NewNodeupContext(ctx context.Context, target NodeupTarget, keystore KeystoreReader, bootConfig *nodeup.BootConfig, nodeupConfig *nodeup.Config, tasks map[string]NodeupTask) (*NodeupContext, error) {
// We don't expect deletions, but we would be the one to handle them.
deletionProcessingMode := DeletionProcessingModeDeleteIncludingDeferred

sub := NodeupSubContext{
BootConfig: bootConfig,
NodeupConfig: nodeupConfig,
Keystore: keystore,
}
return newContext[NodeupSubContext](ctx, target, sub, tasks)
return newContext[NodeupSubContext](ctx, deletionProcessingMode, target, sub, tasks)
}

func NewCloudupContext(ctx context.Context, target CloudupTarget, cluster *kops.Cluster, cloud Cloud, keystore Keystore, secretStore SecretStore, clusterConfigBase vfs.Path, tasks map[string]CloudupTask) (*CloudupContext, error) {
func NewCloudupContext(ctx context.Context, deletionProcessingMode DeletionProcessingMode, target CloudupTarget, cluster *kops.Cluster, cloud Cloud, keystore Keystore, secretStore SecretStore, clusterConfigBase vfs.Path, tasks map[string]CloudupTask) (*CloudupContext, error) {
sub := CloudupSubContext{
Cloud: cloud,
Cluster: cluster,
ClusterConfigBase: clusterConfigBase,
Keystore: keystore,
SecretStore: secretStore,
}
return newContext[CloudupSubContext](ctx, target, sub, tasks)
return newContext[CloudupSubContext](ctx, deletionProcessingMode, target, sub, tasks)
}

func (c *Context[T]) AllTasks() map[string]Task[T] {
Expand Down
18 changes: 8 additions & 10 deletions upup/pkg/fi/default_methods.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,22 +104,20 @@ func defaultDeltaRunMethod[T SubContext](e Task[T], c *Context[T]) error {
}
}

if producesDeletions, ok := e.(ProducesDeletions[T]); ok && c.Target.ProcessDeletions() {
var deletions []Deletion[T]
deletions, err = producesDeletions.FindDeletions(c)
if producesDeletions, ok := e.(ProducesDeletions[T]); ok && c.deletionProcessingMode != DeletionProcessingModeIgnore {
deletions, err := producesDeletions.FindDeletions(c)
if err != nil {
return err
}
for _, deletion := range deletions {
if _, ok := c.Target.(*DryRunTarget[T]); ok {
err = c.Target.(*DryRunTarget[T]).Delete(deletion)
} else if _, ok := c.Target.(*DryRunTarget[T]); ok {
err = c.Target.(*DryRunTarget[T]).Delete(deletion)
if err := c.Target.(*DryRunTarget[T]).RecordDeletion(deletion); err != nil {
return err
}
} else {
err = deletion.Delete(c.Target)
}
if err != nil {
return err
if err := deletion.Delete(c.Target); err != nil {
return err
}
}
}
}
Expand Down
12 changes: 12 additions & 0 deletions upup/pkg/fi/deletions.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,18 @@ limitations under the License.

package fi

type DeletionProcessingMode string

const (
// DeletionProcessingModeIgnore will ignore all deletion tasks.
DeletionProcessingModeIgnore DeletionProcessingMode = "Ignore"
// TODO: implement deferred-deletion in the tasks!
// DeletionProcessingModeDeleteIfNotDeferrred will delete resources only if they are not marked for deferred-deletion.
DeletionProcessingModeDeleteIfNotDeferrred DeletionProcessingMode = "IfNotDeferred"
// DeletionProcessingModeDeleteIncludingDeferrred will delete resources including those marked for deferred-deletion.
DeletionProcessingModeDeleteIncludingDeferred DeletionProcessingMode = "DeleteIncludingDeferred"
)

type ProducesDeletions[T SubContext] interface {
FindDeletions(*Context[T]) ([]Deletion[T], error)
}
Expand Down
7 changes: 1 addition & 6 deletions upup/pkg/fi/dryrun_target.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,6 @@ func NewNodeupDryRunTarget(assetBuilder *assets.AssetBuilder, out io.Writer) *No
return newDryRunTarget[NodeupSubContext](assetBuilder, out)
}

func (t *DryRunTarget[T]) ProcessDeletions() bool {
// We display deletions
return true
}

func (t *DryRunTarget[T]) DefaultCheckExisting() bool {
return true
}
Expand All @@ -117,7 +112,7 @@ func (t *DryRunTarget[T]) Render(a, e, changes Task[T]) error {
return nil
}

func (t *DryRunTarget[T]) Delete(deletion Deletion[T]) error {
func (t *DryRunTarget[T]) RecordDeletion(deletion Deletion[T]) error {
t.mutex.Lock()
defer t.mutex.Unlock()

Expand Down
5 changes: 0 additions & 5 deletions upup/pkg/fi/nodeup/install/install_target.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,6 @@ func (t *InstallTarget) Finish(taskMap map[string]fi.InstallTask) error {
return nil
}

func (t *InstallTarget) ProcessDeletions() bool {
// We don't expect any, but it would be our job to process them
return true
}

func (t *InstallTarget) DefaultCheckExisting() bool {
return true
}
Expand Down
5 changes: 0 additions & 5 deletions upup/pkg/fi/nodeup/local/local_target.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,6 @@ func (t *LocalTarget) Finish(taskMap map[string]fi.NodeupTask) error {
return nil
}

func (t *LocalTarget) ProcessDeletions() bool {
// We don't expect any, but it would be our job to process them
return true
}

func (t *LocalTarget) DefaultCheckExisting() bool {
return true
}
Expand Down
4 changes: 0 additions & 4 deletions upup/pkg/fi/target.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@ type Target[T SubContext] interface {
// Lifecycle methods, called by the driver
Finish(taskMap map[string]Task[T]) error

// ProcessDeletions returns true if we should delete resources
// Some providers (e.g. Terraform) actively keep state, and will delete resources automatically
ProcessDeletions() bool

// DefaultCheckExisting returns true if DefaultDeltaRun tasks which aren't HasCheckExisting
// should invoke Find() when running against this Target.
DefaultCheckExisting() bool
Expand Down
Loading