HOSTEDCP-1051: addition of grace period for aws infra destruction #2967

Patryk-Stefanski · 2023-08-31T11:50:09Z

What this PR does / why we need it:

Adds InfraGracePeriod timeout flag for deleting AWS infrastructure resources so that if something goes wrong with deleting the infrastructure resources, the deletion process is not stuck.

Which issue(s) this PR fixes
Fixes #

HOSTEDCP-1051

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

muraee · 2023-08-31T14:51:53Z

cmd/infra/aws/destroy.go

@@ -77,9 +80,14 @@ func NewDestroyCommand() *cobra.Command {
 	return cmd
 }

-func (o *DestroyInfraOptions) Run(ctx context.Context) error {
+func (o *DestroyInfraOptions) Run(ctx context.Context, timeout time.Duration) error {


why do you need to pass the timeout as an argument here?
you can just get it from the DestroyInfraOptions struct as o.AwsInfraGracePeriod

I don't, good spot.

netlify · 2023-09-04T11:24:18Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`fbfec64`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/64f5be3add53b6000895145f
😎 Deploy Preview	https://deploy-preview-2967--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Patryk-Stefanski · 2023-09-05T10:37:34Z

/retest-required

muraee · 2023-09-05T15:43:45Z

cmd/infra/aws/destroy.go

@@ -78,8 +81,13 @@ func NewDestroyCommand() *cobra.Command {
 }

 func (o *DestroyInfraOptions) Run(ctx context.Context) error {
+	destroyInfraCtx, destroyInfraCtxCancel := context.WithTimeout(ctx, o.AwsInfraGracePeriod)


you probably need to check if AwsInfraGracePeriod is set here and use a default if not set before using it.

Setting the default value in the args/flags is not enough as this code could be called from somewhere else like we do in our e2e here https://github.com/openshift/hypershift/blob/main/test/e2e/util/fixture.go#L81-L87

Patryk-Stefanski · 2023-09-06T08:42:37Z

/retest-required

jparrill · 2023-09-12T10:02:28Z

cmd/infra/aws/destroy.go

+	destroyInfraCtx, destroyInfraCtxCancel := context.WithTimeout(ctx, o.AwsInfraGracePeriod)
+	defer destroyInfraCtxCancel()
+
+	o.Log.Info(fmt.Sprintf("waiting %d s", int(o.AwsInfraGracePeriod.Seconds())))


Carefully with the log entries, we usually set most of the things on conditions to avoid the "too much verbosity" issue. Maybe make sense to explore to update the reason of InfrastructureReady condition saying something like Deprovisioning... Grace Period remaining X or something like that (IMHO). WDYT @muraee ?

yeah makes sense. Will I remove it for now and if needs be I can look into reporting it in the condition?

Logs and conditions are signal for different problem spaces. Conditions are contractual signal for consumers subject to API support policies. Logs are non contractual UX for humans.
External clients like the cli should no often manipulate status for resources owned by controllers. Also at the time of deleting the infra, all the CRs should be gone anyways.
Agree on wanting to avoid unnecessary logs, but as far as I cans see this line would only log once which is fine. I'd consider even adding a line with the remaining timeout along the existing o.Log.Info("WARNING: error during destroy, will retry", "error", err.Error()).

I changed the log to be more descriptive so it now looks like Infra destruction timeout set to 5 s. I think when someone sets a timeout and the timeout duration set is reported back to him should be enough don't think there's a need for a countdown.

What happens if the timeout is met? I'd expect we return an err and communicate deletion is moving ahead without infra deletion succeeding.

Atm it exits out with an error and doesn’t proceed with the destruction. Don’t think implementing what you explained above would be too much work however I think it would lead to confusion as there would be resources left over after a successful destruction. I think it makes more sense to leave it as is but I’m open to discussion.

cmd/cluster/aws/destroy.go

cmd/infra/aws/destroy.go

enxebre · 2023-09-25T13:52:23Z

cmd/infra/aws/destroy.go

@@ -60,6 +61,7 @@ func NewDestroyCommand() *cobra.Command {
 	cmd.Flags().StringVar(&opts.Name, "name", opts.Name, "A name for the cluster")
 	cmd.Flags().StringVar(&opts.BaseDomain, "base-domain", opts.BaseDomain, "The ingress base domain for the cluster")
 	cmd.Flags().StringVar(&opts.BaseDomainPrefix, "base-domain-prefix", opts.BaseDomainPrefix, "The ingress base domain prefix for the cluster, defaults to cluster name. se 'none' for an empty prefix")
+	cmd.Flags().DurationVar(&opts.AwsInfraGracePeriod, "aws-infra-grace-period", opts.AwsInfraGracePeriod, "Timeout for destroying infrastructure in minutes")


can we comment include what default is in the flag description?

flag is optional and there is no default value

enxebre · 2023-09-25T13:54:07Z

Thanks! can you please squash commits and update the message to follow conventions in https://hypershift-docs.netlify.app/contribute/?

bryan-cox · 2023-10-03T12:22:30Z

cmd/cluster/aws/destroy.go

@@ -32,6 +31,7 @@ func NewDestroyCommand(opts *core.DestroyOptions) *cobra.Command {
 	cmd.Flags().StringVar(&opts.AWSPlatform.BaseDomain, "base-domain", opts.AWSPlatform.BaseDomain, "Cluster's base domain; inferred from the hosted cluster by default")
 	cmd.Flags().StringVar(&opts.AWSPlatform.BaseDomainPrefix, "base-domain-prefix", opts.AWSPlatform.BaseDomainPrefix, "Cluster's base domain prefix; inferred from the hosted cluster by default")
 	cmd.Flags().StringVar(&opts.CredentialSecretName, "secret-creds", opts.CredentialSecretName, "A Kubernetes secret with a platform credential, pull-secret and base-domain. The secret must exist in the supplied \"--namespace\"")
+	cmd.Flags().DurationVar(&opts.AWSPlatform.AwsInfraGracePeriod, "aws-infra-grace-period", opts.AWSPlatform.AwsInfraGracePeriod, "Timeout for destroying infrastructure in minutes")


Should this be added to the product CLI?

yep, addressed in latest commit

Patryk-Stefanski · 2023-10-10T12:21:22Z

/retest-required

sjenning · 2023-10-10T19:54:03Z

/lgtm
/approve

openshift-ci · 2023-10-10T19:54:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Patryk-Stefanski, sjenning

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~~ [sjenning]

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

openshift-ci-robot · 2023-10-10T21:29:46Z

/retest-required

Remaining retests: 0 against base HEAD 111b136 and 2 for PR HEAD 4d47f04 in total

openshift-ci · 2023-10-11T00:30:55Z

@Patryk-Stefanski: 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.

celebdor · 2023-10-17T08:59:10Z

/retitle HOSTEDCP-1051: addition of grace period for aws infra destruction

openshift-ci bot added the do-not-merge/needs-area label Aug 31, 2023

openshift-ci bot requested review from hasueki and sjenning August 31, 2023 11:51

openshift-ci bot added area/cli Indicates the PR includes changes for CLI and removed do-not-merge/needs-area labels Aug 31, 2023

muraee reviewed Aug 31, 2023

View reviewed changes

Patryk-Stefanski force-pushed the HOSTEDCP-1051 branch 2 times, most recently from 06b9821 to fbfec64 Compare September 4, 2023 11:23

Patryk-Stefanski changed the title ~~Draft: HOSTEDCP-1051 addition of grace period for aws infra destruction~~ HOSTEDCP-1051 addition of grace period for aws infra destruction Sep 5, 2023

muraee reviewed Sep 5, 2023

View reviewed changes

jparrill reviewed Sep 12, 2023

View reviewed changes

enxebre reviewed Sep 12, 2023

View reviewed changes

cmd/cluster/aws/destroy.go Outdated Show resolved Hide resolved

enxebre reviewed Sep 12, 2023

View reviewed changes

cmd/infra/aws/destroy.go Outdated Show resolved Hide resolved

enxebre reviewed Sep 12, 2023

View reviewed changes

cmd/infra/aws/destroy.go Outdated Show resolved Hide resolved

enxebre reviewed Sep 25, 2023

View reviewed changes

HOSTEDCP-1051: Addition of grace period for aws infra destruction

7283d58

Patryk-Stefanski force-pushed the HOSTEDCP-1051 branch from 4a34b10 to 7283d58 Compare September 25, 2023 15:56

bryan-cox reviewed Oct 3, 2023

View reviewed changes

HOSTEDCP-1051 add infra grace period to product-cli

4d47f04

openshift-ci bot assigned sjenning Oct 10, 2023

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

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

openshift-ci bot merged commit 8f5fd6d into openshift:main Oct 11, 2023
12 checks passed

openshift-ci bot changed the title ~~HOSTEDCP-1051 addition of grace period for aws infra destruction~~ HOSTEDCP-1051: addition of grace period for aws infra destruction Oct 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HOSTEDCP-1051: addition of grace period for aws infra destruction #2967

HOSTEDCP-1051: addition of grace period for aws infra destruction #2967

Patryk-Stefanski commented Aug 31, 2023 •

edited

muraee Aug 31, 2023

Patryk-Stefanski Aug 31, 2023

netlify bot commented Sep 4, 2023

Patryk-Stefanski commented Sep 5, 2023

muraee Sep 5, 2023

Patryk-Stefanski commented Sep 6, 2023

jparrill Sep 12, 2023

Patryk-Stefanski Sep 12, 2023

enxebre Sep 12, 2023

Patryk-Stefanski Sep 12, 2023

enxebre Sep 12, 2023

Patryk-Stefanski Sep 12, 2023

enxebre Sep 25, 2023

Patryk-Stefanski Sep 25, 2023

enxebre commented Sep 25, 2023

bryan-cox Oct 3, 2023

Patryk-Stefanski Oct 4, 2023

Patryk-Stefanski commented Oct 10, 2023

sjenning commented Oct 10, 2023

openshift-ci bot commented Oct 10, 2023

openshift-ci-robot commented Oct 10, 2023

openshift-ci bot commented Oct 11, 2023

celebdor commented Oct 17, 2023 •

edited by openshift-ci bot

HOSTEDCP-1051: addition of grace period for aws infra destruction #2967

HOSTEDCP-1051: addition of grace period for aws infra destruction #2967

Conversation

Patryk-Stefanski commented Aug 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Sep 4, 2023

✅ Deploy Preview for hypershift-docs ready!

Patryk-Stefanski commented Sep 5, 2023

Choose a reason for hiding this comment

Patryk-Stefanski commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patryk-Stefanski commented Oct 10, 2023

sjenning commented Oct 10, 2023

openshift-ci bot commented Oct 10, 2023

openshift-ci-robot commented Oct 10, 2023

openshift-ci bot commented Oct 11, 2023

celebdor commented Oct 17, 2023 • edited by openshift-ci bot

Patryk-Stefanski commented Aug 31, 2023 •

edited

celebdor commented Oct 17, 2023 •

edited by openshift-ci bot