OCPBUGS-32091: Add Top-level Context for Create Commands

[patrickdillon]

Adds a top-level context when running create commands. The context is passed through the asset store and then to any PostRun functions. The main motivation is for a clean shutdown of CAPI controllers, but there are a wide variety of potential use cases.

This PR uses the pattern introduced in #6009 to enable the introduction of a GenerateWithContext function for assets and an adapter to drop the context and call the original Generate function. Currently only the Cluster asset implements GenerateWithContext.

Currently the PR achieves the primary goal of shutting down capi controllers on interrupt. There are some remaining issues I would like to clear up:

• on interrupt, the error message simply says context cancelled. Would be better for the error message to indicate a user interrupt
• Need to test that the logrus exit handler is properly shutting down the controllers on error (not just interrupt)
• the interrupt signal handler is being short circuited by the call to logrus.Exit, this means the controllers are not "gracefully shutting down", but on the other hand the processes are being properly killed. It's unclear to me whether we need any special handling now that we have the top-level context.

[openshift-ci-robot]

@patrickdillon: This pull request references CORS-3241 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Adds a top-level context when running create commands. The context is passed through the asset store and then to any PostRun functions. The main motivation is for a clean shutdown of CAPI controllers, but there are a wide variety of potential use cases.

This PR uses the pattern introduced in #6009 to enable the introduction of a GenerateWithContext function for assets and an adapter to drop the context and call the original Generate function. Currently only the Cluster asset implements GenerateWithContext.

Currently the PR achieves the primary goal of shutting down capi controllers on interrupt. There are some remaining issues I would like to clear up:

• on interrupt, the error message simply says context cancelled. Would be better for the error message to indicate a user interrupt
• Need to test that the logrus exit handler is properly shutting down the controllers on error (not just interrupt)
• the interrupt signal handler is being short circuited by the call to logrus.Exit, this means the controllers are not "gracefully shutting down", but on the other hand the processes are being properly killed. It's unclear to me whether we need any special handling now that we have the top-level context.

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 openshift-eng/jira-lifecycle-plugin repository.

[patrickdillon]

/cc @andfasano @vincepri

[barbacbd]

Did this sneak into this PR? This doesn't seem directly related to the work on contexts.

[patrickdillon]

I could move to a different commit or PR, but it's somewhat related with the focus of a clean shutdown of the capi controllers.

[patrickdillon]

/assign @vincepri

Even though we are using this context when starting the controllers

installer/pkg/clusterapi/internal/process/process.go

Line 131 in 6b4523d

ps.Cmd = exec.CommandContext(ctx, ps.Path, ps.Args...) //nolint:gosec

We still need to handle the CTRL+C user interrupt, because the context will not be done/cancelled with an interrupt, right?

How can we ensure that the CAPI system Teardown() function has enough time to complete? In the current implementation, the Teardown function begins, but when the context is cancelled an error is returned which ends up calling logrus.Exit before Teardown reports it is done:

installer/cmd/openshift-install/create.go

Lines 313 to 323 in 6b4523d

	if err != nil {
	if strings.Contains(err.Error(), asset.InstallConfigError) {
	logrus.Error(err)
	logrus.Exit(exitCodeInstallConfigError)
	}
	if strings.Contains(err.Error(), asset.ClusterCreationError) {
	logrus.Error(err)
	logrus.Exit(exitCodeInfrastructureFailed)
	}
	logrus.Fatal(err)
	}

This is similar to what you handled in #7693 and #7864 but I still don't see how to make it work here.

[vincepri]

@patrickdillon In this case we'd have to create a new context, as a parent of the one that we use from signals. We can then wait for ctx.Done() before exiting and calling the Teardown function

[bfournie]

/cc @bfournie

[openshift-bot]

@patrickdillon: This pull request references CORS-3241 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Adds a top-level context when running create commands. The context is passed through the asset store and then to any PostRun functions. The main motivation is for a clean shutdown of CAPI controllers, but there are a wide variety of potential use cases.

This PR uses the pattern introduced in #6009 to enable the introduction of a GenerateWithContext function for assets and an adapter to drop the context and call the original Generate function. Currently only the Cluster asset implements GenerateWithContext.

Currently the PR achieves the primary goal of shutting down capi controllers on interrupt. There are some remaining issues I would like to clear up:

• on interrupt, the error message simply says context cancelled. Would be better for the error message to indicate a user interrupt
• Need to test that the logrus exit handler is properly shutting down the controllers on error (not just interrupt)
• the interrupt signal handler is being short circuited by the call to logrus.Exit, this means the controllers are not "gracefully shutting down", but on the other hand the processes are being properly killed. It's unclear to me whether we need any special handling now that we have the top-level context.

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 openshift-eng/jira-lifecycle-plugin repository.

[r4f4]

Tried this with a ctrl-c right after capi started:

^CWARNING Received interrupt signal
ERROR failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed to run cluster api system: failed to run controller "aws infrastructure provider": failed to prepare controller "aws infrastructure provider" webhook options: the server was unable to return a response in the time allotted, but may still be processing the request (get mutatingwebhookconfigurations.admissionregistration.k8s.io capa-mutating-webhook-configuration)
INFO Shutting down local Cluster API control plane...
INFO Local Cluster API system has completed operations

[patrickdillon]

Tried this with a ctrl-c right after capi started:

^CWARNING Received interrupt signal
ERROR failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed to run cluster api system: failed to run controller "aws infrastructure provider": failed to prepare controller "aws infrastructure provider" webhook options: the server was unable to return a response in the time allotted, but may still be processing the request (get mutatingwebhookconfigurations.admissionregistration.k8s.io capa-mutating-webhook-configuration)
INFO Shutting down local Cluster API control plane...
INFO Local Cluster API system has completed operations

Yup that is the expected result. The returned error will vary depending on where/why exit.

In terms of UX, I wonder if we should move these to debug level but I think that is mostly a nit.

> INFO Shutting down local Cluster API control plane...
> INFO Local Cluster API system has completed operations

[openshift-ci-robot]

@patrickdillon: This pull request references Jira Issue OCPBUGS-32091, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.16.0) matches configured target version for branch (4.16.0)
• bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @gpei

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Adds a top-level context when running create commands. The context is passed through the asset store and then to any PostRun functions. The main motivation is for a clean shutdown of CAPI controllers, but there are a wide variety of potential use cases.

This PR uses the pattern introduced in #6009 to enable the introduction of a GenerateWithContext function for assets and an adapter to drop the context and call the original Generate function. Currently only the Cluster asset implements GenerateWithContext.

Currently the PR achieves the primary goal of shutting down capi controllers on interrupt. There are some remaining issues I would like to clear up:

• on interrupt, the error message simply says context cancelled. Would be better for the error message to indicate a user interrupt
• Need to test that the logrus exit handler is properly shutting down the controllers on error (not just interrupt)
• the interrupt signal handler is being short circuited by the call to logrus.Exit, this means the controllers are not "gracefully shutting down", but on the other hand the processes are being properly killed. It's unclear to me whether we need any special handling now that we have the top-level context.

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 openshift-eng/jira-lifecycle-plugin repository.

[vincepri]

/lgtm

[r4f4]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: r4f4, vincepri

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 [r4f4]

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

[openshift-ci]

@patrickdillon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-e2e-agent-compact-ipv4	401fccb	link	false	/test okd-e2e-agent-compact-ipv4
ci/prow/altinfra-e2e-aws-ovn-localzones	0398c63	link	false	/test altinfra-e2e-aws-ovn-localzones
ci/prow/altinfra-e2e-aws-ovn-shared-vpc-edge-zones	0398c63	link	false	/test altinfra-e2e-aws-ovn-shared-vpc-edge-zones
ci/prow/altinfra-e2e-aws-ovn-single-node	0398c63	link	false	/test altinfra-e2e-aws-ovn-single-node
ci/prow/altinfra-e2e-aws-custom-security-groups	0398c63	link	false	/test altinfra-e2e-aws-custom-security-groups
ci/prow/altinfra-e2e-aws-ovn-fips	0398c63	link	false	/test altinfra-e2e-aws-ovn-fips
ci/prow/altinfra-e2e-aws-ovn-wavelengthzones	0398c63	link	false	/test altinfra-e2e-aws-ovn-wavelengthzones
ci/prow/okd-e2e-aws-ovn-upgrade	a132f85	link	false	/test okd-e2e-aws-ovn-upgrade

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.

[openshift-ci-robot]

@patrickdillon: Jira Issue OCPBUGS-32091: All pull requests linked via external trackers have merged:

• openshift/installer#8063

Jira Issue OCPBUGS-32091 has been moved to the MODIFIED state.

In response to this:

Adds a top-level context when running create commands. The context is passed through the asset store and then to any PostRun functions. The main motivation is for a clean shutdown of CAPI controllers, but there are a wide variety of potential use cases.

This PR uses the pattern introduced in #6009 to enable the introduction of a GenerateWithContext function for assets and an adapter to drop the context and call the original Generate function. Currently only the Cluster asset implements GenerateWithContext.

Currently the PR achieves the primary goal of shutting down capi controllers on interrupt. There are some remaining issues I would like to clear up:

• on interrupt, the error message simply says context cancelled. Would be better for the error message to indicate a user interrupt
• Need to test that the logrus exit handler is properly shutting down the controllers on error (not just interrupt)
• the interrupt signal handler is being short circuited by the call to logrus.Exit, this means the controllers are not "gracefully shutting down", but on the other hand the processes are being properly killed. It's unclear to me whether we need any special handling now that we have the top-level context.

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-bot]

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-installer-altinfra-container-v4.16.0-202404121144.p0.gd8c7872.assembly.stream.el8 for distgit ose-installer-altinfra.
All builds following this will include this PR.