User can specify the namespace in yaml for knative-serving #2708

zxDiscovery · 2018-12-13T06:11:35Z

Proposed Changes

User can set knative-serving deployment namespace in yaml file. The default namespace for knative-serving is also knative-serving

Release Note

NONE

knative-prow-robot

@zxDiscovery: 0 warnings.

In response to this:

Fixes #2703

Proposed Changes

User can set knative-serving deployment namespace in yaml file. The default namespace for knative-serving is also knative-serving

Release Note
NONE

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.

markusthoemmes · 2018-12-13T10:04:14Z

@zxDiscovery Thanks for your PR. Is this work in progress? As it stands, how'd we actually configure the namespace when deploying Knative?

zxDiscovery · 2018-12-13T11:28:21Z

@markusthoemmes I tested it based on the code of v0.2.2 and it works.
For example. If user want to specify the namespace knative-system for deployment autoscaler, it can set in the deployment yaml file as follows:

kind: Deployment
metadata:
  name: autoscaler
  namespace: knative-system
spec:
  template:
    spec:
      containers:
        env:
        - name: SYSTEM_NAMESPACE
          valueFrom:
            fieldRef:
              fieldPath: metadata.namespace

markusthoemmes · 2018-12-13T11:37:26Z

Right, is this how we actually want to ship this? Is there a way to globally set this value in a config-map or something like that?

zxDiscovery · 2018-12-13T12:11:30Z

This method was copied from knative-build. If this method is not well, I will find anther better solution.

zxDiscovery · 2018-12-14T05:29:03Z

@markusthoemmes I think this method to set the namespace is simple. If we just use config-map to set the namespace, it may be more complicated for this.

markusthoemmes · 2018-12-14T06:20:55Z

@zxDiscovery Yep, the knative-build commit makes sense to me. Should we add the same changes here (setting the Environment variable based on the namespace the YAML is applied to for each deployment)?

zxDiscovery · 2018-12-14T06:59:15Z

@markusthoemmes The current knative serving, build and eventing are deployed in three namespaces. They are all belong to knative, and why we don't put them into the same namespaces so that the chart can management easily. I hear there is ongoing work to explore make this configurable(#2623). If we want to this configuration work, knative-serving need to be deployed in some other namespace. So I think we need add the same changes here.

markusthoemmes · 2018-12-14T07:51:11Z

@zxDiscovery right. I'm referring to the changes that've been made when build introduced this mechanism. See this commit: knative/build@64938e4.

It also adds the SYSTEM_NAMESPACE environment variable to every deployment. Would it make sense to do the same here?

zxDiscovery · 2018-12-14T08:30:08Z

@markusthoemmes Do you mean the environment variable SYSTEM_NAMESPACE is duplicate and may cause conflicts?

markusthoemmes · 2018-12-14T08:45:07Z

No I'm speaking of the changes that PR in build made:

It took the variable from the environment if it's there (same as you did here)
It added

env:
  - name: SYSTEM_NAMESPACE
    valueFrom:
      fieldRef:
        fieldPath: metadata.namespace

To all deployments to actually set that value accordingly.

Shall we do the second step here as well?

zxDiscovery · 2018-12-14T09:13:33Z

@markusthoemmes Oh I see. Yes, we need do this for the yaml file. I will update this.

markusthoemmes · 2018-12-19T11:49:03Z

/ok-to-test

To me, the approach seems fine as it's also already used over in build.

/assign @mattmoor

greghaynes · 2018-12-19T23:42:59Z

pkg/system/names.go

 )
+
+func init() {


What do you think of making this just a getter method rather than having an init which fills in a global (and therefore needs care taken to ensure its called before referencing the global)? I.e.

const ( NamespaceEnvKey = "SYSTEM_NAMESPACE" ) func Namespace() string { return os.Getenv(NamespaceEnvKey) }

But if use this method, the default namespace knative-serving can't be set. We need the default namespace knative-serving in current version.

I think @greghaynes was referring to actually adding the defaulting logic into the getter, i.e.:

const ( NamespaceEnvKey = "SYSTEM_NAMESPACE" ) func Namespace() string { if ns := os.Getenv(NamespaceEnvKey); ns != "" { return ns } return "knative-serving" }

I think init() is called automatically before running main though. Both approaches might be valid in this case, knative-build opted for the init function.

+1 on getter. Why do we need the default? Tests?

I'd like so see if we can create a shared library for this in knative/pkg to standardize this across Knative.

@mattmoor I also hope the knative-serving doesn't need the default namespace and user can free to specify the namespace. But the current knative can't put the serving, build and eventing into the same namespace. The current doc and test case are all specify the knative-serving in knative-serving namespace. And also knative-build specify in knative-build namespace. So I follow by the code of knative-build to change here.

I understand that we can't (yet) put things in the same namespace, but this is one of a number of changes to move in that direction.

I'd like to see us have a shared library for this in knative/pkg that folks can pull in to determine the system namespace, assuming it's configured as you have in the yamls.

I'm wondering when the defaulting here actually kicks in (tests?), so that we can take steps to address that.

After I check the commit history, the namespace is hardcode(#1101) in pkg/names.go from the beginning. I think this is not only for testing, but also for deployment.

// GetServingSystemNamespace returns the namespace where // serving controllers are deployed to. func GetServingSystemNamespace() string { return "knative-serving-system" }

After #1549, pkg/names.go move to pkg/system/names.go, the code has changed as follows:

package system const ( // Namespace holds the K8s namespace where our serving system // components run. Namespace = "knative-serving" )

I think @mattmoor's question was:

Once you apply your change of reading the namespace from environment (including the YAML changes), is there a place where the default is actually used (hence: do we even need it?).

Your changes make sure that all of our components are properly configured so either there is a place somewhere that isn't configured (and might need to be?) or we don't need the default at all.

The default namespaces is unnecessary. The getter method is better than current. But there will be more code changes. I have updated the code.

zxDiscovery · 2019-01-08T06:31:02Z

/retest

zxDiscovery · 2019-01-08T06:58:23Z

/retest

zxDiscovery · 2019-01-08T07:28:03Z

/retest

zxDiscovery · 2019-01-17T09:59:42Z

cmd/queue/main.go

@@ -303,7 +302,7 @@ func main() {
 	}()

 	// Open a websocket connection to the autoscaler
-	autoscalerEndpoint := fmt.Sprintf("ws://%s.%s.svc.%s:%d", servingAutoscaler, system.Namespace, utils.GetClusterDomainName(), servingAutoscalerPort)
+	autoscalerEndpoint := fmt.Sprintf("ws://%s.%s.svc.%s:%d", servingAutoscaler, "knative-serving", utils.GetClusterDomainName(), servingAutoscalerPort)


Here, we must get the namespace knative-serving which deploy the service autoscaler. If we use system.Namespace() only can get default namespaces knative-testing if we set the default value in test lib. Do you have some better method to get the namespaces which autoscaler deployed. @mattmoor

This means that we're somehow getting the test library linked into one of our application binaries, which is troubling.

zxDiscovery · 2019-01-17T15:58:37Z

cmd/queue/main.go

@@ -93,6 +93,7 @@ func initEnv() {
 	servingNamespace = util.GetRequiredEnvOrFatal("SERVING_NAMESPACE", logger)
 	servingRevision = util.GetRequiredEnvOrFatal("SERVING_REVISION", logger)
 	servingAutoscaler = util.GetRequiredEnvOrFatal("SERVING_AUTOSCALER", logger)
+	autoscalerNamespace = util.GetRequiredEnvOrFatal("AUTOSCALER_NAMESPACE", logger)


Can we use the environment value to transmit the autoscaler namespace?@mattmoor

I don't understand why this is different from system.Namespace(), I'd rather chase the root cause than work around it like this.

Ok, I understand (sorry it took so long).

I would change this to system.Namespace(), and... (see next comment)

zxDiscovery · 2019-01-17T16:06:09Z

pkg/reconciler/v1alpha1/revision/resources/queue.go

@@ -131,6 +131,9 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, a
 		}, {
 			Name:  "USER_PORT",
 			Value: strconv.Itoa(int(userPort)),
+		}, {
+			Name:  "AUTOSCALER_NAMESPACE",
+			Value: "knative-serving",


The queue sidecar's container spec created by the function makeQueueContainer. I can add an environment value to transmit the knative-serving. But the namespace at here is also hardcode. I don't know is this right? @mattmoor

I would change this to:

Name: system.NamespaceEnvKey, Value: system.Namespace(),

mattmoor · 2019-01-18T03:22:45Z

pkg/reconciler/v1alpha1/revision/resources/queue.go

@@ -135,6 +136,9 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, a
 		}, {
 			Name:  "USER_PORT",
 			Value: strconv.Itoa(int(userPort)),
+		}, {
+			Name:  system.NamespaceEnvKey,


I think this requires an update to queue_test.go, which will need to link our new library.

knative-metrics-robot · 2019-01-18T06:07:44Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/autoscaling/kpa/kpa.go	87.5%	84.1%	-3.4

mattmoor · 2019-01-18T06:27:56Z

pkg/reconciler/v1alpha1/revision/resources/queue_test.go

@@ -381,6 +393,9 @@ func TestMakeQueueContainer(t *testing.T) {
 			}, {
 				Name:  "USER_PORT",
 				Value: strconv.Itoa(v1alpha1.DefaultUserPort),
+			}, {
+				Name:  "SYSTEM_NAMESPACE",
+				Value: "knative-testing",


we generally use system.Namespace() to avoid hard-coding the string.

This makes `pkg/system` panic if `SYSTEM_NAMESPACE` is not set. For tests, it exposes a simple library you can link to set this variable to a suitable namespace.

knative-metrics-robot · 2019-01-18T08:07:24Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/autoscaling/kpa/kpa.go	84.1%	87.5%	3.4

mattmoor

/lgtm
/approve

knative-prow-robot · 2019-01-18T14:31:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, zxDiscovery

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:

~~OWNERS~~ [mattmoor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow-robot requested review from mdemirhan and vaikas December 13, 2018 06:11

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 13, 2018

knative-prow-robot reviewed Dec 13, 2018

View reviewed changes

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 13, 2018

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 14, 2018

zxDiscovery force-pushed the serving-namespace branch from decbc75 to 97fde11 Compare December 17, 2018 06:48

knative-prow-robot assigned mattmoor Dec 19, 2018

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 19, 2018

greghaynes reviewed Dec 19, 2018

View reviewed changes

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 7, 2019

zxDiscovery force-pushed the serving-namespace branch 2 times, most recently from 2d47ccd to 18f067f Compare January 8, 2019 06:05

zxDiscovery force-pushed the serving-namespace branch 2 times, most recently from 50e45de to 83e6150 Compare January 17, 2019 09:00

zxDiscovery commented Jan 17, 2019

View reviewed changes

zxDiscovery force-pushed the serving-namespace branch from cbb6729 to c1327d7 Compare January 18, 2019 01:54

mattmoor reviewed Jan 18, 2019

View reviewed changes

zxDiscovery force-pushed the serving-namespace branch from c1327d7 to 9d46532 Compare January 18, 2019 05:41

mattmoor reviewed Jan 18, 2019

View reviewed changes

zxDiscovery force-pushed the serving-namespace branch from 49941fd to 7b7d4b5 Compare January 18, 2019 06:38

zxDiscovery and others added 9 commits January 18, 2019 16:02

User can specify the namespace in yaml for knative-serving

14e653b

use getter method for namespaces

57b1ada

add default namespaces for test

0bcb2f5

panic if the SYSTEM_NAMESPACE environment variable isn't set

b882bd9

This makes `pkg/system` panic if `SYSTEM_NAMESPACE` is not set. For tests, it exposes a simple library you can link to set this variable to a suitable namespace.

update namespace in crd_checks.go

555b83e

change test ns knative-serving to knative-testing

a16b958

use environment value set the autoscaler namespaces

1f4788f

queue get env from system.Namespace

0118a55

update the test case to match the new environment value

d59ab7f

zxDiscovery force-pushed the serving-namespace branch from 7b7d4b5 to d59ab7f Compare January 18, 2019 08:04

mattmoor reviewed Jan 18, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2019

knative-prow-robot merged commit 3d137ac into knative:master Jan 18, 2019

zxDiscovery mentioned this pull request Jan 22, 2019

Add namespaces in knative/pkg knative/pkg#235

Merged

zxDiscovery deleted the serving-namespace branch January 23, 2019 06:57

bobcatfish mentioned this pull request Jan 24, 2019

allow default system namespace to be overridden using an env var tektoncd/pipeline#427

Merged

User can specify the namespace in yaml for knative-serving #2708

User can specify the namespace in yaml for knative-serving #2708

Conversation

zxDiscovery commented Dec 13, 2018

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

markusthoemmes commented Dec 13, 2018

zxDiscovery commented Dec 13, 2018

markusthoemmes commented Dec 13, 2018

zxDiscovery commented Dec 13, 2018

zxDiscovery commented Dec 14, 2018

markusthoemmes commented Dec 14, 2018

zxDiscovery commented Dec 14, 2018

markusthoemmes commented Dec 14, 2018

zxDiscovery commented Dec 14, 2018

markusthoemmes commented Dec 14, 2018

zxDiscovery commented Dec 14, 2018

markusthoemmes commented Dec 19, 2018

greghaynes Dec 19, 2018 • 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

zxDiscovery commented Jan 8, 2019

zxDiscovery commented Jan 8, 2019

zxDiscovery commented Jan 8, 2019

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

knative-metrics-robot commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jan 18, 2019

mattmoor left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 18, 2019

greghaynes Dec 19, 2018 •

edited

Loading