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

Update crossplane-runtime to 0b469fcc77cd #782

Merged
merged 1 commit into from
Jul 29, 2021

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Jul 28, 2021

Signed-off-by: Alper Rifat Ulucinar ulucinar@users.noreply.github.com

Description of your changes

A recently merged PR that depends on the new managed.ExternalObservation.Diff struct field, which is not yet available on any crossplane-runtime releases has broken the master. After discussing the issue with @muvaf, we have decided to update the crossplane-runtime dependency to the current master HEAD.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Successfully provisioned CloudFront Distribution & CachePolicy on a branch containing this PR's branch.

@ulucinar ulucinar requested a review from muvaf July 28, 2021 15:39
k8s.io/api v0.21.2
k8s.io/apimachinery v0.21.2
k8s.io/client-go v0.21.2
sigs.k8s.io/controller-runtime v0.9.2
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that this is a minor version bump for pre-1.0 controller-runtime, so we might want to double check for any breaking changes :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @hasheddan,
That seems to be change coming from controller-runtime (v0.14.0 has controller-runtime v0.8.0 and master depends on v0.9.2). Would it be enough run provider-aws operator and try a MR? How do you generally verify these changes?

Copy link
Member

Choose a reason for hiding this comment

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

I think most breaking changes at that level are usually compile errors, which we can verify with make reviewable and make build and integration test we have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gave this PR a try by successfully provisioning/editing/deleting a MR (CloudFront Distribution and CachePolicy).

@ulucinar
Copy link
Collaborator Author

ulucinar commented Jul 28, 2021

It looks like we cannot use go v1.14.15 with the new controller-runtime version:

github.com/crossplane/provider-aws/apis/acm/v1alpha1 imports
        sigs.k8s.io/controller-runtime/pkg/client tested by
        sigs.k8s.io/controller-runtime/pkg/client.test imports
        sigs.k8s.io/controller-runtime/pkg/envtest imports
        sigs.k8s.io/controller-runtime/pkg/internal/testing/addr imports
        io/fs: package io/fs is not in GOROOT (/opt/go.1.14.15/src/io/fs)

Should we update go version also, or would rolling back of the PR be a better option @hasheddan, @muvaf ?

@ulucinar ulucinar changed the title Update crossplane-runtime to 0b469fcc77 Update crossplane-runtime to 0b469fcc77cd Jul 28, 2021
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

@ulucinar I think bumping go version sounds fine, the primary place it needs to happen is here: https://github.com/crossplane/provider-aws/blob/f8b42a22bf07b9ad4eb0de9e1799ea1754dda29f/.github/workflows/ci.yml#L13

@ulucinar ulucinar force-pushed the aru/fix-runtime-dep branch 2 times, most recently from d7e169a to aee7093 Compare July 28, 2021 16:24
@ulucinar
Copy link
Collaborator Author

@ulucinar I think bumping go version sounds fine, the primary place it needs to happen is here:

https://github.com/crossplane/provider-aws/blob/f8b42a22bf07b9ad4eb0de9e1799ea1754dda29f/.github/workflows/ci.yml#L13

Thanks @hasheddan!

@alecrajeev
Copy link
Contributor

alecrajeev commented Jul 28, 2021

Sorry about this. I will make it more clear when I have a change that is dependent on crossplane-runtime being updated going forward.

@ulucinar
Copy link
Collaborator Author

@alecrajeev no problem at all. Let's get this merged.

@ulucinar
Copy link
Collaborator Author

ulucinar commented Jul 28, 2021

@hasheddan, is it possible to retry the e2e-tests job only? I did not see an option on GitHub UI to retrigger job.

I think currently it is not possible:
https://git.luolix.topmunity/t/ability-to-rerun-just-a-single-job-in-a-workflow/17234

I was suspecting I was missing the necessary privileges :)

@ulucinar ulucinar force-pushed the aru/fix-runtime-dep branch 3 times, most recently from c899d1a to 840601a Compare July 28, 2021 17:16
@ulucinar
Copy link
Collaborator Author

ulucinar commented Jul 28, 2021

e2e tests are failing on my workstation also with similar symptoms:

 make e2e
17:49:01 [ .. ] installing kind v0.9.0
17:49:02 [ OK ] installing kind v0.9.0
17:49:02 [ .. ] installing kubectl v1.17.11
17:49:02 [ OK ] installing kubectl v1.17.11
17:49:02 [ .. ] installing helm3 v2.16.7
17:49:03 [ OK ] installing helm3 v2.16.7
17:49:03 [ .. ] running integration tests using kind v0.9.0

>>>>>>> setting up local package cache
created cache dir at /home/alper/data/workspaces/github.com/ulucinar/crossplane/provider-aws/.work/inttest-package-cache

>>>>>>> creating k8s cluster using kind
Creating cluster "build-81fe2fd6-inttests" ...
 ✓ Ensuring node image (kindest/node:v1.19.1) 🖼
 ✓ Preparing nodes 📦
 ✓ Writing configuration 📜
 ✓ Starting control-plane 🕹️
 ✓ Installing CNI 🔌
 ✓ Installing StorageClass 💾
 ✗ Waiting ≤ 5m0s for control-plane = Ready ⏳
 • WARNING: Timed out waiting for Ready ⚠️
Set kubectl context to "kind-build-81fe2fd6-inttests"
You can now use your cluster with:

kubectl cluster-info --context kind-build-81fe2fd6-inttests

Thanks for using kind! 😊
Image: "crossplane/provider-aws-controller:v0.18.0-rc.0.194.g840601a4" with ID "sha256:fa7ec785e5431380e4141315ff346e9455bcd31759dc62c00f64f81f547ee205" not yet present on node "build-81fe2fd6-inttests-control-plane", loading...

>>>>>>> pre-cache package by copying to kind node

>>>>>>> create crossplane-system namespace
namespace/crossplane-system created

>>>>>>> create persistent volume and claim for mounting package-cache
persistentvolume/package-cache created
persistentvolumeclaim/package-cache created

>>>>>>> installing crossplane from master channel
"crossplane-master" has been added to your repositories

using crossplane version 1.4.0-rc.0.54.g8e76b9cd
Error: timed out waiting for the condition

>>>>>>> Cleaning up...
Deleting cluster "build-81fe2fd6-inttests" ...
18:00:19 [FAIL]
make[1]: *** [Makefile:92: test-integration] Error 1
make: *** [build/makelib/common.mk:377: e2e] Error 2```

This seemed to be related to kind on my workstation. Could be a similar issue with the e2e-tests job. 

@ulucinar
Copy link
Collaborator Author

Will try with a new kind image...

@hasheddan
Copy link
Member

@ulucinar this looks like latest Crossplane is failing to install might be worth doing a watch on the pods while tests are running.. if you run make e2e.run skipcleanup=true the cluster will stick around after the tests fail

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar
Copy link
Collaborator Author

ulucinar commented Jul 28, 2021

@ulucinar this looks like latest Crossplane is failing to install might be worth doing a watch on the pods while tests are running.. if you run make e2e.run skipcleanup=true the cluster will stick around after the tests fail

Hi @hasheddan,
It turns out that we have hit this issue, which is related to a recent kernel change.

I had observed a similar issue when preparing a demo cluster for the hack week. As indicated here, I have retried with a new kind version and node image and it seems to be working.

Thank you very much for your help!

@muvaf muvaf merged commit 47c56fd into crossplane-contrib:master Jul 29, 2021
@ulucinar ulucinar deleted the aru/fix-runtime-dep branch July 29, 2021 08:30
ulucinar added a commit to ulucinar/provider-azure that referenced this pull request Aug 19, 2021
- Make kind version used in e2e tests configurable
- Update kind version to 0.11.1
- Update kind node image version to 1.19.11
- These are needed to get e2e tests running. Please see:
  crossplane-contrib/provider-aws#782 (comment)

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
ulucinar added a commit to ulucinar/provider-azure that referenced this pull request Aug 19, 2021
- Make kind version used in e2e tests configurable
- Update kind version to 0.11.1
- Update kind node image version to 1.19.11
- These are needed to get e2e tests running. Please see:
  crossplane-contrib/provider-aws#782 (comment)

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
ulucinar added a commit to ulucinar/provider-azure that referenced this pull request Aug 19, 2021
- Make kind version used in e2e tests configurable
- Update kind version to 0.11.1
- Update kind node image version to 1.19.11
- These are needed to get e2e tests running. Please see:
  crossplane-contrib/provider-aws#782 (comment)

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
turkenh added a commit to turkenh/provider-sql that referenced this pull request Aug 25, 2021
Details: crossplane-contrib/provider-aws#782 (comment)

Signed-off-by: Hasan Turken <turkenh@gmail.com>
turkenh added a commit to turkenh/provider-helm that referenced this pull request Aug 25, 2021
Details: crossplane-contrib/provider-aws#782 (comment)

Signed-off-by: Hasan Turken <turkenh@gmail.com>
turkenh added a commit to turkenh/provider-helm that referenced this pull request Aug 25, 2021
Details: crossplane-contrib/provider-aws#782 (comment)

Signed-off-by: Hasan Turken <turkenh@gmail.com>
turkenh added a commit to turkenh/provider-sql that referenced this pull request Aug 27, 2021
Details: crossplane-contrib/provider-aws#782 (comment)

Signed-off-by: Hasan Turken <turkenh@gmail.com>
(cherry picked from commit 038e3b4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants