Skip to content

Comments

CORS-2978: enable AWS SDK Installation through feature gates#7715

Merged
openshift-merge-bot[bot] merged 3 commits intoopenshift:masterfrom
patrickdillon:aws-sdk-fg
Dec 1, 2023
Merged

CORS-2978: enable AWS SDK Installation through feature gates#7715
openshift-merge-bot[bot] merged 3 commits intoopenshift:masterfrom
patrickdillon:aws-sdk-fg

Conversation

@patrickdillon
Copy link
Contributor

@patrickdillon patrickdillon commented Nov 13, 2023

Plumbs feature gates through to the infrastructure provider package to enable exposing alternate infrastructure providers through feature gates. Particularly enables the AWS sdk infrastructure provider.

Also includes a fixup of the machine manifests due to the API bump.

This PR plumbs the featuregate into the cluster metadata so that it can be used with the bootstrap destroy code. I know this can be problematic for hive (cc @2uasimojo) when we depend on updates to cluster metadata for destroy code, because cluster metadata is not always present. The functionality added here is only being used in bootstrap destroy code, so I don't think this will negatively affect hive. I think it would be a good idea to remove the feature gate field from the metadata once we have finished implementing the alternate infrastructure providers.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 13, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 13, 2023

@patrickdillon: This pull request references CORS-2829 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 epic to target the "4.15.0" version, but no target version was set.

Details

In response to this:

Plumbs feature gates through to the infrastructure provider package to enable exposing alternate infrastructure providers through feature gates. Particularly enables the AWS sdk infrastructure provider.

Also includes a fixup of the machine manifests due to the API bump.

Depends on #7676

This PR plumbs the featuregate into the cluster metadata so that it can be used with the bootstrap destroy code. I know this can be problematic for hive (cc @2uasimojo) when we depend on updates to cluster metadata for destroy code, because cluster metadata is not always present. The functionality added here is only being used in bootstrap destroy code, so I don't think this will negatively affect hive. I think it would be a good idea to remove the feature gate field from the metadata once we have finished implementing the alternate infrastructure providers.

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.

@patrickdillon
Copy link
Contributor Author

/hold
as 6fa2f552780ee97f6d6a29d0c10b71a18eeb12c0 depends on #7676

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2023
@openshift-ci openshift-ci bot requested review from jhixson74 and mdbooth November 13, 2023 16:19
@patrickdillon
Copy link
Contributor Author

/hold cancel
We can add feature gate directly in #7676 or subsequently.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2023
@r4f4
Copy link
Contributor

r4f4 commented Nov 13, 2023

Don't forget to update the platform_altinfra.go file as well, otherwise you get the following error when building with the altinfra tag:

# github.com/openshift/installer/pkg/asset/cluster
pkg/asset/cluster/cluster.go:112:55: too many arguments in call to infra.ProviderForPlatform
	have (string, featuregates.FeatureGate)
	want (string)

@patrickdillon
Copy link
Contributor Author

Don't forget to update the platform_altinfra.go file as well, otherwise you get the following error when building with the altinfra tag:

# github.com/openshift/installer/pkg/asset/cluster
pkg/asset/cluster/cluster.go:112:55: too many arguments in call to infra.ProviderForPlatform
	have (string, featuregates.FeatureGate)
	want (string)

Just pushed a fix

@r4f4
Copy link
Contributor

r4f4 commented Nov 15, 2023

I think it's just missing a go mod tidy now.

@patrickdillon
Copy link
Contributor Author

I think it's just missing a go mod tidy now.

oh thanks. pushed a fix.

@2uasimojo
Copy link
Member

@patrickdillon

The functionality added here is only being used in bootstrap destroy code

Meaning it's only used when destroying the bootstrap node during day 0 installation? And not at all in the destroy flow? If that's the case, then you are correct, there should be no hive impact.

@patrickdillon
Copy link
Contributor Author

@patrickdillon

The functionality added here is only being used in bootstrap destroy code

Meaning it's only used when destroying the bootstrap node during day 0 installation? And not at all in the destroy flow? If that's the case, then you are correct, there should be no hive impact.

exactly

@patrickdillon
Copy link
Contributor Author

/hold
serializing featuregates to metadata is not working

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2023
@patrickdillon
Copy link
Contributor Author

/hold cancel

Fixed the metadata serialization by serializing the feature sets and then reconstructing the feature gate in the destroy code.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 16, 2023

@patrickdillon: This pull request references CORS-2829 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 epic to target the "4.15.0" version, but no target version was set.

Details

In response to this:

Plumbs feature gates through to the infrastructure provider package to enable exposing alternate infrastructure providers through feature gates. Particularly enables the AWS sdk infrastructure provider.

Also includes a fixup of the machine manifests due to the API bump.

This PR plumbs the featuregate into the cluster metadata so that it can be used with the bootstrap destroy code. I know this can be problematic for hive (cc @2uasimojo) when we depend on updates to cluster metadata for destroy code, because cluster metadata is not always present. The functionality added here is only being used in bootstrap destroy code, so I don't think this will negatively affect hive. I think it would be a good idea to remove the feature gate field from the metadata once we have finished implementing the alternate infrastructure providers.

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.

@patrickdillon
Copy link
Contributor Author

Depends on openshift/api#1668
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2023
@patrickdillon patrickdillon force-pushed the aws-sdk-fg branch 2 times, most recently from 5491fd0 to 5bc2f10 Compare November 20, 2023 14:45
@patrickdillon
Copy link
Contributor Author

/hold cancel
ready for review

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2023
@patrickdillon
Copy link
Contributor Author

/hold
Actually it looks like pulling in openshift/api@master will break openstack-manifests. I am headed out for Thanksgiving vacation soon, so I have dropped the api vendor commits. Hopefully those will get pulled in by another PR and then this one can merged subsequently (or someone should feel free to take this over in another PR while I'm out for the week)

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2023
@r4f4
Copy link
Contributor

r4f4 commented Nov 22, 2023

/hold cancel
openstack-manifests fix has merged.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2023
@r4f4
Copy link
Contributor

r4f4 commented Nov 22, 2023

/retest-required

@r4f4
Copy link
Contributor

r4f4 commented Nov 23, 2023

/hold
Needs a bump in the config operator first.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 23, 2023
@patrickdillon
Copy link
Contributor Author

/skip
/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 26, 2023

@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/altinfra-e2e-aws-custom-security-groups 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test altinfra-e2e-aws-custom-security-groups
ci/prow/e2e-aws-ovn-shared-vpc 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-aws-ovn-shared-vpc
ci/prow/e2e-aws-ovn-imdsv2 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-aws-ovn-imdsv2
ci/prow/e2e-azure-ovn-shared-vpc 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-azure-ovn-shared-vpc
ci/prow/e2e-gcp-ovn-shared-vpc 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-gcp-ovn-shared-vpc
ci/prow/e2e-openstack-proxy 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-openstack-proxy
ci/prow/e2e-aws-ovn-localzones 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-aws-ovn-localzones
ci/prow/altinfra-e2e-aws-ovn-fips 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test altinfra-e2e-aws-ovn-fips
ci/prow/e2e-gcp-secureboot 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-gcp-secureboot
ci/prow/e2e-aws-ovn-single-node 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-aws-ovn-single-node
ci/prow/e2e-openstack-sdn-parallel 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-openstack-sdn-parallel
ci/prow/e2e-azurestack 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-azurestack
ci/prow/e2e-gcp-ovn-xpn 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-gcp-ovn-xpn
ci/prow/e2e-aws-ovn-fips 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-aws-ovn-fips
ci/prow/e2e-aws-custom-security-groups 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-aws-custom-security-groups
ci/prow/altinfra-e2e-aws-ovn-single-node 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test altinfra-e2e-aws-ovn-single-node
ci/prow/e2e-aws-ovn-shared-vpc-localzones 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-aws-ovn-shared-vpc-localzones
ci/prow/e2e-openstack-nfv-intel 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link false /test e2e-openstack-nfv-intel
ci/prow/e2e-openstack-ovn 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link true /test e2e-openstack-ovn
ci/prow/openstack-manifests 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link true /test openstack-manifests
ci/prow/e2e-gcp-ovn 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link true /test e2e-gcp-ovn
ci/prow/e2e-azure-ovn 9721aae548258c2cc1187bf57b94e93b1a17f0e2 link true /test e2e-azure-ovn

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

@patrickdillon
Copy link
Contributor Author

patrickdillon commented Nov 27, 2023

/hold Needs a bump in the config operator first.

Depends on openshift/cluster-config-operator#384

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2023
Includes the feature gates for the cluster. These feature gates can
be used for testing bootstrap destroy functionality.

Including the feature gates and using them in this manner should not
create any issues. Particularly this should not affect hive, as long
as this metadata field is not used in the cluster destroy code (as
opposed to the bootstrap destroy code it is being used with here).

Regardless, it seems like the need for carrying featuregates in the
cluster metadata will be short lived while we move to new infrastructure
providers. Once these infrastructure providers are considered default,
it would be good to consider removing the feature gate in order to
keep cluster metadata lightweight.
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 28, 2023
Plumbs through feature gates to the infrastructure provider selector
switch. Allows for enabling alternate infrastructure providers through
feature gates.
Enables using the SDK alternative infrastructure provider for AWS
with a feature gate.
@patrickdillon
Copy link
Contributor Author

/hold Needs a bump in the config operator first.

Depends on openshift/cluster-config-operator#384

/hold cancel
This doesn't actually depend on the CCO PR. The API has already been vendored in for this gate: https://github.com/openshift/installer/blob/master/vendor/github.com/openshift/api/config/v1/feature_gates.go#L335

In fact, it is probably better to merge this before openshift/cluster-config-operator#384, so that PR can be adequately tested.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2023
Copy link
Contributor

@r4f4 r4f4 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2023
@patrickdillon
Copy link
Contributor Author

/retest

@patrickdillon
Copy link
Contributor Author

/retest-required

1 similar comment
@patrickdillon
Copy link
Contributor Author

/retest-required

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 153837a and 2 for PR HEAD dff6f97 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 88ec5bc and 1 for PR HEAD dff6f97 in total

@openshift-merge-bot openshift-merge-bot bot merged commit 065acd5 into openshift:master Dec 1, 2023
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-installer-altinfra-container-v4.15.0-202312010731.p0.g065acd5.assembly.stream for distgit ose-installer-altinfra.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants