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

Retrieve CAPI/CAPA from release image #1826

Merged
merged 2 commits into from
Nov 24, 2022

Conversation

bryan-cox
Copy link
Member

What this PR does / why we need it:
Retrieves the CAPI and CAPA image components from the release image rather than registry.ci.openshift.org/hypershift/cluster-api:v1.0.0 and registry.ci.openshift.org/hypershift/cluster-api-aws-controller:v1.1.0 respectively

Which issue(s) this PR fixes:
Fixes HOSTEDCP-577

Checklist

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

@openshift-ci openshift-ci bot requested review from csrwng and enxebre October 21, 2022 13:02
@bryan-cox bryan-cox force-pushed the HOSTEDCP-577 branch 2 times, most recently from 838bc4a to 29ffc5c Compare October 21, 2022 15:48
@netlify
Copy link

netlify bot commented Oct 25, 2022

Deploy Preview for hypershift-docs ready!

Name Link
🔨 Latest commit ee99719
🔍 Latest deploy log https://app.netlify.com/sites/hypershift-docs/deploys/637d07075eaa22000893b7c1
😎 Deploy Preview https://deploy-preview-1826--hypershift-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@bryan-cox bryan-cox force-pushed the HOSTEDCP-577 branch 4 times, most recently from 5a585ce to e3a7763 Compare October 26, 2022 12:42
@bryan-cox bryan-cox requested review from enxebre and removed request for csrwng October 26, 2022 14:08
@sjenning
Copy link
Contributor

This change is causing a huge number of GETs from the hypershift-operator

=== RUN   TestEtcdChaos/EnsureAPIBudget/hypershift-operator_read
    util.go:604: over budget: budget: 5000, actual: 100247

@bryan-cox
Copy link
Member Author

bryan-cox commented Oct 26, 2022

This change is causing a huge number of GETs from the hypershift-operator

=== RUN   TestEtcdChaos/EnsureAPIBudget/hypershift-operator_read
    util.go:604: over budget: budget: 5000, actual: 100247

@sjenning I think this might be due to the change I made in order to delete the AWS machine sets. I'll try reworking that and see if that resolves the problem.

@bryan-cox bryan-cox force-pushed the HOSTEDCP-577 branch 2 times, most recently from 38482fa to 33c6567 Compare October 27, 2022 17:00
@bryan-cox bryan-cox force-pushed the HOSTEDCP-577 branch 2 times, most recently from c369ec7 to a1bbfc9 Compare October 28, 2022 16:06
@enxebre
Copy link
Member

enxebre commented Oct 31, 2022

@bryan-cox let's move forward with and ship this:

This change seems to have come from kubernetes-sigs/cluster-api-provider-aws#2271. Specifically from the changes in machine.go in UseSecretsManager and the changes in awsmachine_controller.go in reconcileDelete.

Can you share logs from capa you are having when trying to delete the secret before the awsMachine?

Let's address #1826 (comment)
then should be good to go.

Orthogonally kubernetes-sigs/cluster-api-provider-aws#3805

@bryan-cox
Copy link
Member Author

@bryan-cox let's move forward with and ship this:

This change seems to have come from kubernetes-sigs/cluster-api-provider-aws#2271. Specifically from the changes in machine.go in UseSecretsManager and the changes in awsmachine_controller.go in reconcileDelete.

Can you share logs from capa you are having when trying to delete the secret before the awsMachine?

Let's address #1826 (comment) then should be good to go.

Orthogonally kubernetes-sigs/cluster-api-provider-aws#3805

Ok I think I've addressed #1826 (comment)

I included a snippet of the error log messages in here

@enxebre
Copy link
Member

enxebre commented Nov 17, 2022

/test e2e-aws

2 similar comments
@enxebre
Copy link
Member

enxebre commented Nov 18, 2022

/test e2e-aws

@enxebre
Copy link
Member

enxebre commented Nov 18, 2022

/test e2e-aws

@enxebre
Copy link
Member

enxebre commented Nov 18, 2022

This seems to be failing consistently @bryan-cox, did you have the chance to test it?
/test e2e-aws

@enxebre
Copy link
Member

enxebre commented Nov 18, 2022

/test e2e-aws

@bryan-cox bryan-cox force-pushed the HOSTEDCP-577 branch 2 times, most recently from 9768c2f to 61e9eda Compare November 22, 2022 00:43
@enxebre
Copy link
Member

enxebre commented Nov 22, 2022

/test e2e-aws

@enxebre
Copy link
Member

enxebre commented Nov 22, 2022

This PR keeps failing leaking resources, have you seen it working locally?
Can you please bump the timeout for the PR here

err := wait.PollImmediate(5*time.Second, 15*time.Minute, func() (bool, error) {

If that still fails then failure is legit.

@bryan-cox bryan-cox force-pushed the HOSTEDCP-577 branch 4 times, most recently from 381e8a0 to 0da1c49 Compare November 22, 2022 17:36
Retrieves the CAPI and CAPA image components from the release image rather than registry.ci.openshift.org/hypershift/cluster-api:v1.0.0 and registry.ci.openshift.org/hypershift/cluster-api-aws-controller:v1.1.0 respectively
@enxebre
Copy link
Member

enxebre commented Nov 23, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2022
@enxebre
Copy link
Member

enxebre commented Nov 23, 2022

/test e2e-aws

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD eae233b and 2 for PR HEAD 78fa5c3 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 0bc2e8d and 1 for PR HEAD 78fa5c3 in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 24, 2022

@bryan-cox: 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/e2e-ibmcloud-roks e3a7763 link false /test e2e-ibmcloud-roks
ci/prow/e2e-ibmcloud-iks e3a7763 link false /test e2e-ibmcloud-iks
ci/prow/periodics-4.13-images e3a7763 link true /test periodics-4.13-images
ci/prow/kubevirt-e2e-kubevirt-azure-ovn 78fa5c3 link false /test kubevirt-e2e-kubevirt-azure-ovn
ci/prow/kubevirt-e2e-kubevirt-gcp-ovn 78fa5c3 link false /test kubevirt-e2e-kubevirt-gcp-ovn
ci/prow/capi-provider-agent-sanity 78fa5c3 link false /test capi-provider-agent-sanity

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-merge-robot openshift-merge-robot merged commit 3262046 into openshift:main Nov 24, 2022
@bryan-cox bryan-cox deleted the HOSTEDCP-577 branch November 29, 2022 14:34
@eranco74
Copy link
Contributor

eranco74 commented Feb 8, 2023

The job that tests ocm-2.6 (ocp 4.11) is still trying to use registry.ci.openshift.org/hypershift/cluster-api:v1.0.0.
For some reason, the above image no longer exists (since Jan 25)
/cherry-pick release-4.11

@openshift-cherrypick-robot

@eranco74: #1826 failed to apply on top of branch "release-4.11":

Applying: Retrieve CAPI/CAPA from release image
Using index info to reconstruct a base tree...
M	hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
M	hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
M	hypershift-operator/controllers/hostedcluster/hostedcluster_webhook_test.go
M	hypershift-operator/controllers/hostedcluster/internal/platform/aws/aws.go
M	hypershift-operator/controllers/nodepool/nodepool_controller.go
M	support/releaseinfo/registryclient/client.go
M	test/e2e/util/fixture.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/util/fixture.go
CONFLICT (content): Merge conflict in test/e2e/util/fixture.go
Auto-merging support/releaseinfo/registryclient/client.go
CONFLICT (content): Merge conflict in support/releaseinfo/registryclient/client.go
Auto-merging hypershift-operator/controllers/nodepool/nodepool_controller.go
CONFLICT (content): Merge conflict in hypershift-operator/controllers/nodepool/nodepool_controller.go
Auto-merging hypershift-operator/controllers/hostedcluster/internal/platform/aws/aws.go
Auto-merging hypershift-operator/controllers/hostedcluster/hostedcluster_webhook_test.go
Auto-merging hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
Auto-merging hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
CONFLICT (content): Merge conflict in hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Retrieve CAPI/CAPA from release image
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".

In response to this:

The job that tests ocm-2.6 (ocp 4.11) is still trying to use registry.ci.openshift.org/hypershift/cluster-api:v1.0.0.
For some reason, the above image no longer exists (since Jan 25)
/cherry-pick release-4.11

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants