initial imagestream plugin work: logs without copying local images #1

sseago · 2019-03-20T19:27:21Z

This includes the basic logic needed for the backup plugin. The skopeo log
statements will be replaced with actual image copying once the skopeo
code is made available, and the necessary velero validation changes to support
the shared s3-backed registry are put in place.

arapov

I guess it looks good to me with minor changes.
Also I would use empty lines to split the blocks within the functions for better readability, e.g:

func (p *BackupPlugin) coreClient() (*corev1.CoreV1Client, error) {
	config, err := rest.InClusterConfig()
	if err != nil {
		return nil, err
	}

	client, err := corev1.NewForConfig(config)
	if err != nil {
		return nil, err
	}
	
	return client, nil
}

I didn't jump into p.Log.Info() facility, so I can be wrong... But I feel like we may want to use it to report errors too.

arapov · 2019-03-21T08:37:24Z

velero-plugins/imagestream/backup.go

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/rest"

 	"github.com/heptio/velero/pkg/apis/velero/v1"


I would also change this to
v1 "github.com/heptio/velero/pkg/apis/velero/v1"

it makes easier for me to navigate through */v1 imports later...

arapov · 2019-03-21T08:38:24Z

velero-plugins/imagestream/backup.go

+type imagePolicyConfig struct {
+	InternalRegistryHostname string `json:"internalRegistryHostname"`
+}
+type ApiServerConfig struct {


Do we want this exported?

Good question. I copied this bit from one of the other plugin files. I suspect we either want this not exported, or we want it exported and shared among the plugin files. @dymurray what do you think?

arapov · 2019-03-21T08:39:40Z

velero-plugins/imagestream/backup.go

+        }
+    }
+    return nil
+}


findSpecTag idented with spaces instead of tabs. It must be tabs, unless required we have a strong reason for this.

Good catch. There's no reason for this. It happened because I pasted and modified the code from elsewhere and I didn't notice the indent problem in my editor.

And yeah, I'd forgotten about gofmt.

arapov · 2019-03-21T08:40:17Z

velero-plugins/imagestream/backup.go

+        }
+    }
+    return nil
+}


ditto. same comment as for findSpecTag applies

arapov · 2019-03-21T08:41:21Z

velero-plugins/imagestream/backup.go

+
+		}
+		return internalRegistry, nil
+	} else if  strings.HasPrefix(ocpVersion, "4.") {


unneeded extra space after if. @sseago you may want to run gofmt and golint before creating pull requests.

arapov · 2019-03-21T08:41:56Z

velero-plugins/imagestream/backup.go

+
+func findStatusTag(tags []imagev1API.NamedTagEventList, name string) (*imagev1API.NamedTagEventList) {
+    for _, tag := range tags {
+        if tag.Tag==name {


add spaces around ==

arapov · 2019-03-21T08:45:02Z

velero-plugins/imagestream/backup.go

+		internalRegistry := registrySvc.Spec.ClusterIP
+		if registrySvc.Spec.Ports[0].Port != 80 {
+			internalRegistry = internalRegistry + ":" + strconv.Itoa(int(registrySvc.Spec.Ports[0].Port))
+


remove newline

sseago · 2019-03-21T13:17:39Z

@arapov OK, I fixed the gofmt issues. We still need to resolve the ApiServerConfig export question. Regarding the logging, I know that passing the error back causes velero to log the error message along with failing the backup/restore. Perhaps we also want to add a separate log message from the plugin here? @dymurray what do you think? Is it just extra code that duplicates messages, or is there some value in seeing the error log message from the plugin directly?

dymurray

Looks great to me. I noticed you use len(string) to check for empty strings. I think doing that and doing if string != "" is both used in golangs string libraries so good with me.

dymurray · 2019-03-21T14:22:49Z

velero-plugins/imagestream/backup.go

+			return "", err
+		}
+		internalRegistry := registrySvc.Spec.ClusterIP
+		if registrySvc.Spec.Ports[0].Port != 80 {


Why are we checking to ensure it's not port 80 here?

So I think 1) 80 may be the wrong port anyway and 2) it's probably unnecessary. I was trying to account for the case where the dockerImageReferences in the local registry were missing the (optional) port number, but I haven't actually seen any cases where this is true. Even if you use an exposed route for the registry and push/tag externally using this route path (without a port), internally the :5000 gets added, at least from what I've seen. I'll remove the check here and assume the port will always be shown. This is only relevant for 3.x anyway, as for 4.x we can pull the registry path directly from the configMap.

dymurray · 2019-03-21T14:38:31Z

@sseago Re: error messages, since you are still developing on this I don't mind having the extra error messages since it helps debugging. Long term I say remove them and use velero's log message when you return the error.

arapov · 2019-03-21T15:25:07Z

Looks great to me. I noticed you use len(string) to check for empty strings. I think doing that and doing if string != "" is both used in golangs string libraries so good with me.

Both are indeed valid, though I would keep the way the code is more clear and gives context. For instance, if we want to look at element x then len(string) > x, even for x == 0 and if we care about "this very specific string" then write string == "".
Also I remember folks were comparing against len() in the past due to compiler optimization. Since 2011 or so it is not true anymore. I'd prefer == "" here.

sseago · 2019-03-21T15:49:33Z

Both are indeed valid, though I would keep the way the code is more clear and gives context. For instance, if we want to look at element x then len(string) > x, even for x == 0 and if we care about "this very specific string" then write string == "".

In this case, I'm just trying to do the equivalent of checking for nil. What's the clearest way of doing that for strings in go? Either len(x)==0, or x=="" works, so it's a question clarity/style.

dymurray · 2019-03-21T20:09:23Z

I generally see x == "" more frequently, but go's internal strings package uses both so I am cool with either. Just a stylistic approach.

This includes the basic logic needed for the backup plugin. The skopeo log statements will be replaced with actual image copying once the skopeo code is made available, and the necessary velero validation changes to support the shared s3-backed registry are put in place.

General velero plugin implementation

arapov suggested changes Mar 21, 2019

View reviewed changes

sseago force-pushed the imagestream-backup branch from 66ab0c1 to 04ef725 Compare March 21, 2019 13:15

dymurray approved these changes Mar 21, 2019

View reviewed changes

sseago force-pushed the imagestream-backup branch from 04ef725 to 7efbce4 Compare March 21, 2019 15:11

arapov approved these changes Mar 21, 2019

View reviewed changes

sseago force-pushed the imagestream-backup branch from 7efbce4 to 55dafef Compare March 21, 2019 23:55

sseago merged commit feeb23f into fusor:master Mar 22, 2019

alexander-demicev pushed a commit to alexander-demicev/ocp-velero-plugin that referenced this pull request Oct 9, 2019

Merge pull request fusor#1 from sseago/first-working-version

b23e553

General velero plugin implementation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial imagestream plugin work: logs without copying local images #1

initial imagestream plugin work: logs without copying local images #1

sseago commented Mar 20, 2019

arapov left a comment •

edited

Loading

arapov Mar 21, 2019 •

edited

Loading

arapov Mar 21, 2019

sseago Mar 21, 2019

arapov Mar 21, 2019

sseago Mar 21, 2019

sseago Mar 21, 2019

arapov Mar 21, 2019

arapov Mar 21, 2019

arapov Mar 21, 2019

arapov Mar 21, 2019

sseago commented Mar 21, 2019

dymurray left a comment

dymurray Mar 21, 2019

sseago Mar 21, 2019

dymurray commented Mar 21, 2019

arapov commented Mar 21, 2019 •

edited

Loading

sseago commented Mar 21, 2019

dymurray commented Mar 21, 2019

initial imagestream plugin work: logs without copying local images #1

initial imagestream plugin work: logs without copying local images #1

Conversation

sseago commented Mar 20, 2019

arapov left a comment • edited Loading

Choose a reason for hiding this comment

arapov Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sseago commented Mar 21, 2019

dymurray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dymurray commented Mar 21, 2019

arapov commented Mar 21, 2019 • edited Loading

sseago commented Mar 21, 2019

dymurray commented Mar 21, 2019

arapov left a comment •

edited

Loading

arapov Mar 21, 2019 •

edited

Loading

arapov commented Mar 21, 2019 •

edited

Loading