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.

Sign up for GitHub

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

Jump to bottom

Bumping k8s to 1.20.2 and controller-runtime to 0.8.3 #692

Merged
merged 1 commit into from
Jul 26, 2021
Merged

Bumping k8s to 1.20.2 and controller-runtime to 0.8.3 #692

merged 1 commit into from
Jul 26, 2021

Conversation

liranmauda
Copy link
Contributor

@liranmauda liranmauda commented Jul 21, 2021 •
edited
Loading

Explain the changes:

  • Bumping k8s to 1.20.2

  • Bumping controller-runtime to 0.8.3

  • Bumping github.com/openshift/cloud-credential-operator

  • event changes:

    • MetaOld to ObjectOld
    • MetaNew to ObjectNew
    • Meta to Object

  • Changed runtime.Object to client.Object (and in case of list to client.ObjectList)

  • Updated the EnqueueRequestsFromMapFunc according to the new changes

  • Add context where needed

  • Added util.SecretResetStringDataFromData after Calling controllerutil.CreateOrUpdate in system reconcile

  • Removed the Panic from func ObjectKey as client.ObjectKeyFromObject does not return error anymore

Update according to those Breaking changes:

  1. ⚠️ Use controllerutil.Object for event handling kubernetes-sigs/controller-runtime#1118
  2. kubernetes-sigs/controller-runtime@d61f561#diff-5a927c01385831c16ce8c52c9b3ac094d9e830c2719ca5431d3bc500daca47fe
  3. kubernetes-sigs/controller-runtime@f35d811

Related:

#662

Fixes:

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1892335
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1892337

@liranmauda
Copy link
Contributor Author

Please ignore the number of commits, LMLM in the code, and that it is not signed.
I will wrap it out when we approve it.

Also please pay attention to the fact that I had to add util.SecretResetStringDataFromData in several places, for some reason after the bump of controller-runtime, not all the places that we looked at StringData were populated.

@liranmauda liranmauda force-pushed the liran-bump-k8s branch 4 times, most recently from 81d99fe to cbbee38 Compare July 22, 2021 09:30
@liranmauda liranmauda marked this pull request as ready for review July 22, 2021 09:31
@dannyzaken
Copy link
Member

@liranmauda I think we need to understand why util.SecretResetStringDataFromData is not working inside KubeCheck. having to call it outside of KubeCheck is very bug-prone

@liranmauda
Copy link
Contributor Author

Never said it is not working.
The timing has changed, so sometimes we try to reach StringData and it is empty, or in different cases, the map was not even initialized yet.

@liranmauda
Copy link
Contributor Author

After @dannyzaken and I looked, we saw that StringData is getting zeroed after Calling controllerutil.CreateOrUpdate

dannyzaken
dannyzaken approved these changes Jul 26, 2021
View reviewed changes
pkg/backingstore/reconciler.go
@@ -967,6 +967,9 @@ func (r *Reconciler) ReconcilePool() error {
}

func (r *Reconciler) reconcilePvPool() error {
if r.Secret.StringData == nil {
Copy link
Member

Choose a reason for hiding this comment

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

is it needed now, after you added SecretResetStringDataFromData to ReconcileObject?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested it and yes, It is still needed.

pkg/system/phase4_configuring.go Outdated Show resolved Hide resolved
@liranmauda
Bumping k8s to 1.20.2 and controller-runtime to 0.8.3
01e5716 
- Bumping k8s to 1.20.2
- Bumping controller-runtime to 0.8.3
- Bumping github.com/openshift/cloud-credential-operator

Signed-off-by: liranmauda <[email protected]>
@liranmauda liranmauda merged commit 16200b3 into noobaa:master Jul 26, 2021
@liranmauda liranmauda mentioned this pull request Jul 26, 2021
Closed
@liranmauda liranmauda deleted the liran-bump-k8s branch July 26, 2021 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannyzaken dannyzaken dannyzaken approved these changes

@guymguym guymguym Awaiting requested review from guymguym

@nimrod-becker nimrod-becker Awaiting requested review from nimrod-becker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@liranmauda @dannyzaken