-
Notifications
You must be signed in to change notification settings - Fork 182
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 rook to latest master build. #1141
Update rook to latest master build. #1141
Conversation
rook update pulls in an update of the controller-runtime dependency from 0.6.3 to 0.8.0. |
73dd135
to
60fe1aa
Compare
Weeding out the build errors one by one...
|
Remaining error:
|
Locally, I am getting this error now:
|
@@ -176,10 +176,11 @@ func (r *StorageClusterReconciler) newExternalCephObjectStoreInstances( | |||
return nil, err | |||
} | |||
// enable bucket healthcheck | |||
time60s, _ := time.ParseDuration("60s") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to parse time anymore, just use 1 * time.Minute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed (yet to push). Actually time.Minute
is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did all the CRDs get deleted in c428b8b?
Right that is weird... Investigating. |
updated with the current state |
We are making progress. Now rook's image is fixed upstream via rook/rook#7512 and the image target starts to pass. Still some things to fix from the controller-runtime update... |
e209b68
to
2576c25
Compare
@jarrpa - I incorporated your patches on top of my PR and rebased. Looks a lot better! 😄 Only waiting for the e2e tests to complete now - everything else passed! |
From the test output:
Not sure if this is a flake or real. FWIW, I see |
We're using OCP 4.7. |
/retest |
Okay, the e2e failures may indeed be a flake. It seems it failed to even install the OCS CSV. Looking around, I found that the CatalogSource they generate to hold the CSV was in TRANSIENT_FAILURE:
So, as is often the case, we are probably dealing with network issues and/or API limits. /test ocs-operator-bundle-e2e-aws |
/retest |
26e6275
to
d8ef033
Compare
Created PR #1144 for only the CSV part of the rook upgrade. |
e3d0638
to
52a1e92
Compare
@raghavendra-talur yay, your patch fixes it! |
When working on polishing / squashing the patches, I came across the test code that @jarrpa commented out. Removing the comments lets the unit tests fail this way:
Not sure really how to fix this. But ideally, we should be commenting out such lines... |
@jarrpa , @raghavendra-talur , @umangachapagain - any idea? |
No idea. I wasn't able to figure anything further out, hence why I had to leave it like that just to make progress. It has something to do with how the fake reconciler is implemented, but that is still fairly opaque to me. |
@jarrpa - do you suggest to move forward with the commented-out lines? (It just feels odd, since the lines seemed to have been there for a purpose... ;-) |
For the sake of expediency, yes. There should at least me a TODO to figure out why they were suddenly a problem. |
Latest rook master image. Signed-off-by: Michael Adam <obnox@redhat.com>
Signed-off-by: Michael Adam <obnox@redhat.com>
Latest master version with crds (re)addition to image. Picking up (among other things) crd updates, including volumereplication support. This is committing the result of: `go get github.com/rook/rook@1b77b9e` Signed-off-by: Michael Adam <obnox@redhat.com>
Signed-off-by: Michael Adam <obnox@redhat.com>
Why is there a diff now?... Signed-off-by: Michael Adam <obnox@redhat.com>
Quite some signatures have changed. Most importantly: - reconciler.Reconcile()now requries a context argument - fix use of interval to new Duration type - UpdateEvent has changed - NewFakeClientWithScheme is deprecated, using NewClientBuilder instead. Signed-off-by: Michael Adam <obnox@redhat.com>
Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>
Prevent us from creating a second operatorgroup in the namsepace. Co-authored-by: Raghavendra Talur <raghavendra.talur@gmail.com> Signed-off-by: Michael Adam <obnox@redhat.com>
The controller runtime now uses leases for its leader election mechanism by default. It looks like the RBAC configuration in kubebuilder does not add this permission by default though. We add it manually here. Signed-off-by: Raghavendra Talur <raghavendra.talur@gmail.com>
updated to the latest and greatest rook image rook/ceph:v1.6.0.95.gf4cfc7a still the only weird thing is the commented out test code lines... I added a TODO comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jarrpa 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:
Approvers can indicate their approval by writing |
/override ci/prow/ci-index-dev-master-dependencies |
@jarrpa: Overrode contexts on behalf of jarrpa: ci/prow/ci-index-dev-master-dependencies In response to this:
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. |
For 4.8 preparation, we need a newer rook image.