OCPBUGS-34975: aws: terraform: add spot instance support for masters

[r4f4]

Spot instances can result on savings for short-lived clusters. If the control plane machine manifests have been edited with spot instance information, enable the use of spot instances in the terraform config. Notice that max price information is ignored, since it's not advised for it to be set.

[r4f4]

Depends on openshift/release#51664 for testing spot instances deployments.

/hold

[2uasimojo]

Implements (part of) RFE-5545

[r4f4]

/retitle CORS-3523: aws: terraform: add spot instance support for masters

[openshift-ci-robot]

@r4f4: This pull request references CORS-3523 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.17.0" version, but no target version was set.

Details

In response to this:

Spot instances can result on savings for short-lived clusters. If the control plane machine manifests have been edited with spot instance information, enable the use of spot instances in the terraform config. Notice that max price information is ignored, since it's not advised for it to be set.

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]

/retitle CORS-3524, CORS-3523: aws: terraform: add spot instance support for masters

[2uasimojo]

/lgtm

[patrickdillon]

I suppose grep works fine here, and perhaps we don't need to over engineer but would it be more convenient to structure this output, say in json or something?

[r4f4]

@patrickdillon what about a --json argument for that?

[r4f4]

Done

[2uasimojo]

Cool; though the current use case still requires:

• parsing the output with something (be it grep or jq or [[ "$(...)" == *'"terraform-spot-masters"'* ]])
• accounting for what happens when invoking this on an older installer binary (stdout is empty, RC is nonzero)

...so YAGNI for now 🤷

The only thing I can think of that would make it easier to consume would be something like openshift-install is-hidden-feature-supported terraform-spot-masters which exits zero if yes, nonzero if no. That way it can reliably be consumed as:

if ! openshift-install is-hidden-feature-supported terraform-spot-masters >/dev/null 2>&1; then
    ...
fi

...because I'll get a nonzero RC on older installers as well as newer ones that have this code but disclaim the support.

[2uasimojo]

to be clear: I'm not asking for ⬆️, just thinking aloud :)

[r4f4]

/hold cancel
The way we're implementing this, openshift/release#51664 and this PR can merge in any order.

[r4f4]

/override ci/prow/okd-images
Known issue

[openshift-ci]

@r4f4: Overrode contexts on behalf of r4f4: ci/prow/okd-images

Details

In response to this:

/override ci/prow/okd-images
Known issue

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-sigs/prow repository.

[openshift-ci]

@r4f4: 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-aws-ovn-upgrade	8ec547f	link	false	/test okd-e2e-aws-ovn-upgrade
ci/prow/e2e-aws-custom-security-groups	8ec547f	link	false	/test e2e-aws-custom-security-groups

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

[2uasimojo]

/lgtm

[2uasimojo]

/lgtm cancel

Discussion still open around the hidden subcommand.

[patrickdillon]

/lgtm cancel

Discussion still open around the hidden subcommand.

Yes, I am hesitant to add a new command to the installer for such a specific use. And I'm not certain that the problem requires a new command. I think it would help to have more context for the motivation. Reading through the slack thread I identified the motivation as:

My goal was to put in a check so that, if you (accidentally or out of ignorance) tried to do this (tf + spot masters) on a .z without your PR, it would puke.

I am not sure if this is intended to be used in CI or for QE running manual tests or both. I would imagine in both of these cases we're pretty much only testing the latest z streams anyway, so my impression is that we're solving a transient problem with code that will live on much longer.

Can we layout in more detail the problem we're tackling with the command?

[2uasimojo]

Yes, I am hesitant to add a new command to the installer for such a specific use.

Ack, and likewise.

And I'm not certain that the problem requires a new command.

I would love for us to come up with some other solution.

Can we layout in more detail the problem we're tackling with the command?

If I pass an install-config with controlPlane spotMarketOptions into an installer with either CAPI featuregate flaggage or the terraform changes from this PR, it is honored.
If I pass an install-config with controlPlane spotMarketOptions into any other installer currently in existence, it is silently ignored.
Unchecked, this can easily result in a scenario where I think I'm saving money by enabling spot masters, but I'm actually not.

I am not sure if this is intended to be used in CI or for QE running manual tests or both.

Yes, the primary use case is for CI jobs, where we have different personas responsible for the install-config vs the job config. It is via env vars to the latter that spot instance config (for masters and/or workers) is injected by tooling into the former. It is for this use case that we need protection, again because if I provide that env var but don't actually get spot masters:

• I haven't saved $$ as I thought I should.
• If something goes wrong, things could get confusing as I triage on the assumption that I'm using spot masters.

I imagine QE would find this similarly useful if they have scripting around generating install-configs. Not so much if they're doing everything by hand.

I would imagine in both of these cases we're pretty much only testing the latest z streams anyway, so my impression is that we're solving a transient problem with code that will live on much longer.

This is a sane point. If we intend to backport this change to all living y-streams, and CI/QE exclusively tests only the latest (nightlies and) z-streams thereon, I agree the window of usefulness is smallish. Are those assumptions valid? In particular, do we really not run CI on old-but-still-supported z-streams?

[r4f4]

/lgtm cancel

Discussion still open around the hidden subcommand.

Yes, I am hesitant to add a new command to the installer for such a specific use. And I'm not certain that the problem requires a new command. I think it would help to have more context for the motivation. Reading through the slack thread I identified the motivation as:

My goal was to put in a check so that, if you (accidentally or out of ignorance) tried to do this (tf + spot masters) on a .z without your PR, it would puke.

I am not sure if this is intended to be used in CI or for QE running manual tests or both. I would imagine in both of these cases we're pretty much only testing the latest z streams anyway, so my impression is that we're solving a transient problem with code that will live on much longer.

Can we layout in more detail the problem we're tackling with the command?

That's not totally true because we have to modify the IPI steps which are shared among all ocp versions. So we need a why to identify which versions of the installer support the spot instances feature (the other option is that older installers will silently ignore the spot feature, even though the master manifests were changed. That's what we were trying to avoid).

[patrickdillon]

@r4f4 @2uasimojo what about solving these problems with tests? In ci run a post-install test that checks the envvar and if set ensures spot instances are running. Share this tactic with qe

[patrickdillon]

@r4f4 @2uasimojo what about solving these problems with tests? In ci run a post-install test that checks the envvar and if set ensures spot instances are running. Share this tactic with qe

sorry was in a rush to share this idea but I realize I did it in an entirely unconvincing and simple way. i'm not sure how large the surface area in ci is for failing this test, but if it is large, the test would need to be optional (not sure if this is possible) to be informative (this would give us an idea of how to expand coverage), if it is massive perhaps this idea would not work. this is perhaps less of a silent fail than the proposed solution is?

[r4f4]

@r4f4 @2uasimojo what about solving these problems with tests? In ci run a post-install test that checks the envvar and if set ensures spot instances are running. Share this tactic with qe

sorry was in a rush to share this idea but I realize I did it in an entirely unconvincing and simple way. i'm not sure how large the surface area in ci is for failing this test, but if it is large, the test would need to be optional (not sure if this is possible) to be informative (this would give us an idea of how to expand coverage), if it is massive perhaps this idea would not work. this is perhaps less of a silent fail than the proposed solution is?

Remember that backports in the installer take way longer than we wish they would. So I think failing tests is out of question because we'd fail too many. Besides, a mechanism for detecting available features would be useful for other ci-only functionality like public-only subnets and BYOIPv4 (which is active for certain regions but we are silently ignoring until we add support in the capi path).

It doesn't need to be a hidden command but I think it'll be very useful to have something.

[2uasimojo]

solving these problems with tests? In ci run a post-install test that checks the envvar and if set ensures spot instances are running.

Interesting idea. Digging in...

IIUC this would look like:
Include a post-install step in the IPI flow where we're adding support for spot masters.
The post-install step would, if SPOT_MASTERS is set, inspect the masters on the created cluster and make sure they're spots, failing the setup if they're not.
To be useful, this step would need to always be part of the ipi setup flow.
It would thus affect, what, hundreds? of CI jobs.
...which, if misconfigured, would be creating entire clusters -- with expensive on-demand instances -- and quickly destroying them (without running the meat of whatever broader test this is part of) because they're misconfigured.
And these test would take ~40m to fail, vs "immediately".
I suppose we would catch this misconfig for a given job at pj-rehearse time... assuming we ran a pj-rehearse for every job we try to switch this on for, which doesn't sound like it would be reasonable for the scale we're talking about.
Also, anecdotally, the rate at which job failures are ignored in OSCI... I'm not even sure we would catch or react to this with any efficiency.

In summary: IMHO making the failure fast and cheap is moar better than making it slow and expensive.

[patrickdillon]

solving these problems with tests? In ci run a post-install test that checks the envvar and if set ensures spot instances are running.

Interesting idea. Digging in...

IIUC this would look like: Include a post-install step in the IPI flow where we're adding support for spot masters.

write/run a test--not add a step. I haven't written such a test before so I don't know how it's done offhand (would need to nail this down to be serious). the test would fail if the job config sets the envvar for spot instances but the resulting cluster isn't running (on) them. Ideally would be able to use something like ci search or sippy to identify failing jobs.

But I agree that it is a much slower feedback loop. This discussion helped me understand how we're going to use this to roll out the spot instances.

/approve

@2uasimojo feel free to re-lgtm we are probably going to need to backport this pretty far I assume

[patrickdillon]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [patrickdillon]

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

[2uasimojo]

Awesome, let’s do it

/lgtm

[2uasimojo]

/cherry-pick release-4.15 release-4.14 release-4.13 release-4.12

[openshift-cherrypick-robot]

@2uasimojo: #8349 failed to apply on top of branch "release-4.15":

Applying: aws: terraform: add spot instance support for masters
Using index info to reconstruct a base tree...
M	data/data/aws/bootstrap/main.tf
M	data/data/aws/cluster/main.tf
M	data/data/aws/variables-aws.tf
Falling back to patching base and 3-way merge...
Auto-merging data/data/aws/variables-aws.tf
CONFLICT (content): Merge conflict in data/data/aws/variables-aws.tf
Auto-merging data/data/aws/cluster/main.tf
Auto-merging data/data/aws/bootstrap/main.tf
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 aws: terraform: add spot instance support for masters
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-4.15 release-4.14 release-4.13 release-4.12

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-sigs/prow repository.

[openshift-bot]

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-installer-altinfra-container-v4.17.0-202405312141.p0.ge7a8199.assembly.stream.el9 for distgit ose-installer-altinfra.
All builds following this will include this PR.

[r4f4]

/cherry-pick release-4.16

[openshift-cherrypick-robot]

@r4f4: new pull request created: #8526

Details

In response to this:

/cherry-pick release-4.16

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-sigs/prow repository.

[r4f4]

/retitle OCPBUGS-34975: aws: terraform: add spot instance support for masters

[openshift-ci-robot]

@r4f4: Jira Issue OCPBUGS-34975: All pull requests linked via external trackers have merged:

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

Details

In response to this:

Spot instances can result on savings for short-lived clusters. If the control plane machine manifests have been edited with spot instance information, enable the use of spot instances in the terraform config. Notice that max price information is ignored, since it's not advised for it to be set.

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.