Georgettica/bump external snapshotter version (fixes #2966) #3202

georgettica · 2020-12-23T09:23:32Z

georgettica · 2020-12-23T09:36:26Z

yay! all checks passed :) now I need to find a way to test this did what is was supposed to..

suggestions?

go.mod

zubron

Thanks for looking at this! I think the replace directives will need to be updated. It looks like those directives were introduced to work around the fact that the CSI snapshotter library imported the Kubernetes libraries directly (see #2651). It may be the case that we can remove them with this change.

Either way, I think there will be more changes needed to this PR as v0.19.0 version isn't being used yet and once it is, it will require other libraries to be updated and changes in the Velero codebase.

go.mod

georgettica · 2021-01-04T23:13:34Z

After having a slack talk I have some bad news..

One of the dependant libraries we need I. Order to make this change are planning to upgrade in Feb

If someone has better insight on it I would gladly push our upgrade to a nearer target
https://kubernetes.slack.com/archives/C8TSNPY4T/p1609798576313300?thread_ts=1609798576.313300&cid=C8TSNPY4T

zubron · 2021-01-05T16:49:46Z

@georgettica Thanks so much for reaching out to the Cluster API folks and checking the status of their releases! 😄

Summarising the thread here: It looks like the release with this change will be their next minor release (v0.4.0) and that likely won't be available until end of Q1. Given that we just import it to use the patch helper they are considering making it available in controller-runtime but that will require refactoring to make it less specific to cluster API.

@vmware-tanzu/velero-maintainers: It seems our main options are to use an unreleased version of the library or hold off until the official release is available. We already use pre-release versions of other libraries so I think it should be fine to do the same here?

jenting · 2021-01-06T01:07:07Z

I'm 👍 to use the pre-release version.

georgettica · 2021-01-06T21:04:44Z

I'll look into changing the PR in accordance with your changes.

I am glad to reach to the community when I can XD

georgettica · 2021-01-07T08:52:05Z

so with the help of ya'll I moved it forward a bit more..

there is a build issue, caused by this change in controller-runtime
kubernetes-sigs/controller-runtime@8d45507

I am not sure what to change the function to, so I'll let the sages here look at it

jenting · 2021-01-13T07:49:35Z

so with the help of ya'll I moved it forward a bit more..

there is a build issue, caused by this change in controller-runtime
kubernetes-sigs/controller-runtime@8d45507

I am not sure what to change the function to, so I'll let the sages here look at it

@carlisia might know how to change the function to fulfill controller-runtime API change.
The build fails at https://github.com/vmware-tanzu/velero/blob/4048c02/internal/util/managercontroller/managercontroller.go#L32-L53

nrb · 2021-01-22T02:25:21Z

Based on https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/manager/manager.go#L259, it looks like the Runnable interface now takes a Context rather than a <-chan struct{} channel.

I made the following change locally to the server.go and managercontroller.go files and was able to compile.

I tried to push this commit to George's fork, but I don't have permissions. I've included it here as a patch file, and it can be applied with git am < <filename>, if you want to use it @georgettica.

Thank you tons for digging in to this, I appreciate it!

From e0056830f1eaef8362d5e382b10afe372bf070ba Mon Sep 17 00:00:00 2001
From: Nolan Brubaker <[email protected]>
Date: Thu, 21 Jan 2021 21:19:15 -0500
Subject: [PATCH] Use Context directly on the Runnable interface

New versions of controller-runtime use a Context directly rather than a
channel.

Signed-off-by: Nolan Brubaker <[email protected]>
---
 .../managercontroller/managercontroller.go    | 21 ++-----------------
 pkg/cmd/server/server.go                      |  2 +-
 2 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/internal/util/managercontroller/managercontroller.go b/internal/util/managercontroller/managercontroller.go
index 5a009d7f..be7391e2 100644
--- a/internal/util/managercontroller/managercontroller.go
+++ b/internal/util/managercontroller/managercontroller.go
@@ -29,25 +29,8 @@ import (
 // Runnable will turn a "regular" runnable component (such as a controller)
 // into a controller-runtime Runnable
 func Runnable(p controller.Interface, numWorkers int) manager.Runnable {
-	f := func(stop <-chan struct{}) error {
-
-		// Create a cancel context for handling the stop signal.
-		ctx, cancel := context.WithCancel(context.Background())
-		defer cancel()
-
-		// If a signal is received on the stop channel, cancel the
-		// context. This will propagate the cancel into the p.Run
-		// function below.
-		go func() {
-			select {
-			case <-stop:
-				cancel()
-			case <-ctx.Done():
-			}
-		}()
-
-		// This is a blocking call that either completes
-		// or is cancellable on receiving a stop signal.
+	// Pass the provided Context down to the run function.
+	f := func(ctx context.Context) error {
 		return p.Run(ctx, numWorkers)
 	}
 	return manager.RunnableFunc(f)
diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go
index a84c9a67..5cee3900 100644
--- a/pkg/cmd/server/server.go
+++ b/pkg/cmd/server/server.go
@@ -871,7 +871,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string
 
 	s.logger.Info("Server starting...")
 
-	if err := s.mgr.Start(s.ctx.Done()); err != nil {
+	if err := s.mgr.Start(s.ctx); err != nil {
 		s.logger.Fatal("Problem starting manager", err)
 	}
 
-- 
2.24.0

nrb · 2021-01-22T02:28:57Z

@georgettica Also, I see the DCO check is failing - https://github.com/vmware-tanzu/velero/pull/3202/checks?check_run_id=1661384030. Only the 3 latest commits need the signoff from what I can tell.

georgettica · 2021-01-22T21:42:54Z

Saw it now, and sad you didn't have permissions to push :(

Will look at it on Sunday and will for sure put these changes in.

Thanks!!

arianitu · 2021-01-25T21:35:22Z

Hey @georgettica , just checking on on this PR. It's blocking our builds since we use the velero code directly, any chance we can get this merged?

georgettica · 2021-01-25T21:40:28Z

@arianitu Not yet, as the build is broken.
But there is a fix to solve the next issue I had in line.

I am not working on this full time and will update when I get to a new breakthrough.

georgettica · 2021-01-26T10:00:18Z

all checks are now passing!
now I need to fix all of this mess I did with my commits and we can go forward

georgettica · 2021-01-26T10:01:23Z

tests are failing.. will see what is the matter there next

georgettica · 2021-01-26T10:30:12Z

after some git-fu, all the commits are now on uptream/main and I will fix the tests

georgettica · 2021-01-26T10:52:50Z

@zubron I think it works now.. right?

Signed-off-by: Ron Green <[email protected]>

now versions are working and there are code changes that need to happen - release candidate versions are aligned and working - replaces fields are removed and not required anymore controller runtime has been changed during the 'make' command Signed-off-by: Ron Green <[email protected]>

Signed-off-by: Nolan Brubaker <[email protected]> Signed-off-by: Ron Green <[email protected]>

- change to new api resource not all tests are passing, but most of them do Signed-off-by: Ron Green <[email protected]>

Signed-off-by: Ron Green <[email protected]>

georgettica · 2021-01-26T11:11:41Z

the lint check here is the issue I have now

also the CI check that fails on the test target

config/crd/bases/velero.io_restores.yaml

zubron

Thanks so much for your continued efforts on this, @georgettica!

This is looking great so far. There is only one further change needed which is to fix the controller tests. As the controller runtime manager now takes a context, we should update this test environment fixture to use a context and then pass that context into t.Manager.Start. I think it should be enough then to just mark that context as Done in the stop function.

I also suggested upgrading the Kubernetes libraries to the latest available patch version but if that turns out to be a lot of effort, you can leave them at the version you're currently using and we'll address the upgrade in #3340.

go.mod

zubron · 2021-01-26T15:42:31Z

pkg/controller/backup_storage_location_controller.go

@@ -52,7 +52,7 @@ type BackupStorageLocationReconciler struct {

 // +kubebuilder:rbac:groups=velero.io,resources=backupstoragelocations,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=velero.io,resources=backupstoragelocations/status,verbs=get;update;patch
-func (r *BackupStorageLocationReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
+func (r *BackupStorageLocationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {


We can address this in a follow up PR, but I think that now that Reconcile takes a context, we can upgrade our existing controllers to not have their own context but instead use this passed in context. Looking at the code from controller-runtime, this is the context that is now passed in to s.mgr.Start in server.go. I'll open an issue to address this.

Created #3353

Signed-off-by: Ron Green <[email protected]>

this fix is from my understanding of the context package, can be fixed later Signed-off-by: Ron Green <[email protected]>

georgettica · 2021-01-26T18:41:18Z

I think I have a solution, hope it works as I expect

georgettica · 2021-01-26T18:47:40Z

EEEverything passes ❤️

zubron

Thanks for making the fixes! LGTM 🎉

For those interested, I tested this out by creating a small Go project which imports the Velero package. When including require github.com/vmware-tanzu/velero v1.5.3, I get the error about the missing version: k8s.io/[email protected]: reading k8s.io/api/go.mod at revision v0.0.0: unknown revision v0.0.0. If I add a replace directive to use the branch from this PR (replace github.com/vmware-tanzu/velero => github.com/georgettica/velero v1.5.2-0.20210126184033-cba96280fdc3), it builds correctly without error.

georgettica · 2021-01-26T21:00:24Z

Yes!! Thanks :)

[Non-blocker] Can this go.mod be incorporated I to the repo u til everyone has transitioned to working with velero directly?

zubron · 2021-01-26T22:15:59Z

[Non-blocker] Can this go.mod be incorporated I to the repo u til everyone has transitioned to working with velero directly?

Sorry, I'm not sure what you mean by this? Once this PR is merged, it will be available to use, although it won't be an official release. We're working on v1.6 at the moment which will include this fix, so it will be available as an official release then. If people want to use it before that, then they can use versions from the main branch.

jenting

LGTM, thank you.

github-actions bot requested review from ashish-amarnath, carlisia, dsu-igeek, jenting, nrb and zubron December 23, 2020 09:23

github-actions bot assigned georgettica Dec 23, 2020

georgettica force-pushed the georgettica/bump-external-snapshotter-version branch from 08a122d to 1eeb100 Compare December 23, 2020 09:27

georgettica commented Dec 23, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

zubron suggested changes Jan 4, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

github-actions bot requested a review from zubron January 7, 2021 08:50

georgettica mentioned this pull request Jan 16, 2021

Cannot install velero with go get + go install #2966

Closed

georgettica force-pushed the georgettica/bump-external-snapshotter-version branch from f26adb7 to f684637 Compare January 26, 2021 09:51

georgettica force-pushed the georgettica/bump-external-snapshotter-version branch 2 times, most recently from b761370 to 1a9aca1 Compare January 26, 2021 10:29

georgettica force-pushed the georgettica/bump-external-snapshotter-version branch from 64c5d84 to 0c041cf Compare January 26, 2021 10:50

refactor(gomod): move replaces

5d72a06

Signed-off-by: Ron Green <[email protected]>

georgettica force-pushed the georgettica/bump-external-snapshotter-version branch 2 times, most recently from 6a2dc54 to 57db4e0 Compare January 26, 2021 10:57

georgettica added 6 commits January 26, 2021 13:06

refactor(external-snapshotter): bump to v4

861cc78

Signed-off-by: Ron Green <[email protected]>

chore(changelog): add changelog message

c4484d1

Signed-off-by: Ron Green <[email protected]>

fix: apply patch

ef07b72

Signed-off-by: Nolan Brubaker <[email protected]> Signed-off-by: Ron Green <[email protected]>

fix(tests): make tests pass

f0472bd

- change to new api resource not all tests are passing, but most of them do Signed-off-by: Ron Green <[email protected]>

chore(update): run 'make update'

a27824d

Signed-off-by: Ron Green <[email protected]>

georgettica force-pushed the georgettica/bump-external-snapshotter-version branch from 8c0adea to a27824d Compare January 26, 2021 11:06

georgettica commented Jan 26, 2021

View reviewed changes

config/crd/bases/velero.io_restores.yaml Show resolved Hide resolved

zubron suggested changes Jan 26, 2021

View reviewed changes

zubron mentioned this pull request Jan 26, 2021

Use manager context in controller Reconcile functions #3353

Closed

chore(gomod): bump k8s version

f4355f6

Signed-off-by: Ron Green <[email protected]>

github-actions bot requested a review from zubron January 26, 2021 18:21

fix(tests): make tests pass?

cba9628

this fix is from my understanding of the context package, can be fixed later Signed-off-by: Ron Green <[email protected]>

zubron approved these changes Jan 26, 2021

View reviewed changes

jenting approved these changes Jan 26, 2021

View reviewed changes

jenting merged commit e4b6f94 into vmware-tanzu:main Jan 26, 2021

georgettica deleted the georgettica/bump-external-snapshotter-version branch January 26, 2021 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Georgettica/bump external snapshotter version (fixes #2966) #3202

Georgettica/bump external snapshotter version (fixes #2966) #3202

georgettica commented Dec 23, 2020 •

edited

Loading

georgettica commented Dec 23, 2020

zubron left a comment

georgettica commented Jan 4, 2021

zubron commented Jan 5, 2021

jenting commented Jan 6, 2021

georgettica commented Jan 6, 2021

georgettica commented Jan 7, 2021

jenting commented Jan 13, 2021

nrb commented Jan 22, 2021

nrb commented Jan 22, 2021

georgettica commented Jan 22, 2021

arianitu commented Jan 25, 2021

georgettica commented Jan 25, 2021

georgettica commented Jan 26, 2021

georgettica commented Jan 26, 2021

georgettica commented Jan 26, 2021

georgettica commented Jan 26, 2021

georgettica commented Jan 26, 2021 •

edited

Loading

zubron left a comment

zubron Jan 26, 2021

zubron Jan 26, 2021

georgettica commented Jan 26, 2021

georgettica commented Jan 26, 2021 •

edited

Loading

zubron left a comment

georgettica commented Jan 26, 2021

zubron commented Jan 26, 2021

jenting left a comment

Georgettica/bump external snapshotter version (fixes #2966) #3202

Georgettica/bump external snapshotter version (fixes #2966) #3202

Conversation

georgettica commented Dec 23, 2020 • edited Loading

georgettica commented Dec 23, 2020

zubron left a comment

Choose a reason for hiding this comment

georgettica commented Jan 4, 2021

zubron commented Jan 5, 2021

jenting commented Jan 6, 2021

georgettica commented Jan 6, 2021

georgettica commented Jan 7, 2021

jenting commented Jan 13, 2021

nrb commented Jan 22, 2021

nrb commented Jan 22, 2021

georgettica commented Jan 22, 2021

arianitu commented Jan 25, 2021

georgettica commented Jan 25, 2021

georgettica commented Jan 26, 2021

georgettica commented Jan 26, 2021

georgettica commented Jan 26, 2021

georgettica commented Jan 26, 2021

georgettica commented Jan 26, 2021 • edited Loading

zubron left a comment

Choose a reason for hiding this comment

zubron Jan 26, 2021

Choose a reason for hiding this comment

zubron Jan 26, 2021

Choose a reason for hiding this comment

georgettica commented Jan 26, 2021

georgettica commented Jan 26, 2021 • edited Loading

zubron left a comment

Choose a reason for hiding this comment

georgettica commented Jan 26, 2021

zubron commented Jan 26, 2021

jenting left a comment

Choose a reason for hiding this comment

georgettica commented Dec 23, 2020 •

edited

Loading

georgettica commented Jan 26, 2021 •

edited

Loading

georgettica commented Jan 26, 2021 •

edited

Loading