Support manifest on riff system install #691

glyn · 2018-08-24T13:23:57Z

Fixes #648

ericbottard · 2018-08-24T13:29:11Z

cmd/commands/system.go

+components to be installed. The manifest file contents should be of the following form:
+
+version: 0.1
+istio-crds: https://path/to/istio-crds.yaml


Shouldn't this be an underscore?

Indeed. Well spotted!

trisberg · 2018-08-24T22:06:05Z

docs/riff_system_install.md

+Use the '--manifest' flag to specify the path of a manifest file which provides the URLs of the YAML files to be applied.
+The manifest file contents should be of the following form:
+
+version: 0.1


the format here is different than what I see in the cmd/commands/system.go file.

I kind of like the grouping here, so if we could have the following:

istio: - https://path/to/istio.yaml knative: - https://path/to/knative-build.yaml - https://path/to/knative-serving.yaml - https://path/to/knative-eventing.yaml - https://path/to/stub-clusterbus.yaml namespace: - https://path/to/riff-buildtemplate.yaml

then we would have more flexibility in including one, two ore more files for each group

I was considering a grouping along those lines, especially since @scothis suggested it in the issue. However, there is some special processing which doesn't lend itself to this approach.

For istio, there is a 5 second delay between installing the CRDs and installing the other istio resources. Apart from the obvious race involved when deploying to a very sluggish system, I was trying not to break the current behaviour. I guess I could introduce a 5 second delay between all the items in the istio group, but that feels like a bit of a hack.

Similarly, there is a change applied to the serving YAML if and only if --node-port is specified. I am a bit nervous about applying this change to all the YAML files in the knative group because we can't tell what YAML files users will add to each group and whether such a global change would be correct or not.

@trisberg As per our chat, I'll rework in terms of an istio array with 5 second intervals between each pair and a knative array with a more specific global change. Thanks for helping me work that through. /cc @scothis

The current Istio config is a hack to work around a bug that has been fixed. There are still potential issues when applying the new istio yaml, but that can be solved by a retry on failure. A retry is not a bad idea for any applied yaml.

The NodePort/LoadBalancer swap should be "relatively" safe, we can make the match more selective if there are false positives.

how do we know when to retry? we don't really know what's in the yaml files, or which namespaces they target except for the istio install

how do we know when to retry?

When the command you're running exits with a non-zero code.

The 5 second delay between istio YAMLs may be necessary if someone uses a manifest to install istio earlier than v1.0.1. I'd prefer to defer retries to a separate issue to avoid scope creep in this issue.

I'll open a separate issue for retry. #704

scothis · 2018-08-27T11:07:45Z

pkg/core/manifest.go

+// Manifest defines the location of YAML files for system components.
+type Manifest struct {
+	Version         string
+	IstioCRDs       string `yaml:"istio_crds"`


The convention in yaml is camelCase, but I like Thomas' suggestion better.

Thanks. I'll fix this for now regardless of whether we adopt the grouping suggestion.

scothis

a few nits, otherwise lgtm

scothis · 2018-08-29T15:04:09Z

pkg/core/fixtures/valid.yaml

@@ -0,0 +1,8 @@
+version: 0.1
+istio:
+  - istio-crds


nit: is this really valid, or just good enough pass the basic validation

I suspect it's valid, but the intention was that it was just enough to test the manifest parsing.

scothis · 2018-08-29T15:07:58Z

pkg/core/system.go

@@ -236,7 +253,7 @@ func resolveReleaseURLs(filename string) (url.URL, error) {
 	if u.Scheme == "http" || u.Scheme == "https" {
 		return *u, nil
 	}
-	return *u, fmt.Errorf("filename must be file, http or https, got %s", u.Scheme)
+	return *u, fmt.Errorf("filename must be file, http or https, got %s", u.Scheme) // TODO: support file scheme


if we don't support file, remove it from the error message

scothis · 2018-08-29T15:09:25Z

pkg/core/system.go

@@ -31,7 +31,8 @@ import (
 )

 const (
-	istioNamespace  = "istio-system"
+	istioNamespace = "istio-system"
+	// NOTE: KEEP THE FOLLOWING IN STEP WITH manifest.yaml IN THE ROOT OF THIS REPOSITORY


If this is important, add a test

Yeah, beats codegen. Thanks.

glyn · 2018-08-29T15:25:32Z

pkg/core/fixtures/invalid.yaml

@@ -0,0 +1 @@
+.


Fixes projectriff#648

dturanski added the in progress label Aug 24, 2018

glyn requested a review from trisberg August 24, 2018 13:24

ericbottard reviewed Aug 24, 2018

View reviewed changes

glyn force-pushed the manifest branch from 7209c7a to 191d291 Compare August 24, 2018 13:30

trisberg reviewed Aug 24, 2018

View reviewed changes

scothis reviewed Aug 27, 2018

View reviewed changes

glyn force-pushed the manifest branch 6 times, most recently from 70f0129 to 2fd166b Compare August 29, 2018 14:47

scothis approved these changes Aug 29, 2018

View reviewed changes

glyn commented Aug 29, 2018

View reviewed changes

pkg/core/fixtures/invalid.yaml

@@ -0,0 +1 @@

.

Copy link

Contributor Author

glyn Aug 29, 2018

Choose a reason for hiding this comment

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

Fixed.

Support manifest on riff system install

fca3644

Fixes projectriff#648

glyn force-pushed the manifest branch from 2fd166b to fca3644 Compare August 29, 2018 15:36

trisberg approved these changes Aug 29, 2018

View reviewed changes

trisberg merged commit 376663e into projectriff:master Aug 29, 2018

dturanski removed the in progress label Aug 29, 2018

trisberg mentioned this pull request Aug 29, 2018

riff system install --latest #686

Closed

glyn deleted the manifest branch October 4, 2018 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support manifest on riff system install #691

Support manifest on riff system install #691

glyn commented Aug 24, 2018

ericbottard Aug 24, 2018

glyn Aug 24, 2018

trisberg Aug 24, 2018

glyn Aug 28, 2018

glyn Aug 28, 2018

scothis Aug 28, 2018

trisberg Aug 28, 2018 •

edited

Loading

scothis Aug 28, 2018

glyn Aug 29, 2018

scothis Aug 29, 2018 •

edited

Loading

scothis Aug 27, 2018

glyn Aug 28, 2018

scothis left a comment

scothis Aug 29, 2018

glyn Aug 29, 2018

scothis Aug 29, 2018

glyn Aug 29, 2018

scothis Aug 29, 2018

glyn Aug 29, 2018

glyn Aug 29, 2018

Support manifest on riff system install #691

Support manifest on riff system install #691

Conversation

glyn commented Aug 24, 2018

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

trisberg Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scothis Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scothis left a 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

trisberg Aug 28, 2018 •

edited

Loading

scothis Aug 29, 2018 •

edited

Loading